Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1152213007:
Rename LayoutAnalyzer counters to displayable strings (Closed)
|
Created:
5 years, 6 months ago by benjhayden Modified:
5 years, 2 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionRename LayoutAnalyzer counters to displayable strings.
Trace viewer shouldn't need to map from ugly strings to pretty strings. It can
display descriptions in tool tips instead.
If the long strings are used as column headers, then the table would be about
75" wide. If the short strings are used, then the table is about 20" wide.
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : rebase #Messages
Total messages: 29 (12 generated)
nduca@chromium.org changed reviewers: + nduca@chromium.org
lgtm
benjhayden@chromium.org changed reviewers: + jchaffraix@chromium.org - nduca@chromium.org
benjhayden@chromium.org changed reviewers: + nduca@chromium.org
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
What's wrong with mapping on the trace-viewer side? Seems like having more descriptive names in the trace is better, no?
jchaffraix@chromium.org changed reviewers: - dsinclair@chromium.org
lgtm. The description should include how this CL saves a lot of space on trace viewer (~5x) and thus makes trace viewer a lot useful for analyzing the counters. That would justify the change a lot more than the current description.
On 2015/05/29 at 20:25:32, dsinclair wrote: > What's wrong with mapping on the trace-viewer side? Seems like having more descriptive names in the trace is better, no? When we add a counter, we would have to add it to both blink and trace viewer before the counter would be usable. https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail...
On 2015/05/29 at 20:27:53, benjhayden wrote: > On 2015/05/29 at 20:25:32, dsinclair wrote: > > What's wrong with mapping on the trace-viewer side? Seems like having more descriptive names in the trace is better, no? > > When we add a counter, we would have to add it to both blink and trace viewer before the counter would be usable. > > https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... It's usable without adding it to trace-viewer, it just doesn't necessarily look as nice in the table. But, that doesn't stop it from showing up.
On 2015/05/29 at 20:27:53, benjhayden wrote: > On 2015/05/29 at 20:25:32, dsinclair wrote: > > What's wrong with mapping on the trace-viewer side? Seems like having more descriptive names in the trace is better, no? > > When we add a counter, we would have to add it to both blink and trace viewer before the counter would be usable. > > https://codereview.appspot.com/233660043/diff/140001/trace_viewer/extras/rail... That for loop just needs an else to add the string if not found in the shortened list.
> > > What's wrong with mapping on the trace-viewer side? Seems like having more > descriptive names in the trace is better, no? Side bar: one core philosophical attribute of trace-viewer that I think serves us well is that if you want to see new data in the ui, you typically just have to patch c++. you only have to patch the ui when you want improved ui. So, for instance, you can add args to a trace event and it just shows up in the ui. This works well for us because we're not building devtools.... we're building a tool for power users. I totally agree that in broad strokes, descriptive is good. But, visually its very at odds with the tabular constraints being used in this ui. I don't really see any easy way to have reasonably sized columns while also having descriptiveness and ~15 columns... I think that means tooltips, which we could add, but its at this point that I think its worth coming back to the sidebar: if we go engineers some solution that lets us have shorthands for columns but also longhands, we're either adding ui code and breaking that sidebar point of "just change c++" by making people add js tweaks in their code just to see the column, or we're adding more complexity on the c++ side to allow data to have tooltips and names and... its just not easy to solve this, I guess, and still preserve the "just x" properties that I think make trace viewr good.
(therefore, i like the idea of just having the trace dumps have the shorthand in them, if my position wasn't clear)
On 2015/05/30 at 03:05:28, nduca wrote: > > > > What's wrong with mapping on the trace-viewer side? Seems like having more > > descriptive names in the trace is better, no? > > Side bar: one core philosophical attribute of trace-viewer that I think serves us well is that if you want to see new data in the ui, you typically just have to patch c++. you only have to patch the ui when you want improved ui. So, for instance, you can add args to a trace event and it just shows up in the ui. This works well for us because we're not building devtools.... we're building a tool for power users. > > I totally agree that in broad strokes, descriptive is good. But, visually its very at odds with the tabular constraints being used in this ui. I don't really see any easy way to have reasonably sized columns while also having descriptiveness and ~15 columns... I think that means tooltips, which we could add, but its at this point that I think its worth coming back to the sidebar: if we go engineers some solution that lets us have shorthands for columns but also longhands, we're either adding ui code and breaking that sidebar point of "just change c++" by making people add js tweaks in their code just to see the column, or we're adding more complexity on the c++ side to allow data to have tooltips and names and... its just not easy to solve this, I guess, and still preserve the "just x" properties that I think make trace viewr good. So, I disagree with your sidebar, heh. My preference is to have the C++ output as specific a piece of information we can have. So, we output millisecond numbers, we output the full layer tree, etc. We output very specific things and then patch them up on the front end to be more readable (We pad numbers, round timestamps, etc). This is also why I ask for things like when memory infa was adding their dumping I asked for longer names for each of the dumped things. We actually ended up changing back to short names a few weeks later, but only after we found out the extra cost in JSON size wasn't worth it. Looking at this CL as an example, seeing something like "width changed" in the JSON doesn't tell me as much as LayoutBlockWidthChanged. Or "simple chars" vs "CharactersInLayoutObjectsThatAreTextAndCanUseTheSimpleFontCodePath". Sure, the second is _really_ long, but it has a lot of context bundled up in that name. That context is good, and is really good in a year when we're looking at a JSON file trying to figure out what things mean. The other place I see the explicit names as useful is for node js processing. If we write a node-viewer script that can pull data out of the model having it spit out 8 simple chars vs 8 CharactersInLayoutObjectsThatAreTextAndCanUseTheSimpleFontCodePath The second has a lot of context that, while we can show it in the viewer, is lost in the command line. I don't see the burden as that high such that we add a new layout object counter, and for the week or so until we roll deps it has a wacky long name. The number of people that run ToT trace-viewer on a custom built chrome with ToT blink is small. (Given it currently crashes the renderer and no-one has mentioned it, very very small).
On 2015/05/30 at 03:05:48, nduca wrote: > (therefore, i like the idea of just having the trace dumps have the shorthand in them, if my position wasn't clear) I won't block the patch, I explicitly didn't add myself as a reviewer when I commented. Just wanted to make the case that, in my opinion, the long names are better.
The CQ bit was checked by benjhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152213007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64632)
The CQ bit was checked by benjhayden@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152213007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64876)
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, jchaffraix@chromium.org Link to the patchset: https://codereview.chromium.org/1152213007/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1152213007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...) |
