Issue 233660043: RailInteractionRecord.layoutStats table of counters
|
Created:
8 years, 11 months ago by benjhayden Modified:
8 years, 8 months ago Reviewers:
nduca CC:
trace-review_googlegroups.com Base URL:
git@github.com:google/trace-viewer@master Visibility:
Public. |
DescriptionRailInteractionRecord.layoutStats table of counters
This is still empty until associatedEvents is implemented.
This is still unused and invisible until a RAILInteractionRecordView is implemented. That will get rir.layoutStats directly.
BUG=
Patch Set 1 #
Total comments: 2
Patch Set 2 : cleaner, more tests #Patch Set 3 : optimization #
Total comments: 10
Patch Set 4 : comments #
Total comments: 13
Patch Set 5 : comments #Patch Set 6 : comments #Patch Set 7 : comments #
Total comments: 21
Patch Set 8 : comments #Patch Set 9 : comments #Patch Set 10 : comments #Patch Set 11 : rebase #
Total comments: 2
Patch Set 12 : comments #Patch Set 13 : assert.deepEqual #
MessagesTotal messages: 23
lgtm awesome
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/1/trace_viewer/extras/audits/ra... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/1/trace_viewer/extras/audits/ra... trace_viewer/extras/audits/rail_interaction_record.html:60: var layouts = this.associatedEvents.filter(function(event) { actually, can you come up with a cleaner way to write this? maybe a filtering function? this updateArgs function is way too long
Sign in to reply to this message.
PTAL https://codereview.appspot.com/233660043/diff/1/trace_viewer/extras/audits/ra... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/1/trace_viewer/extras/audits/ra... trace_viewer/extras/audits/rail_interaction_record.html:60: var layouts = this.associatedEvents.filter(function(event) { On 2015/05/19 23:17:43, nduca wrote: > actually, can you come up with a cleaner way to write this? maybe a filtering > function? > > this updateArgs function is way too long Split into summarizeLayouts, but it needs to loop over the layouts at least twice when the counters are available so it can create a well-formed table, where every column appears in every row.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:47: if (!(layouts instanceof Array) || (layouts.length === 0)) why would this be non-array? i get the length === 0 one but the other seems overkll https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:70: layouts.some(function(layout) { i think this would be a lot clearer with forEach() and just overcompute instead of stoping looking. also, why not just make the columns to be all the possible field values instead of the columns that we find? that'd simplify this code a whole lot... https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:85: return layouts.map(function(layout) { this code is really dense. i really think you should be tryign to make this cleaner. i still can't quite read it clearly enough to advise on how to do it better. the main sin you're doing here is using some, map() and concat() and then inside those functions affecting state outside their functions. that's pretty gross if you think about it. i think you'd be better off doing more explicit passes. https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:113: this.args.layouts = summarizeLayouts(this.associatedEvents.filter( args.layoutStats? this is info about layouts, not a list of layouts so... https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:115: return event.title === LAYOUT_TITLE; i think you can put the string inline here
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:47: if (!(layouts instanceof Array) || (layouts.length === 0)) On 2015/05/20 23:34:32, nduca wrote: > why would this be non-array? i get the length === 0 one but the other seems > overkll Done. https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:70: layouts.some(function(layout) { On 2015/05/20 23:34:32, nduca wrote: > i think this would be a lot clearer with forEach() and just overcompute instead > of stoping looking. Done. > > also, why not just make the columns to be all the possible field values instead > of the columns that we find? that'd simplify this code a whole lot... Yes, it would simplify the code, but it would significantly complicate the UI, and there are other ways to simplify the code. https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:85: return layouts.map(function(layout) { On 2015/05/20 23:34:32, nduca wrote: > this code is really dense. i really think you should be tryign to make this > cleaner. i still can't quite read it clearly enough to advise on how to do it > better. > > the main sin you're doing here is using some, map() and concat() and then inside > those functions affecting state outside their functions. that's pretty gross if > you think about it. > > i think you'd be better off doing more explicit passes. I was trying to be efficient, not sinful. https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:113: this.args.layouts = summarizeLayouts(this.associatedEvents.filter( On 2015/05/20 23:34:32, nduca wrote: > args.layoutStats? this is info about layouts, not a list of layouts so... Done. https://codereview.appspot.com/233660043/diff/40001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:115: return event.title === LAYOUT_TITLE; On 2015/05/20 23:34:32, nduca wrote: > i think you can put the string inline here Done.
Sign in to reply to this message.
more comments :) https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:21: var ABBREV = { is there a better name that we can pick here? imagine someone reading this code for the first time... https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:45: function getColumns(layouts) { idle question... this feels kinda like your table figuring outting code in the layout tree view too... is there common code opportunity here? https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:82: return null; we almost never use null. undefined if we must, but i'm curious why not return count, total, avg with valid things [avg being nan] https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:91: return {'count': layouts.length, we really should augment time_span to accept an array of durations, and then have the span itself show an average with tooltip for count, stddev, total. check out how we have timespan and timestamp, and then base/time.html have objecty wrappers around doubles. whats really cool here is that gov now recognizes these objects and actually creates the timestamp and timespan widgets automagically. i think we want to augment this even further. in time.html, we could have ArrayOfTimeStamps which has a list of timestamps [like how SliceGroup works, not a subclass of array since thats crazy but contains an array]. Then, we modify gov to recognize that type and create a TimeSpan object again, and pass this array object. Point being, internallyGoV can then do this display automatically. Make sense? Again, I"m not necessairly blocking this patch on this but trying to give context about code health and stuff we could do to reduce ad hoc code like this throughout the codebase while being blocked on review. :) https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record_test.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record_test.html:82: rir.associatedEvents = model.rendererMain.sliceGroup.slices; push not overwrite https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record_test.html:89: no good coverage for the counters case...
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:29: LayoutObjectsThatHadNeverHadLayout: 'new', i finally figured out what was confusing me with this table. in your layout tree dumps, you dont have to rename field names because they're dumped without such obscenely long names. yet here, we have obscenely long names. why not just modify the c++ code to not require renaming? i think that the approach being used for having renaming is actually destructive to hackability: if you want to add a new field to the c++ side, we have to also modify the ui [or have awkward names]. i'd vastly prefer the ui to be a passthrough and just doing totalling.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:21: var ABBREV = { On 2015/05/27 02:52:38, nduca wrote: > is there a better name that we can pick here? imagine someone reading this code > for the first time... Done. https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:45: function getColumns(layouts) { On 2015/05/27 02:52:38, nduca wrote: > idle question... this feels kinda like your table figuring outting code in the > layout tree view too... > > is there common code opportunity here? Could I make table builder detect when a column contains only empty strings and just remove/hide the column? If there are tables that want empty columns, could I make it a new mode like tableBuilder.removeEmptyColumns = true? https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:82: return null; On 2015/05/27 02:52:38, nduca wrote: > we almost never use null. undefined if we must, but i'm curious why not return > count, total, avg with valid things [avg being nan] Done. https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record_test.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record_test.html:82: rir.associatedEvents = model.rendererMain.sliceGroup.slices; On 2015/05/27 02:52:39, nduca wrote: > push not overwrite Done. https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record_test.html:89: On 2015/05/27 02:52:39, nduca wrote: > no good coverage for the counters case... Sorry, what doesn't the first test('layoutCounters') cover?
Sign in to reply to this message.
> https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... > trace_viewer/extras/audits/rail_interaction_record_test.html:89: > On 2015/05/27 02:52:39, nduca wrote: > > no good coverage for the counters case... > > Sorry, what doesn't the first test('layoutCounters') cover? you dont have coverage for the case that you have detailed information on layouts. you're not testing any of the individual counters.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... File trace_viewer/extras/audits/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/80001/trace_viewer/extras/audit... trace_viewer/extras/audits/rail_interaction_record.html:29: LayoutObjectsThatHadNeverHadLayout: 'new', On 2015/05/27 05:02:37, nduca wrote: > i finally figured out what was confusing me with this table. in your layout tree > dumps, you dont have to rename field names because they're dumped without such > obscenely long names. > > yet here, we have obscenely long names. > > why not just modify the c++ code to not require renaming? i think that the > approach being used for having renaming is actually destructive to hackability: > if you want to add a new field to the c++ side, we have to also modify the ui > [or have awkward names]. i'd vastly prefer the ui to be a passthrough and just > doing totalling. Actually, the layout tree viewer does rename fields in a way: it displays hard-coded column headings, not the dumped property names. That said, I would also love to let blink produce displayable strings: https://codereview.chromium.org/1152213007 If those 20 lines LGTY then I'll see if I can get them by a blink reviewer.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:68: var averages = {'host': 'AVG'}; Is this format to set it up for table builder? Seems like we should interject this later? If we aren't using table builder then it's just extra rows. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:82: return {'count': 0, 'totalMs': 0, 'avgMs': NaN}; Why NaN and not 0? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:99: row.host = layout.args.counters.host; What does host mean? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:100: row.ms = layout.duration; We typically use duration in the code not ms to hold duration information.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:68: var averages = {'host': 'AVG'}; On 2015/05/29 20:43:10, dsinclair wrote: > Is this format to set it up for table builder? Seems like we should interject > this later? If we aren't using table builder then it's just extra rows. RAILInteractionRecord is a model class, not view. RAILInteractionRecord.args is rendered as a generic object view, which might be changed to use table builder when isTable(object): https://codereview.appspot.com/238620043 Yes, in the future, it might be neat to move the sums, averages, and standard deviations rows into table builder as footer rows. Generic object view's isTable() function might also move into table builder. Table builder might also change names soon. There are lots of opportunities to improve table builder, some of which might affect more than that 1 file. Does all that need to block this CL? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:82: return {'count': 0, 'totalMs': 0, 'avgMs': NaN}; On 2015/05/29 20:43:10, dsinclair wrote: > Why NaN and not 0? http://math.stackexchange.com/questions/909395/what-is-the-average-of-no-numbers https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:99: row.host = layout.args.counters.host; On 2015/05/29 20:43:10, dsinclair wrote: > What does host mean? Like, in general? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:100: row.ms = layout.duration; On 2015/05/29 20:43:10, dsinclair wrote: > We typically use duration in the code not ms to hold duration information. Done.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:68: var averages = {'host': 'AVG'}; On 2015/05/29 21:19:55, benjhayden wrote: > On 2015/05/29 20:43:10, dsinclair wrote: > > Is this format to set it up for table builder? Seems like we should interject > > this later? If we aren't using table builder then it's just extra rows. > > RAILInteractionRecord is a model class, not view. RAILInteractionRecord.args is > rendered as a generic object view, which might be changed to use table builder > when isTable(object): https://codereview.appspot.com/238620043 > > Yes, in the future, it might be neat to move the sums, averages, and standard > deviations rows into table builder as footer rows. Generic object view's > isTable() function might also move into table builder. Table builder might also > change names soon. There are lots of opportunities to improve table builder, > some of which might affect more than that 1 file. > Does all that need to block this CL? If this is a model class then what does {'host': 'AVG'} mean? and {'host': 'SUM'} in the above data? Seems like it shouldn't be there? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:82: return {'count': 0, 'totalMs': 0, 'avgMs': NaN}; On 2015/05/29 21:19:55, benjhayden wrote: > On 2015/05/29 20:43:10, dsinclair wrote: > > Why NaN and not 0? > > http://math.stackexchange.com/questions/909395/what-is-the-average-of-no-numbers Let's just use 0. There is less chance of us causing an exception by trying to do something with it. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:99: row.host = layout.args.counters.host; On 2015/05/29 21:19:55, benjhayden wrote: > On 2015/05/29 20:43:10, dsinclair wrote: > > What does host mean? > > Like, in general? Yes, what is layout.args.counts.host? Like, www.google.com? Something else, what does host mean here? I have no idea. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:103: row[colAbbrev] = layout.args.counters[colVerbose] || 0; I think this should wait until we figure out the intermediary structure in the layout view design doc. We don't want code throughout the system accessing the args like this. What if we change the way blink outputs in the future? We don't want to have to update a whole pile of places, better just a single object needing updating.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:68: var averages = {'host': 'AVG'}; On 2015/05/29 21:41:11, dsinclair wrote: > On 2015/05/29 21:19:55, benjhayden wrote: > > On 2015/05/29 20:43:10, dsinclair wrote: > > > Is this format to set it up for table builder? Seems like we should > interject > > > this later? If we aren't using table builder then it's just extra rows. > > > > RAILInteractionRecord is a model class, not view. RAILInteractionRecord.args > is > > rendered as a generic object view, which might be changed to use table builder > > when isTable(object): https://codereview.appspot.com/238620043 > > > > Yes, in the future, it might be neat to move the sums, averages, and standard > > deviations rows into table builder as footer rows. Generic object view's > > isTable() function might also move into table builder. Table builder might > also > > change names soon. There are lots of opportunities to improve table builder, > > some of which might affect more than that 1 file. > > Does all that need to block this CL? > > > If this is a model class then what does {'host': 'AVG'} mean? and {'host': > 'SUM'} in the above data? Seems like it shouldn't be there? Done. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:82: return {'count': 0, 'totalMs': 0, 'avgMs': NaN}; On 2015/05/29 21:41:11, dsinclair wrote: > On 2015/05/29 21:19:55, benjhayden wrote: > > On 2015/05/29 20:43:10, dsinclair wrote: > > > Why NaN and not 0? > > > > > http://math.stackexchange.com/questions/909395/what-is-the-average-of-no-numbers > > Let's just use 0. There is less chance of us causing an exception by trying to > do something with it. How could NaN cause an exception? Isn't the point of NaN to avoid exceptions? https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:99: row.host = layout.args.counters.host; On 2015/05/29 21:41:11, dsinclair wrote: > On 2015/05/29 21:19:55, benjhayden wrote: > > On 2015/05/29 20:43:10, dsinclair wrote: > > > What does host mean? > > > > Like, in general? > > > Yes, what is layout.args.counts.host? Like, http://www.google.com Something else, what > does host mean here? I have no idea. It looks like you had at least one idea, and it was correct. I'm still not sure what "host" could possibly mean in this context besides an internet host, so I don't know what options I need to distinguish among, so I'm not sure if the comment that I added is sufficient. Here's a menu of possible meanings for "host" in the entire English language. Could you point out the possible meanings that might apply in this context besides the first entry "Host (network)" under Computing, which is the only entry directly under Computing? http://en.wikipedia.org/wiki/Host https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:103: row[colAbbrev] = layout.args.counters[colVerbose] || 0; On 2015/05/29 21:41:12, dsinclair wrote: > I think this should wait until we figure out the intermediary structure in the > layout view design doc. We don't want code throughout the system accessing the > args like this. What if we change the way blink outputs in the future? We don't > want to have to update a whole pile of places, better just a single object > needing updating. Sorry, what does the layout tree snapshot view have to do with layout analyzer counters? These layouts are FrameView::performLayout trace events, not snapshots. Is there a system like ObjectSnapshot where I need to register a class to parse trace event args?
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:82: return {'count': 0, 'totalMs': 0, 'avgMs': NaN}; On 2015/06/03 20:20:46, benjhayden wrote: > On 2015/05/29 21:41:11, dsinclair wrote: > > On 2015/05/29 21:19:55, benjhayden wrote: > > > On 2015/05/29 20:43:10, dsinclair wrote: > > > > Why NaN and not 0? > > > > > > > > > http://math.stackexchange.com/questions/909395/what-is-the-average-of-no-numbers > > > > Let's just use 0. There is less chance of us causing an exception by trying to > > do something with it. > > How could NaN cause an exception? Isn't the point of NaN to avoid exceptions? Well, the exception can also be seeing 'NaN' in the UI which just looks like something is broken. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:99: row.host = layout.args.counters.host; On 2015/06/03 20:20:46, benjhayden wrote: > On 2015/05/29 21:41:11, dsinclair wrote: > > On 2015/05/29 21:19:55, benjhayden wrote: > > > On 2015/05/29 20:43:10, dsinclair wrote: > > > > What does host mean? > > > > > > Like, in general? > > > > > > Yes, what is layout.args.counts.host? Like, http://www.google.com Something > else, what > > does host mean here? I have no idea. > > It looks like you had at least one idea, and it was correct. > I'm still not sure what "host" could possibly mean in this context besides an > internet host, so I don't know what options I need to distinguish among, so I'm > not sure if the comment that I added is sufficient. > Here's a menu of possible meanings for "host" in the entire English language. > Could you point out the possible meanings that might apply in this context > besides the first entry "Host (network)" under Computing, which is the only > entry directly under Computing? > http://en.wikipedia.org/wiki/Host The host frame. The host widget. The host element. The host page. The server host. etc. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:103: row[colAbbrev] = layout.args.counters[colVerbose] || 0; On 2015/06/03 20:20:46, benjhayden wrote: > On 2015/05/29 21:41:12, dsinclair wrote: > > I think this should wait until we figure out the intermediary structure in the > > layout view design doc. We don't want code throughout the system accessing the > > args like this. What if we change the way blink outputs in the future? We > don't > > want to have to update a whole pile of places, better just a single object > > needing updating. > > Sorry, what does the layout tree snapshot view have to do with layout analyzer > counters? > These layouts are FrameView::performLayout trace events, not snapshots. > Is there a system like ObjectSnapshot where I need to register a class to parse > trace event args? Pulling things from .args in the view is wrong. This data should be in the LayoutObject class we defined. The one we're also building snapshots.
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:103: row[colAbbrev] = layout.args.counters[colVerbose] || 0; I think Ben the fact that we have two layout features in flight at once is confusing. I have had a hard time keeping these two patches apart too ... lets focus on layout tree viewer, then come back to the rail layout info patch later?
Sign in to reply to this message.
fyi with https://codereview.appspot.com/245940043 --- i have landed a generic table and a array_of_numbers_span feature. there's more to be done but i think we now are in a position where we can factor this patch as simply placing tabular data onto the args, a small tweak to make generic object view use the generic table view, and then at that point, further refinement can be placed into the base/units system
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:103: row[colAbbrev] = layout.args.counters[colVerbose] || 0; On 2015/06/03 20:26:30, dsinclair wrote: > On 2015/06/03 20:20:46, benjhayden wrote: > > On 2015/05/29 21:41:12, dsinclair wrote: > > > I think this should wait until we figure out the intermediary structure in > the > > > layout view design doc. We don't want code throughout the system accessing > the > > > args like this. What if we change the way blink outputs in the future? We > > don't > > > want to have to update a whole pile of places, better just a single object > > > needing updating. > > > > Sorry, what does the layout tree snapshot view have to do with layout analyzer > > counters? > > These layouts are FrameView::performLayout trace events, not snapshots. > > Is there a system like ObjectSnapshot where I need to register a class to > parse > > trace event args? > > > Pulling things from .args in the view is wrong. This data should be in the > LayoutObject class we defined. The one we're also building snapshots. This data has absolutely nothing at all to do with that LayoutObject class. This data does not have a heap address or a name or htmlId or classList. The LayoutObject class does not have counters for floats or layers or table cells. These counters are not part of a snapshot of an object. These counters do not form a tree. The only commonalities between this data and that LayoutObject is that they are both generated by files in the blink layout directory. Who says that RAILInteractionRecord is a view? https://codereview.appspot.com/233660043/diff/220001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/220001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:21: function summarizeLayouts(layouts) { layoutEvents
Sign in to reply to this message.
https://codereview.appspot.com/233660043/diff/220001/trace_viewer/extras/rail... File trace_viewer/extras/rail/rail_interaction_record.html (right): https://codereview.appspot.com/233660043/diff/220001/trace_viewer/extras/rail... trace_viewer/extras/rail/rail_interaction_record.html:66: RAILInteractionRecord.prototype = { RAILIR.layoutStats getter
Sign in to reply to this message.
PTAL
Sign in to reply to this message.
sanity check: i thought we agreed we're not moving this right now? one thing dan and I have been encouraging charliea to do when he's got free time in between his primary projects is to do code health activities. dan has a big backlog of code health things [tagged with GoodFirstBug in the database] that you could work on that'd help the overall team, are typically easier to clean up, and might be a great way to build breadth of knowledge in the codebase...
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
