Issue 229690043: LayoutTreeSnapshotView
|
Created:
8 years, 11 months ago by benjhayden Modified:
8 years, 7 months ago Reviewers:
nduca CC:
trace-review_googlegroups.com Base URL:
git@github.com:google/trace-viewer@master Visibility:
Public. |
DescriptionLayoutTreeSnapshotView
Render LayoutTree snapshots in a table builder.
Just the name column for now. No highlighting. No snapshot initialization.
Patch Set 1 #
Total comments: 25
Patch Set 2 : table builder #Patch Set 3 : test #Patch Set 4 : extras/blink/blink.html #
Total comments: 5
Patch Set 5 : comments #Patch Set 6 : LayoutObject #Patch Set 7 : LayoutObject #Patch Set 8 : test #Patch Set 9 : LayoutObject, LayoutTree #
Total comments: 26
Patch Set 10 : fixes #Patch Set 11 : comments #
Total comments: 12
Patch Set 12 : chrome/blink/ #Patch Set 13 : absolutely basic LayoutTreeView #
Total comments: 8
Patch Set 14 : comments #Patch Set 15 : LayoutTreeSnapshotView #Patch Set 16 : rebase #Patch Set 17 : rebase #
MessagesTotal messages: 27
Sign in to reply to this message.
This looks pretty cool. We've been moving away from the zebra stripping backgrounds as when you collapse you have to re-do all the stripes and going towards a devtool model where the bg is the same and we highlight the row the mouse is over. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.css (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.css:7: font-size: 1em; Don't add a custom css file, put it into the <style> tag of the element. We've been trying to remove all of these. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:3: Copyright (c) 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:24: var zws = String.fromCharCode(8203); Can you give this a better name? I don't know what zws is. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:26: function render(layoutObject) { I think this should method should be in its own exported class. Separate the render of an individual item from the tree. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:27: var div = document.createElement('div'); The basic template and styling should be in a template element that gets instantiated. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:31: if (this.layoutObject) { Why would we not have a layoutObject? https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:36: div.appendChild(document.createTextNode(' (positioned)')); There are a lot more of these types of annotations, are they just not available? anonymous, relative positioned, etc. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:67: needsStr += ' self'; Build this as an array and use join instead of string +prev. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:92: needsSpan.style.background = (layoutObject.selfNeeds ? '#99f' : Lets drop the background colour, it seemed more distracting then useful in the image.
Sign in to reply to this message.
this should use table_builder
Sign in to reply to this message.
more comments. pretty cool, yo! https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/chrome_co... File trace_viewer/extras/chrome_config.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/chrome_co... trace_viewer/extras/chrome_config.html:35: <link rel="import" href="/extras/layout_tree.html"> blink/blink.html? then blink/layout_tree.html? https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.css (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.css:6: span.collapseIndicator { hopefully if you use table_builder its tree stuff will handle this for you. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:168: return { will need an instantiate test so that we can test that this is working properly
Sign in to reply to this message.
On 2015/05/07 14:03:18, dsinclair wrote: > This looks pretty cool. Yay! > > We've been moving away from the zebra stripping backgrounds as when you collapse > you have to re-do all the stripes and going towards a devtool model where the bg > is the same and we highlight the row the mouse is over. Why do you have to re-do all the stripes? Are you looking at the horizontal stripes or the vertical stripes on the left? The horizontal stripes stand out because that's what we're used to seeing in tables, but it's the vertical stripes on the left that are important here. I think that stripeless works in devtools better than here partly because devtools shows the end tags. The LayoutTree doesn't have end tags, so the vertical stripes on the left serve to group siblings and guide the eye between children nodes and parents. I also haven't implemented the arrow-key cursor, which would help navigation generally but would be slower than just reading the tree. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > File trace_viewer/extras/layout_tree.css (right): > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.css:7: font-size: 1em; > Don't add a custom css file, put it into the <style> tag of the element. We've > been trying to remove all of these. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > File trace_viewer/extras/layout_tree.html (right): > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:3: Copyright (c) 2013 The Chromium Authors. > All rights reserved. > 2015 > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:24: var zws = String.fromCharCode(8203); > Can you give this a better name? I don't know what zws is. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:26: function render(layoutObject) { > I think this should method should be in its own exported class. > > Separate the render of an individual item from the tree. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:27: var div = > document.createElement('div'); > The basic template and styling should be in a template element that gets > instantiated. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:31: if (this.layoutObject) { > Why would we not have a layoutObject? > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:36: > div.appendChild(document.createTextNode(' (positioned)')); > There are a lot more of these types of annotations, are they just not available? > anonymous, relative positioned, etc. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:67: needsStr += ' self'; > Build this as an array and use join instead of string +prev. > > https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... > trace_viewer/extras/layout_tree.html:92: needsSpan.style.background = > (layoutObject.selfNeeds ? '#99f' : > Lets drop the background colour, it seemed more distracting then useful in the > image.
Sign in to reply to this message.
On 2015/05/07 17:39:53, benjhayden wrote: > On 2015/05/07 14:03:18, dsinclair wrote: > > This looks pretty cool. > > Yay! > > > > > We've been moving away from the zebra stripping backgrounds as when you > collapse > > you have to re-do all the stripes and going towards a devtool model where the > bg > > is the same and we highlight the row the mouse is over. > > Why do you have to re-do all the stripes? > Are you looking at the horizontal stripes or the vertical stripes on the left? > The horizontal stripes stand out because that's what we're used to seeing in > tables, but it's the vertical stripes on the left that are important here. > I think that stripeless works in devtools better than here partly because > devtools shows the end tags. The LayoutTree doesn't have end tags, so the > vertical stripes on the left serve to group siblings and guide the eye between > children nodes and parents. > I also haven't implemented the arrow-key cursor, which would help navigation > generally but would be slower than just reading the tree. > > The horizontal stripes are fine, and make it easier to connect the tree up. The vertical stripes are what I take issue with as you have varying amounts of white space between them which looks, at least to me, bad. My initial assumption was that each vertical line was suppost to be a different colour. But, as nduca@ says, use table_builder and then you don't have to deal with this at all.
Sign in to reply to this message.
Updated screenshot https://x20web.corp.google.com/~benjhayden/229690043.png https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/chrome_co... File trace_viewer/extras/chrome_config.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/chrome_co... trace_viewer/extras/chrome_config.html:35: <link rel="import" href="/extras/layout_tree.html"> On 2015/05/07 17:36:29, nduca wrote: > blink/blink.html? then blink/layout_tree.html? Done. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.css (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.css:6: span.collapseIndicator { On 2015/05/07 17:36:29, nduca wrote: > hopefully if you use table_builder its tree stuff will handle this for you. Done. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.css:7: font-size: 1em; On 2015/05/07 14:03:17, dsinclair wrote: > Don't add a custom css file, put it into the <style> tag of the element. We've > been trying to remove all of these. Done. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:3: Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2015/05/07 14:03:17, dsinclair wrote: > 2015 Done. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:24: var zws = String.fromCharCode(8203); On 2015/05/07 14:03:17, dsinclair wrote: > Can you give this a better name? I don't know what zws is. It was zero width space, but it isn't necessary with table builder. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:26: function render(layoutObject) { On 2015/05/07 14:03:17, dsinclair wrote: > I think this should method should be in its own exported class. > > Separate the render of an individual item from the tree. Acknowledged. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:27: var div = document.createElement('div'); On 2015/05/07 14:03:18, dsinclair wrote: > The basic template and styling should be in a template element that gets > instantiated. I'll let you tell me to what degree that's true in the table builder version. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:31: if (this.layoutObject) { On 2015/05/07 14:03:18, dsinclair wrote: > Why would we not have a layoutObject? Obsolete. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:36: div.appendChild(document.createTextNode(' (positioned)')); On 2015/05/07 14:03:17, dsinclair wrote: > There are a lot more of these types of annotations, are they just not available? > anonymous, relative positioned, etc. This is LayoutObject::isOutOfFlowPositioned(). Can you let me know what specific LayoutObject methods you'd like exposed here? https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:67: needsStr += ' self'; On 2015/05/07 14:03:18, dsinclair wrote: > Build this as an array and use join instead of string +prev. Done. https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:92: needsSpan.style.background = (layoutObject.selfNeeds ? '#99f' : On 2015/05/07 14:03:18, dsinclair wrote: > Lets drop the background colour, it seemed more distracting then useful in the > image. Done for the needs bits. But in the new patch set, I added background color to the 'name' first column to indicate added/removed nodes, and x/y/width/height to indicate change from previous values, and Elliot likes it. :-) https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:168: return { On 2015/05/07 17:36:29, nduca wrote: > will need an instantiate test so that we can test that this is working properly Done.
Sign in to reply to this message.
looks really solid! and gorgous, guhh! https://codereview.appspot.com/229690043/diff/80001/trace_viewer/core/analysi... File trace_viewer/core/analysis/table_builder.html (right): https://codereview.appspot.com/229690043/diff/80001/trace_viewer/core/analysi... trace_viewer/core/analysis/table_builder.html:185: get subRows() { i can haz subRowPropertyName maybe? https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... File trace_viewer/extras/blink/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... trace_viewer/extras/blink/layout_tree.html:48: {title: 'name', value: function(row) { lets see if we can make the layer_tree_host custom object stratgy work here such that the colum descriptors are lighter and we have each arg in the layer tree snapshot wrapped in some helper class LayerTreeNode or something that then has the getters/names/etc. That way we can unit test them for corner cases, and leave the test coverage on this class to be visual testing only. https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... trace_viewer/extras/blink/layout_tree.html:150: var table = document.createElement('tracing-analysis-nested-table'); you'll want to this.textContent = '' that way if you set object snapshot twice to the same version of this class, there will be rejoicing instead of sadness. or, construct the table in the decorate function, and then just give it rows here.
Sign in to reply to this message.
https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... File trace_viewer/extras/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/1/trace_viewer/extras/layout_tr... trace_viewer/extras/layout_tree.html:36: div.appendChild(document.createTextNode(' (positioned)')); On 2015/05/07 22:51:56, benjhayden wrote: > On 2015/05/07 14:03:17, dsinclair wrote: > > There are a lot more of these types of annotations, are they just not > available? > > anonymous, relative positioned, etc. > > This is LayoutObject::isOutOfFlowPositioned(). > Can you let me know what specific LayoutObject methods you'd like exposed here? Take a look at LayoutObject::decoratedName. I think we should have the same things here as decoratedName shows as they're known to be useful. https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... File trace_viewer/extras/blink/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... trace_viewer/extras/blink/layout_tree.html:25: if (!row._prev || row._prev[field] == row[field]) nit: === https://codereview.appspot.com/229690043/diff/80001/trace_viewer/extras/blink... trace_viewer/extras/blink/layout_tree.html:37: if (!tree.children) return; nit: move the returns down to their own lines.
Sign in to reply to this message.
The screenshot is updated. Numeric columns are now aligned on the decimal point. Changes in the classList are now highlighted green for additions, red for deletions. The tbody is now optionally scrollable so that the thead is always visible. The less common columns automatically hide when completely empty. The C++ LayoutObject heap address is now visible so you can ctrl+f for addresses between snapshots and discuss them in bugs. window.currentLayoutTree lets you poke at it on the console and use ad-hoc js to compute things like LayoutAnalyzer. As always, LMK if there's a better way to do that or if you don't think it'll be useful. subRowsPropertyName was useless in a world with LayoutTreeSnapshot, so it was removed. I don't suppose you'd let me abbreviate positioned, relative, anonymous, multicolumn, multicolumnSpan? I'm not sure if I'm using LayoutObject/LayoutTreeSnapshot/LayoutTreeInstance correctly. They are tested, though.
Sign in to reply to this message.
eep this patch is too big to review. find the essence and give me a patch that is appropriately sized. think of this as a general rule: one sentence per patch. 1. introduce skeleton viewer for layout dumps 2. add some other feature 3. add some other feature 4. etc
Sign in to reply to this message.
I split the table builder changes out into a separate cl so now this is just LayoutObject and LayoutTree. I could split LayoutTree into a separate CL, but LayoutObject might not make much sense outside the context of LayoutTree, and LayoutTree is relatively short and simple glue code anyway.
Sign in to reply to this message.
https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... File trace_viewer/extras/blink/layout_object.html (right): https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:23: return; nit: return undefined; https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:31: function recurse(row) { This method is really confusing. What does it do? The name recurse doesn't tell me anything. It looks like sometimes it's changing root, and sometimes it's changing row? Can we split it apart into multiple methods that just operate on one of the two things? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:32: // Can't store root in row: something iterates through these objects. What is the something? Why does iteration mean we can't store the root in the row? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:86: } nit: remove braces. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:93: function shortNumber(row, field) { I don't understand what this is doing based off the name. It outputs things as green, but may have side effects on other rows of numbers? We should split this up to update all the numbers during import post-processing if we need them cleaned up. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:98: var fixed = n.toFixed ? n.toFixed(2) : i; Why is this needed? Is n not a number for some reason? Why is it not a number? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:106: if (i.length < fixed.length) { This seems like a lot of work for little benefit. What's wrong with just always using the toFixed version? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:132: span.style.background = '#cfc'; Having the model generate HTML seems wrong to me. The HTML should be done by the view. If this gets processed by something like telemetry it's going to have to undo all the HTML when it goes to display is it not? The models should be output format agnostic. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:215: getTableRowSpan: function(row) { Why do we, potentially, format the rowspan value in green? What is this for? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:231: getClass: function(row) { This doesn't get a class, it's getting an element. This name doesn't give me any idea of what's going on with this method. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:277: // This takes a few ms for large trees, more in debug builds, and there What is this doing that makes it so slow? https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:278: // may be hundreds of LayoutTrees in a snapshot, so put it in a method Have you seen this with hundreds of layout trees? I'd be surprised if we could get that big as we have a hard limit on the size of the trace based on v8 strings. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:279: // that is only called when the snapshot is about to be rendered. What is the method that is only called later?
Sign in to reply to this message.
https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... File trace_viewer/extras/blink/layout_object.html (right): https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:23: return; On 2015/05/20 17:12:32, dsinclair wrote: > nit: return undefined; findRow() evaporated when I made root._rowCache. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:31: function recurse(row) { On 2015/05/20 17:12:31, dsinclair wrote: > This method is really confusing. What does it do? The name recurse doesn't tell > me anything. It looks like sometimes it's changing root, and sometimes it's > changing row? Can we split it apart into multiple methods that just operate on > one of the two things? I split it up and renamed it traverseTree(). https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:32: // Can't store root in row: something iterates through these objects. On 2015/05/20 17:12:32, dsinclair wrote: > What is the something? Why does iteration mean we can't store the root in the > row? See the new comment in LayoutTreeSnapshot.preInitialize(). https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:86: } On 2015/05/20 17:12:32, dsinclair wrote: > nit: remove braces. Done. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:93: function shortNumber(row, field) { On 2015/05/20 17:12:31, dsinclair wrote: > I don't understand what this is doing based off the name. It outputs things as > green, but may have side effects on other rows of numbers? We should split this > up to update all the numbers during import post-processing if we need them > cleaned up. Done. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:98: var fixed = n.toFixed ? n.toFixed(2) : i; On 2015/05/20 17:12:32, dsinclair wrote: > Why is this needed? Is n not a number for some reason? Why is it not a number? Done. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:106: if (i.length < fixed.length) { On 2015/05/20 17:12:31, dsinclair wrote: > This seems like a lot of work for little benefit. What's wrong with just always > using the toFixed version? When scanning down a long column of round numbers, it's nice to not have to check that all of the numbers after the decimal points are zeros. "42" is just nicer to look at than "42.00", especially when there are thousands of rows. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:132: span.style.background = '#cfc'; On 2015/05/20 17:12:31, dsinclair wrote: > Having the model generate HTML seems wrong to me. The HTML should be done by the > view. If this gets processed by something like telemetry it's going to have to > undo all the HTML when it goes to display is it not? > > The models should be output format agnostic. I wasn't thinking of LayoutObject as a model, I was thinking of it as a helper for the view. All of the functions in LayoutObject are only called directly by table builder through LayoutTree's columns. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:215: getTableRowSpan: function(row) { On 2015/05/20 17:12:32, dsinclair wrote: > Why do we, potentially, format the rowspan value in green? What is this for? I'm not sure what you're asking, so I'll answer some of the questions that you might mean. The rowspan cell is green when the element's rowspan value changed from the previous LayoutTree snapshot for the same FrameView. This function is used by LayoutTree's table builder to compute a viewable value for a table cell in the 'rowspan' column for the given row. The rowspan/colspan are reported here because they are reported by LayoutTreeAsText. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:231: getClass: function(row) { On 2015/05/20 17:12:32, dsinclair wrote: > This doesn't get a class, it's getting an element. This name doesn't give me any > idea of what's going on with this method. This function generates the contents of a cell in table builder in the column of css classes. Added a comment at the top of LayoutObject. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:277: // This takes a few ms for large trees, more in debug builds, and there On 2015/05/20 17:12:32, dsinclair wrote: > What is this doing that makes it so slow? Recursing over each layout object. Updating the has-column flags. Finding previous/next versions of each row. The new root._rowCache seems to have sped up finding the _prev pointers. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:278: // may be hundreds of LayoutTrees in a snapshot, so put it in a method On 2015/05/20 17:12:32, dsinclair wrote: > Have you seen this with hundreds of layout trees? I'd be surprised if we could > get that big as we have a hard limit on the size of the trace based on v8 > strings. I have one trace with over a hundred snapshots. https://codereview.appspot.com/229690043/diff/170001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:279: // that is only called when the snapshot is about to be rendered. On 2015/05/20 17:12:32, dsinclair wrote: > What is the method that is only called later? This method, prepare(), was called directly by LayoutTree when it's about to build the table. I moved the functionality back into initialize(), which is called when the model is being imported, now that root._rowCache speeds up the tree traversal.
Sign in to reply to this message.
https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... File trace_viewer/extras/blink/blink.html (right): https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/blink.html:1: <!DOCTYPE html> i suggest we do https://github.com/google/trace-viewer/issues/970 and then integrate this patch with that one
Sign in to reply to this message.
i think we should start with the layout object patch, talk about that in isolation of the rest... there's a lot to discuss here (good thing!) https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... File trace_viewer/extras/blink/layout_object.html (right): https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:135: getAbsX: function(row) { this is awkward, why isn't this getAbsX: function() { return this.args...} https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:136: return shortNumber(row, 'absX'); also why is the text formatting being done in the object? the object should be yielding its data. the view should be doing prettification https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_object.html:313: function LayoutTreeInstance() { snapshot and instance should be separate files prefer one class per file unless you have a really clear reason otherwise https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... File trace_viewer/extras/blink/layout_tree.html (right): https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:19: value: LayoutObject.getName}, indentation here is funky... prefer { field: blah, otherfield: blah }, { field: something, } https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:81: var LayoutTreeSnapshotView = tv.b.ui.define( this file contains LayoutTreeSnapshotView, so why isn't it called layout_tree_snapshot_view.html? https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:93: this.table_.emptyValue = '(Empty LayoutTree)'; any reason this is after the appendChild? https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:103: return !col.filter || snapshot.args._has[col.filter]; why would col.filter be undefined? why are columns dynamic here? how about starting with them fixed and then improve that down the road? https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:105: this.table_.tableRows = [snapshot.args]; i dont think the table rows should be the args, but this is really a comment on how you've chosen to structure layoutObject. Rather, it should be [snapshot] it seems.. https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:117: this.layerPicker_.selection = selection; do we even have a layer picker? i'm confused how this works... https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree.html:119: tv.b.dispatchSimpleEvent(this, 'cc-selection-change'); is this needed? https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... File trace_viewer/extras/blink/layout_tree_test.html (right): https://codereview.appspot.com/229690043/diff/210001/trace_viewer/extras/blin... trace_viewer/extras/blink/layout_tree_test.html:20: var m = new tv.c.TraceModel(g_TestLayoutTreeEvents); can you use base/xhr.html and do an xhr here instead? that way the test data can be layout_tree_simple_test_data.json and this will all be clearer... the layer tree code doesn't do that for legacy reasons
Sign in to reply to this message.
Ok, LayoutObject/LayoutTreeSnapshot/LayoutTreeViewHelper cannot be reviewed out of context, so let's establish a basic context with the simplest LayoutTreeView possible, then take one thing at a time? PTAL?
Sign in to reply to this message.
On 2015/05/26 18:18:15, benjhayden wrote: > Ok, LayoutObject/LayoutTreeSnapshot/LayoutTreeViewHelper cannot be reviewed out > of context, so let's establish a basic context with the simplest LayoutTreeView > possible, then take one thing at a time? > > PTAL? Is this CL in replace of https://codereview.appspot.com/240070043/ or are the dependent? What's the ordering? Can you like the design doc from this CL?
Sign in to reply to this message.
https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... File trace_viewer/extras/chrome/blink/layout_tree_test_data.js (right): https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_test_data.js:58: 'address': 0, I believe these would be output as 0x1234 would they not? https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... File trace_viewer/extras/chrome/blink/layout_tree_view.html (right): https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:15: * @fileoverview Provides the LayoutTree view. Provide the LayoutTreeSnapshotView. https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:27: var LayoutTreeView = tv.b.ui.define( This should be LayoutTreeSnapshotView and be in a layout_tree_snapshot_view file. https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:54: LayoutTreeView, {typeName: 'LayoutTree'}); typeName Should probably be LayoutTreeSnapshot.
Sign in to reply to this message.
On 2015/05/26 18:26:46, dsinclair wrote: > On 2015/05/26 18:18:15, benjhayden wrote: > > Ok, LayoutObject/LayoutTreeSnapshot/LayoutTreeViewHelper cannot be reviewed > out > > of context, so let's establish a basic context with the simplest > LayoutTreeView > > possible, then take one thing at a time? > > > > PTAL? > > Is this CL in replace of https://codereview.appspot.com/240070043/ or are the > dependent? What's the ordering? Can you like the design doc from this CL? Nevermind that CL. Let's start with this one and then take one whole tiny feature at a time so that we don't need to explain everything at once. https://docs.google.com/document/d/19S7VQNve-0leCW-4JUunGS8OLBvv-93s-KXcIjDUC...
Sign in to reply to this message.
On 2015/05/26 18:32:30, benjhayden wrote: > On 2015/05/26 18:26:46, dsinclair wrote: > > On 2015/05/26 18:18:15, benjhayden wrote: > > > Ok, LayoutObject/LayoutTreeSnapshot/LayoutTreeViewHelper cannot be reviewed > > out > > > of context, so let's establish a basic context with the simplest > > LayoutTreeView > > > possible, then take one thing at a time? > > > > > > PTAL? > > > > Is this CL in replace of https://codereview.appspot.com/240070043/ or are the > > dependent? What's the ordering? Can you like the design doc from this CL? > > Nevermind that CL. Let's start with this one and then take one whole tiny > feature at a time so that we don't need to explain everything at once. > > https://docs.google.com/document/d/19S7VQNve-0leCW-4JUunGS8OLBvv-93s-KXcIjDUC... I'd like a flushed out design doc which shows what internal structure we're building and how it works. We can write the CLs in pieces but having the full design up front makes it a lot easier to review.
Sign in to reply to this message.
It was a great suggestion to switch to a doc. I think that was a better strategy than my suggestion of multiple cl's, I'm sorry for the bad guidance! What we're mostly going back and forth about here is classes and responsibilities, than how the code is written. Thats a great topic for design docs, because we can iterate more quickly. One issue: I tried to start reviewing the doc but it isn't commentable... anyways, a few thoughts from what I read: - expanded state should be in the view only, not on the objects. you could either wrap the actual objects in another object that exposes isExpanded, or you could allow the table builder able to get expanded state from a function on the table builder, whose default implementation is to get expanded state from the row object. - we shoudn't be storing the results of initialize in the args. we can delete original data from the args, if you want to save memory. but, we have a new field that is strongly named. children. nodes. something like that. for instance, you'll notice that layer_tree.html pulls from args and then sets up various explicit fields. - the doc doesn't explain which class finds the sibling nodes, or how that gets mapped into the table builder. both would be good. - I'd also like to see in the doc about how the various fields are represented in the object model. Eg how does className work? How does absx and y work?
Sign in to reply to this message.
The design doc should be more flushed out now, and commentable. https://docs.google.com/document/d/19S7VQNve-0leCW-4JUunGS8OLBvv-93s-KXcIjDUC... https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... File trace_viewer/extras/chrome/blink/layout_tree_test_data.js (right): https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_test_data.js:58: 'address': 0, On 2015/05/26 18:32:21, dsinclair wrote: > I believe these would be output as 0x1234 would they not? Done. https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... File trace_viewer/extras/chrome/blink/layout_tree_view.html (right): https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:15: * @fileoverview Provides the LayoutTree view. On 2015/05/26 18:32:21, dsinclair wrote: > Provide the LayoutTreeSnapshotView. Done. https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:27: var LayoutTreeView = tv.b.ui.define( On 2015/05/26 18:32:21, dsinclair wrote: > This should be LayoutTreeSnapshotView and be in a layout_tree_snapshot_view > file. Done. https://codereview.appspot.com/229690043/diff/250001/trace_viewer/extras/chro... trace_viewer/extras/chrome/blink/layout_tree_view.html:54: LayoutTreeView, {typeName: 'LayoutTree'}); On 2015/05/26 18:32:21, dsinclair wrote: > typeName Should probably be LayoutTreeSnapshot. That would require changing blink and would seem to be inconsistent with LayerTreeHostImplSnapshotView's typeName 'cc::LayerTreeHostImpl'.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
