Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996303002:
[Tracing] Introduce Frame Data Side Panel (Closed)
Description[Tracing] Introduce Frame Data Side Panel
This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer.
The UI is a side panel that contains a table, where each row represents
either a frame context or the top level context of a renderer process.
When selecting a row, all slices attributed to the row's context and the
context itself will get selected, so that their aggregated information
will become available in the bottom panel.
Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk
This patch depends on the class hierarchy of blame context classes in
Trace Viewer, which are implemented in another patch [2].
[1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8APtI/edit
[2] https://codereview.chromium.org/2016213002
BUG=catapult:#2260
TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5ede943f9a5e98cb11a586914f63a218a9a1b8ec
Patch Set 1 #Patch Set 2 : Add some TODOs #
Total comments: 20
Patch Set 3 : Use blame context base classes #Patch Set 4 : Respond to ROI; Merge Frame and FTN #
Total comments: 44
Patch Set 5 : Move the side panel to extras; Replace type-based if by polymorphism #Patch Set 6 : UTC201605270344 #Patch Set 7 : UTC201605270346 #Patch Set 8 : Test case added #
Total comments: 24
Patch Set 9 : Address review comments; Move BlameContext.attributedEvents computation logic in #
Total comments: 7
Patch Set 10 : Address review comment #
Total comments: 12
Patch Set 11 : Add BlameContext type check; Rewrite unit test #Patch Set 12 : Follow-up revision: use type-based conditionals and chrome helpers in Row constructor #
Total comments: 6
Patch Set 13 : #
Total comments: 2
Patch Set 14 : #
Messages
Total messages: 40 (10 generated)
benjhayden@chromium.org changed reviewers: + benjhayden@chromium.org
Thanks! Here are a few ideas. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/model/o... File tracing/tracing/model/object_instance.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/model/o... tracing/tracing/model/object_instance.html:191: get isBlameContext() { Let's use a base class and instanceof instead of hard-coding these types. :-) https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: // TODO(xiaochengh): Use a grouping table instead. A table is too verbose Can you use FrameTreeNode parent pointers to do the grouping manually here? https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:64: // TODO(xiaochengh): Find a saner approach. This belongs in tracing/base/utils.html, though it would be ideal if it could be implemented without using document so that it could be used in headless contexts. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:72: GetContextUrl_: function(context) { Instead of searching, FrameTreeNode should set its RenderFrame.frameTreeNodeSnapshot = this so that this side panel can say context.frameTreeNodeSnapshot.url. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:97: columns.push({ I was imagining columns = [URL, Browser CPU, Renderer CPU, Total CPU], so we could hide the FrameTreeNode/RenderFrame ids as implementation details. The point of this side panel is to display Frames, not contexts. The FrameTreeNode vs RenderFrame distinction is an implementation detail. Single-process browsers such as Webview would only have a single CPU column, of course. Eventually, we can add Memory and Network columns. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:129: eventSetForRow_: function(rowProcess, rowContext) { This should be implemented by the BlameContexts, maybe a base class shared by FrameTreeNode and RenderFrame. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:148: // TODO(xiaochengh): Make this function faster! It's currently doing a Does it cause a noticeable delay on a fairly large trace? If so, please fix it before committing. If not, don't worry about it. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:198: set rangeOfInterest(rangeOfInterest) { Hm, I think it would be super useful if this side panel responded to this. It's common for us to select a time range based on UserExpectations, so it would be useful to see cpu usage by frame while loading. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:206: set selection(_) { This *could* be applicable? Might users want to select some FrameTreeNodes/RenderFrames? Or some events on some threads/processes?
Thanks for the review for such a messy patch! https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/model/o... File tracing/tracing/model/object_instance.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/model/o... tracing/tracing/model/object_instance.html:191: get isBlameContext() { On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > Let's use a base class and instanceof instead of hard-coding these types. :-) Yeah that seems to be a lot better. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: // TODO(xiaochengh): Use a grouping table instead. A table is too verbose On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > Can you use FrameTreeNode parent pointers to do the grouping manually here? I'll do that after I understand how grouping table works... https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:64: // TODO(xiaochengh): Find a saner approach. On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > This belongs in tracing/base/utils.html, though it would be ideal if it could be implemented without using document so that it could be used in headless contexts. Just wondering if there is already any implementation in the code base. "Getting origin from URL string" without creating any garbage (like the <a> in this case) sounds like a rather fundamental wheel that should have been invented. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:72: GetContextUrl_: function(context) { On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > Instead of searching, FrameTreeNode should set its RenderFrame.frameTreeNodeSnapshot = this so that this side panel can say context.frameTreeNodeSnapshot.url. Sounds good. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:97: columns.push({ On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > I was imagining columns = [URL, Browser CPU, Renderer CPU, Total CPU], so we could hide the FrameTreeNode/RenderFrame ids as implementation details. > The point of this side panel is to display Frames, not contexts. The FrameTreeNode vs RenderFrame distinction is an implementation detail. > Single-process browsers such as Webview would only have a single CPU column, of course. > Eventually, we can add Memory and Network columns. Sounds good. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:129: eventSetForRow_: function(rowProcess, rowContext) { On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > This should be implemented by the BlameContexts, maybe a base class shared by FrameTreeNode and RenderFrame. Yeah it should be. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:148: // TODO(xiaochengh): Make this function faster! It's currently doing a On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > Does it cause a noticeable delay on a fairly large trace? > If so, please fix it before committing. If not, don't worry about it. Yes, it takes ~1 min on a sample trace file that I'm currently using. https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:198: set rangeOfInterest(rangeOfInterest) { On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > Hm, I think it would be super useful if this side panel responded to this. > It's common for us to select a time range based on UserExpectations, so it would be useful to see cpu usage by frame while loading. Yeah that should definitely be the way how it works! https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:206: set selection(_) { On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > This *could* be applicable? > Might users want to select some FrameTreeNodes/RenderFrames? Or some events on some threads/processes? I don't have a clear idea what kind of response it should be. Besides, is there any conflict with selectSlicesOfContext_?
https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: // TODO(xiaochengh): Use a grouping table instead. A table is too verbose On 2016/05/23 at 03:17:14, Xiaocheng wrote: > On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > > Can you use FrameTreeNode parent pointers to do the grouping manually here? > > I'll do that after I understand how grouping table works... Sorry, I'm trying to say that I don't think that grouping table is the right tool here. Grouping table is for when you have a collection of things with no inherent correct structure but with several tags, and you want to allow users to dynamically re-group them by different keys. In this case, there is a single correct pre-defined structure that is explicitly defined by the FrameTreeNode.parent references. The base table can display groups using subRows. You can translate the FrameTreeNode.parent references into subRows directly -- this would actually be easier than configuring grouping table to do it magically for you.
https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:64: // TODO(xiaochengh): Find a saner approach. On 2016/05/23 at 03:17:14, Xiaocheng wrote: > On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > > This belongs in tracing/base/utils.html, though it would be ideal if it could be implemented without using document so that it could be used in headless contexts. > > Just wondering if there is already any implementation in the code base. "Getting origin from URL string" without creating any garbage (like the <a> in this case) sounds like a rather fundamental wheel that should have been invented. Catapult uses vinn, which is an unusual environment that has neither node's url module nor browsers' document. Can this CL just display the full URLs? The side panel container should automatically expand to accommodate them, and you can use CSS to limit the size of each cell. Maybe another CL can add a util function?
On 2016/05/23 at 17:18:41, benjhayden wrote: > https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... > File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): > > https://codereview.chromium.org/1996303002/diff/20001/tracing/tracing/ui/side... > tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: // TODO(xiaochengh): Use a grouping table instead. A table is too verbose > On 2016/05/23 at 03:17:14, Xiaocheng wrote: > > On 2016/05/20 at 16:16:05, benjhayden_chromium wrote: > > > Can you use FrameTreeNode parent pointers to do the grouping manually here? > > > > I'll do that after I understand how grouping table works... > > Sorry, I'm trying to say that I don't think that grouping table is the right tool here. > Grouping table is for when you have a collection of things with no inherent correct structure but with several tags, and you want to allow users to dynamically re-group them by different keys. > In this case, there is a single correct pre-defined structure that is explicitly defined by the FrameTreeNode.parent references. > The base table can display groups using subRows. You can translate the FrameTreeNode.parent references into subRows directly -- this would actually be easier than configuring grouping table to do it magically for you. Row grouping should be a very useful feature, but it seems not necessary if we only aim at a minimum usable and landable version in this CL. Maybe we can leave it to a follow-up CL after this one gets landed. I haven't fixed my mind on what to be the grouping condition. Group by either frame tree topology or site seems natural, and we may even think of something else. It seems to be an extensional part that can be decided later.
xiaochengh@chromium.org changed reviewers: + nduca@chromium.org, petrcermak@chromium.org, skyostil@chromium.org
This is how it looks like currently: https://drive.google.com/a/google.com/file/d/0B7U-s0-cBvuQUHRVM3hSZjFfYlE/vie... Hope you are interested, so that we can discuss what should be included in this patch. Any comment is appreciated!
Looks good to me -- I think this is a great starting point. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... File tracing/tracing/extras/chrome/blame_context/frame.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... tracing/tracing/extras/chrome/blame_context/frame.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. nit: no "(c)" needed anymore (here and elsewhere). https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:179: object => object instanceof tr.e.chrome.BlameContextInstance)) nit: add braces around the multi-line if.
Here's a couple of comments. Please: * Add a patch description. I would definitely explain what classes are added and how they are related. Something like a UML diagram (ASCII art) would be more than welcome. * Add tests for the UI component you are adding. At least instantiate it, fill it with some sample data and check that the table in it has the right contents. Thanks, Petr https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... File tracing/tracing/extras/chrome/blame_context/toplevel.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... tracing/tracing/extras/chrome/blame_context/toplevel.html:23: TopLevelSnapshot.prototype = { nit: The name of this file should probably be top_level.html (to match the camel case). https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... File tracing/tracing/model/model.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... tracing/tracing/model/model.html:562: if (snapshot instanceof tr.e.chrome.BlameContextSnapshot) { Maybe you could use `snapshot.eventSet !== undefined`? I would really like to avoid accessing extras from the model. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... tracing/tracing/model/model.html:563: if (patchup.item.duration) Just checking: Is it intended that this is false even if the duration is defined but zero? https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:3: Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:8: <link rel="import" href="/tracing/base/statistics.html"> you shouldn't need this https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:9: <link rel="import" href="/tracing/ui/base/grouping_table.html"> I don't think you're using grouping table. You should import table.html instead. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:12: <link rel="import" href="/tracing/value/numeric.html"> you shouldn't need this https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:25: * TODO(xiaochengh): It's ugly when the width of the panel is enlarged. This is the CSS selector you're probably looking for: table td:nth-child(4) https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:30: min-height: 0px; Do you really need this? https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: (function() { Please use tr.exportTo instead. Sample usage: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:67: this.contexts.forEach(context => context.eventSet.filter(event => Please make this code more readable by adding extra line breaks and use addEventSet: this.contexts.forEach( context => ans.addEventSet(context.eventSet.filter( event => rangeOfInterest.intersectsExplicitRangeInclusive( event.start, event.end)))); this.eventSet = ans; https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:85: var row = this.$.table.selectedTableRow; nit: I don't think there's any value in naming this. Why not just do the following: this.selectSlicesOfContext_(this.$.table.selectedTableRow); https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:93: selectSlicesOfContext_: function(row) { I think it would make more sense to pass the eventSet here (rather than the row). https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:100: var columns = []; The number of columns is fixed, so please do the following (no need to push): return [ { title: 'Renderer', ... }, ... ]; https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:102: title: 'Renderer', nit: this should be indented only 2 spaces (-2) https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:114: row.time, { nit: I would put this on the previous line https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:129: frameDataTableRows_: function() { The name of a method should start with a verb (e.g. "createFrameDataTableRows_" or "buildFrameDataTableRows_") https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:135: this.model_.processes[pid].objects.getAllObjectInstances().filter( By calling getAllObjectInstances and filter, you are unnecessarily creating temporary arrays (wasting memory). Please do the following instead: this.model_.process[pid].objects.iterObjectInstances(function(objectInstance) { if (!(objectInstance instanceof ...)) return; ... }, this); https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:136: instance => instance instanceof tr.e.chrome.BlameContextInstance You really shouldn't access extras (tr.e.*) from generic code. Please try to find a way to avoid this (probably polymorphism as you suggested in your TODO). https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:178: if (m.processes[pid].objects.getAllObjectInstances().some( Again, please use iterObjectInstances to avoid wasting memory. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:216: return 'Frame Data'; nit: invalid indentation (should be 2 spaces)
Description was changed from ========== [WIP] Frame Data Side Panel BUG=catapult:#2260 ========== to ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in trace viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... BUG=catapult:#2260 TEST=to be added... ==========
Sorry I should have made it clear that the patch is heavily incomplete and the discussion is mostly on designing the appearance of the side panel. Anyway, thanks for the review, and I really appreciate your effort on the detailed comments, given how messy my code is. Patch 5 has gone through some major revision. The most notable ones are: 1. frame_data_side_panel.html is moved to extras/, due to its heavy dependence on BlameContext and the subclasses that are also under extras/ 2. Type-based conditioning is replaced by polymorphism: - A callback |ObjectSnapshot.appliedObjectRefPatchup| is added to replace the type check in model - A bunch of methods are added to BlameContext to help construction of table rows The patch should be in a much better shape now, though I still need to add docs and test cases... https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... File tracing/tracing/extras/chrome/blame_context/frame.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... tracing/tracing/extras/chrome/blame_context/frame.html:3: Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/05/24 at 09:49:57, Sami wrote: > nit: no "(c)" needed anymore (here and elsewhere). Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... File tracing/tracing/extras/chrome/blame_context/toplevel.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/extras/... tracing/tracing/extras/chrome/blame_context/toplevel.html:23: TopLevelSnapshot.prototype = { On 2016/05/24 at 10:54:10, petrcermak wrote: > nit: The name of this file should probably be top_level.html (to match the camel case). Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... File tracing/tracing/model/model.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... tracing/tracing/model/model.html:562: if (snapshot instanceof tr.e.chrome.BlameContextSnapshot) { On 2016/05/24 at 10:54:10, petrcermak wrote: > Maybe you could use `snapshot.eventSet !== undefined`? I would really like to avoid accessing extras from the model. This part is rewritten by adding a callback |ObjectSnapshot.appliedObjectRefPatchup| and overriding it in BlameContext. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/model/m... tracing/tracing/model/model.html:563: if (patchup.item.duration) On 2016/05/24 at 10:54:10, petrcermak wrote: > Just checking: Is it intended that this is false even if the duration is defined but zero? Good catch. The intention is to add only slices, so it should be |patchup.item instanceof tr.model.Slice|. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... File tracing/tracing/ui/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:3: Copyright 2015 The Chromium Authors. All rights reserved. On 2016/05/24 at 10:54:11, petrcermak wrote: > 2016 Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:8: <link rel="import" href="/tracing/base/statistics.html"> On 2016/05/24 at 10:54:11, petrcermak wrote: > you shouldn't need this Done. I just copied this part from file_size_stats_side_panel.html and forgot to clean it up... https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:25: * TODO(xiaochengh): It's ugly when the width of the panel is enlarged. On 2016/05/24 at 10:54:10, petrcermak wrote: > This is the CSS selector you're probably looking for: > > table td:nth-child(4) Doesn't work :( The real content of tr-ui-b-table is inside a shadow tree, whose style is not affected by whatever we write here. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:30: min-height: 0px; On 2016/05/24 at 10:54:11, petrcermak wrote: > Do you really need this? Removed. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:43: (function() { On 2016/05/24 at 10:54:11, petrcermak wrote: > Please use tr.exportTo instead. Sample usage: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... The file is now moved to ui/extras/side_panel/ and is following the style of the other files there. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:67: this.contexts.forEach(context => context.eventSet.filter(event => On 2016/05/24 at 10:54:11, petrcermak wrote: > Please make this code more readable by adding extra line breaks and use addEventSet: > > this.contexts.forEach( > context => ans.addEventSet(context.eventSet.filter( > event => rangeOfInterest.intersectsExplicitRangeInclusive( > event.start, event.end)))); > this.eventSet = ans; This part has been rewritten. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:85: var row = this.$.table.selectedTableRow; On 2016/05/24 at 10:54:10, petrcermak wrote: > nit: I don't think there's any value in naming this. Why not just do the following: > > this.selectSlicesOfContext_(this.$.table.selectedTableRow); Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:93: selectSlicesOfContext_: function(row) { On 2016/05/24 at 10:54:10, petrcermak wrote: > I think it would make more sense to pass the eventSet here (rather than the row). Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:100: var columns = []; On 2016/05/24 at 10:54:11, petrcermak wrote: > The number of columns is fixed, so please do the following (no need to push): > > return [ > { > title: 'Renderer', > ... > }, > ... > ]; Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:102: title: 'Renderer', On 2016/05/24 at 10:54:11, petrcermak wrote: > nit: this should be indented only 2 spaces (-2) Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:114: row.time, { On 2016/05/24 at 10:54:11, petrcermak wrote: > nit: I would put this on the previous line Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:129: frameDataTableRows_: function() { On 2016/05/24 at 10:54:11, petrcermak wrote: > The name of a method should start with a verb (e.g. "createFrameDataTableRows_" or "buildFrameDataTableRows_") Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:135: this.model_.processes[pid].objects.getAllObjectInstances().filter( On 2016/05/24 at 10:54:10, petrcermak wrote: > By calling getAllObjectInstances and filter, you are unnecessarily creating temporary arrays (wasting memory). Please do the following instead: > > this.model_.process[pid].objects.iterObjectInstances(function(objectInstance) { > if (!(objectInstance instanceof ...)) > return; > ... > }, this); Done. Thanks for the advice. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:136: instance => instance instanceof tr.e.chrome.BlameContextInstance On 2016/05/24 at 10:54:11, petrcermak wrote: > You really shouldn't access extras (tr.e.*) from generic code. Please try to find a way to avoid this (probably polymorphism as you suggested in your TODO). Done with polymorphism. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:178: if (m.processes[pid].objects.getAllObjectInstances().some( On 2016/05/24 at 10:54:11, petrcermak wrote: > Again, please use iterObjectInstances to avoid wasting memory. Done. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:179: object => object instanceof tr.e.chrome.BlameContextInstance)) On 2016/05/24 at 09:49:57, Sami wrote: > nit: add braces around the multi-line if. No need now. https://codereview.chromium.org/1996303002/diff/60001/tracing/tracing/ui/side... tracing/tracing/ui/side_panel/frame_data_side_panel.html:216: return 'Frame Data'; On 2016/05/24 at 10:54:11, petrcermak wrote: > nit: invalid indentation (should be 2 spaces) Done.
Description was changed from ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in trace viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... BUG=catapult:#2260 TEST=to be added... ========== to ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in trace viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. This patch depends on the class hierarchy of blame context classes in trace viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ==========
PTAL at patch 8, which should be a landable patch now. Some (seemingly) extensional features are left open as TODOs in the future. I still don't know how to set the width of a table column, though... Adding a td selector in <style> doesn't work, as it's block by the shadow tree boundary in the <tr-ui-b-table>.
Looks good overall. I've left a few more inline comments. I don't think you need the "TEST=..." line in the patch description. Thanks, Petr https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:67: }.bind(panel)(); As far as I'm aware, this is unnecessarily complicated: function getRowsForTest(panel) { return panel.$.table.tableRows; } To be honest, I would instead just inline the function everywhere. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:80: model.joinRefs(); I think tr.c.TestUtils.newModel(...) would take care of finalizing the model for you. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:84: assert.equal(getRowsForTest(panel).length, 3); assert.lengthOf https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:101: model.joinRefs(); ditto TestUtils.newModel https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:134: model.updateBounds(); ditto https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:150: function selectRowForTest(panel, row) { ditto: 1. no need for the wrapping in a function and 2. I'd inline this https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:174: model.updateBounds(); ditto TestUtils.newModel https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:182: new tr.model.EventSet([topLevel, slice]))); this should be indented 4 spaces (+2) https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:22: * TODO(xiaochengh): It's ugly when the width of the panel is enlarged. Sorry for the previous incorrect suggestion. tr.ui.b.Table supports custom column width via the 'width' property of columns. Example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... You can space the rest of the columns equally using percent. Example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:41: (function() { Please use tr.exportTo instead (example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a...) https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:57: this.renderer = undefined; perhaps this should be rendererPid? https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:141: for (var pid in this.model_.processes) { consider using tr.b.iterItems: tr.b.iterItems(this.model_.processes, function(pid, process) { ... }, this);
Thanks for the review! The panel width issue is fixed, though in a different way. The model initialization part in the test cases have been rewritten using tr.c.TestUtils.newModel... Another modification is that, the computation of events attributed to each context is moved from blame_context.html (in the depending patch) to createFrameDataTableRows_. The reason is that we don't want to affect the importing cost too much when not interested in this side panel. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:67: }.bind(panel)(); On 2016/05/27 at 14:11:02, petrcermak wrote: > As far as I'm aware, this is unnecessarily complicated: > > function getRowsForTest(panel) { > return panel.$.table.tableRows; > } > > To be honest, I would instead just inline the function everywhere. Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:80: model.joinRefs(); On 2016/05/27 at 14:11:02, petrcermak wrote: > I think tr.c.TestUtils.newModel(...) would take care of finalizing the model for you. Done. Thanks for introducing that to me, which really helps a lot. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:84: assert.equal(getRowsForTest(panel).length, 3); On 2016/05/27 at 14:11:01, petrcermak wrote: > assert.lengthOf Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:101: model.joinRefs(); On 2016/05/27 at 14:11:01, petrcermak wrote: > ditto TestUtils.newModel Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:134: model.updateBounds(); On 2016/05/27 at 14:11:02, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:150: function selectRowForTest(panel, row) { On 2016/05/27 at 14:11:02, petrcermak wrote: > ditto: 1. no need for the wrapping in a function and 2. I'd inline this Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:174: model.updateBounds(); On 2016/05/27 at 14:11:01, petrcermak wrote: > ditto TestUtils.newModel Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:182: new tr.model.EventSet([topLevel, slice]))); On 2016/05/27 at 14:11:01, petrcermak wrote: > this should be indented 4 spaces (+2) Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:22: * TODO(xiaochengh): It's ugly when the width of the panel is enlarged. On 2016/05/27 at 14:11:02, petrcermak wrote: > Sorry for the previous incorrect suggestion. tr.ui.b.Table supports custom column width via the 'width' property of columns. Example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... > > You can space the rest of the columns equally using percent. Example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a... Sorry, I actually didn't have a clear idea about the behavior. It's not about fixing a column width, but: 1. The panel width has a fixed initial value, and can be adjusted 2. The table width and height should be automatically adapted to those of the panel 3. The invisible part of the table should be accessible via scroll bars It's already done in the latest patch, by simply adding a 'width' to the style of :host. Sorry for wasting your efforts on finding those materials for me. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:41: (function() { On 2016/05/27 at 14:11:02, petrcermak wrote: > Please use tr.exportTo instead (example: https://github.com/catapult-project/catapult/blob/master/tracing/tracing/ui/a...) Done. https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:57: this.renderer = undefined; On 2016/05/27 at 14:11:02, petrcermak wrote: > perhaps this should be rendererPid? rendererPid is removed from the BlameContext base class patch: https://codereview.chromium.org/2016213002 It looks a little bit weird to have both pid and rendererPid methods there. Doesn't seem to be a big issue, anyway. (You are also welcomed if you are interested to review that CL) https://codereview.chromium.org/1996303002/diff/140001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:141: for (var pid in this.model_.processes) { On 2016/05/27 at 14:11:02, petrcermak wrote: > consider using tr.b.iterItems: > > tr.b.iterItems(this.model_.processes, function(pid, process) { > ... > }, this); Done.
LGTM with a few more comments and please s/trace viewer/Trace Viewer/g in the patch description. Thanks for your patience! Petr https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:219: nit: remove blank line. https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:105: unit: tr.v.Unit.byName.timeStampInMs, nit: we generally indent 2 spaces inside object literals, so I'd change these lines to: value: row => tr.v.ui.createScalarSpan(row.time, { unit: ... ownerDocument: ... }), cmp: ... https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:114: cmp: (a, b) => (a.url || '').localeCompare(b.url) |''.localeCompared(undefined)| returns -1 while |''.localeCompared('')| returns 0. Hence, |undefined| is "smaller" than itself according to this method. You should change it to: (a.url || '').localeCompare(b.url || '')
Description was changed from ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in trace viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. This patch depends on the class hierarchy of blame context classes in trace viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ========== to ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ==========
Thanks for your review, which really helped improving this patch by a lot! All comments are addressed, and the incorrect file name of the unit test is also fixed. I'll commit after the depending patch gets landed. https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:1: <!DOCTYPE html> Incorrect file name. https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/fame_data_side_panel_test.html:219: On 2016/05/31 at 09:57:24, petrcermak wrote: > nit: remove blank line. Done. https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:105: unit: tr.v.Unit.byName.timeStampInMs, On 2016/05/31 at 09:57:24, petrcermak wrote: > nit: we generally indent 2 spaces inside object literals, so I'd change these lines to: > > value: row => tr.v.ui.createScalarSpan(row.time, { > unit: ... > ownerDocument: ... > }), > cmp: ... Done. https://codereview.chromium.org/1996303002/diff/160001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:114: cmp: (a, b) => (a.url || '').localeCompare(b.url) On 2016/05/31 at 09:57:24, petrcermak wrote: > |''.localeCompared(undefined)| returns -1 while |''.localeCompared('')| returns 0. Hence, |undefined| is "smaller" than itself according to this method. You should change it to: > > (a.url || '').localeCompare(b.url || '') Done.
Still LGTM :-) Petr
This is looking pretty good, thanks! Would you mind adding a link to a screenshot to the CL description? I see it in one of the old comments, but it'd be nice to have in the description. I'd like to make sure that Sami reviews this before it's submitted. Should we also wait for Nat to review? https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:142: // TODO(xiaochengh): Is it possible for AsyncSlices to be attributed? Can you file a bug, add chiniforooshan to it, and link to it here? https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:176: if (instance.blameContextType) Should this be instanceof BlameContext instead? https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html:21: function topLevelEvent(pid, id) { Sorry, maybe I missed something, but why are we using newModelWithEvents instead of newModel with newSliceEx? newModelWithEvents seems to be aimed at facilitating testing lower-level importers, whereas newSliceEx is aimed at facilitating higher-level features like this side panel. It doesn't seem appropriate to test the ContextProcessor here.
Description was changed from ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ========== to ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ==========
Thanks for your review, Ben. This patch won't get landed before the depending patch, so we might have a lot of time to wait for other reviewers. https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:142: // TODO(xiaochengh): Is it possible for AsyncSlices to be attributed? On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > Can you file a bug, add chiniforooshan to it, and link to it here? OK. https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:176: if (instance.blameContextType) On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > Should this be instanceof BlameContext instead? A question that I forgot to ask: is it OK for a side panel in tr.ui.e.s to depend on classes in tr.e.chrome? For this particular case it seems to be yes, since frame_data_side_panel.html is included in chrome_config. Am I correct? https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html:21: function topLevelEvent(pid, id) { On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > Sorry, maybe I missed something, but why are we using newModelWithEvents instead of newModel with newSliceEx? > newModelWithEvents seems to be aimed at facilitating testing lower-level importers, whereas newSliceEx is aimed at facilitating higher-level features like this side panel. It doesn't seem appropriate to test the ContextProcessor here. This side panel requires the model to be complete and finalized (join refs and set boundary), which seems to be a perfect task for newModelWithEvents. Or we can do it manually like what is done in Patch 8. A unit test has to rely on the correctness of something else anyways. I think we can just assume ContextProcessor is correct here.
Don't block on me, I'm swamped.
lgtm, seems like a good base to build on. https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html:65: ans.args.snapshot.RenderFrame = { Make sure you update this to whatever naming convention the C++ code settles on.
https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:176: if (instance.blameContextType) On 2016/06/01 at 01:54:23, Xiaocheng wrote: > On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > > Should this be instanceof BlameContext instead? > > A question that I forgot to ask: is it OK for a side panel in tr.ui.e.s to depend on classes in tr.e.chrome? > > For this particular case it seems to be yes, since frame_data_side_panel.html is included in chrome_config. Am I correct? Yes.
Updates in Patch 11: Do type check of BlameContextInstance instead of checking the |blameContextType| getter. The unit test has been rewritten using TestUtils.newSnapshot(), TestUtils.newSliceEx() and TestUtils.newModel(callback). The dependency on ContextProcessor is removed. https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:127: if (!objectInstance.blameContextType) Also changed this line to if (!objectInstance instanceof tr.e.chrome.BlameContextInstance) https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:176: if (instance.blameContextType) On 2016/06/01 at 22:41:48, benjhayden_chromium wrote: > On 2016/06/01 at 01:54:23, Xiaocheng wrote: > > On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > > > Should this be instanceof BlameContext instead? > > > > A question that I forgot to ask: is it OK for a side panel in tr.ui.e.s to depend on classes in tr.e.chrome? > > > > For this particular case it seems to be yes, since frame_data_side_panel.html is included in chrome_config. Am I correct? > > Yes. OK, done. https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel_test.html:65: ans.args.snapshot.RenderFrame = { On 2016/06/01 at 18:05:47, Sami wrote: > Make sure you update this to whatever naming convention the C++ code settles on. Sure.
https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/180001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:142: // TODO(xiaochengh): Is it possible for AsyncSlices to be attributed? On 2016/06/01 at 01:54:23, Xiaocheng wrote: > On 2016/06/01 at 00:59:23, benjhayden_chromium wrote: > > Can you file a bug, add chiniforooshan to it, and link to it here? > > OK. Wait.. What should I write in the bug's report? Do AsyncSlices really need to be attributed?
Following Ben's suggestions in the depended CL, Patch 12 now performs type-based branching in row construction. It also uses ChromeRendererHelper to determine is a process is renderer or not.
https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:164: // TODO(xiaochengh): Is it possible for AsyncSlices to be attributed? I think it won't be possible for async slices to be attributed for the foreseeable future, so this is an acceptable optimization. It would be architecturally cleaner and easier for us to generalize to non-ThreadSlice events in the future if this logic were implemented in BlameContextSnapshot.attributedEvents -> EventSet. The implementation of that getter can process the model once for all BlameContextSnapshots so that it doesn't iterate over the entire model more than once. Metrics and other non-UI tracing components such as the UserModelBuilder might want to use that information. In general, code that processes the model ought to live in model classes such as Snapshots, instead of views like side panels, so that the code can be re-used by multiple consumers. The view is just one possible consumer; metrics and other model classes such as the UserModel might want to use the same information. For example, ThreadSlice.overlappingSamples was originally implemented in a ui element until a metric wanted to use the information, so it was recently moved to the ThreadSlice model class. https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:182: updateContents_: function() { Blank line between methods, please.
https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:164: // TODO(xiaochengh): Is it possible for AsyncSlices to be attributed? On 2016/06/03 at 16:56:00, benjhayden_chromium wrote: > I think it won't be possible for async slices to be attributed for the foreseeable future, so this is an acceptable optimization. > > It would be architecturally cleaner and easier for us to generalize to non-ThreadSlice events in the future if this logic were implemented in BlameContextSnapshot.attributedEvents -> EventSet. The implementation of that getter can process the model once for all BlameContextSnapshots so that it doesn't iterate over the entire model more than once. > Metrics and other non-UI tracing components such as the UserModelBuilder might want to use that information. > In general, code that processes the model ought to live in model classes such as Snapshots, instead of views like side panels, so that the code can be re-used by multiple consumers. The view is just one possible consumer; metrics and other model classes such as the UserModel might want to use the same information. For example, ThreadSlice.overlappingSamples was originally implemented in a ui element until a metric wanted to use the information, so it was recently moved to the ThreadSlice model class. I'll add a TODO here. https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:169: // TODO(xiaochengh): Throw some warning for broken reference. I'll upload an updated patch set when this is done. What's the correct way of reporting warning from a UI component? https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:182: updateContents_: function() { On 2016/06/03 at 16:56:00, benjhayden_chromium wrote: > Blank line between methods, please. Oops. Done.
https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/220001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:169: // TODO(xiaochengh): Throw some warning for broken reference. On 2016/06/03 at 21:25:06, Xiaocheng wrote: > I'll upload an updated patch set when this is done. > > What's the correct way of reporting warning from a UI component? I'm removing this TODO. If there is any broken context reference, we may hit a lot of broken references since the total number of slices is large. The warning should be placed at slices' <tr-ui-a-single-event-sub-view>s instead.
Description was changed from ========== Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ========== to ========== [Tracing] Introduce Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ==========
lgtm Thanks! https://codereview.chromium.org/1996303002/diff/240001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/240001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:73: this.contexts.some(function(context) { Can you integrate this into the instanceof conditionals above?
The CQ bit was checked by xiaochengh@chromium.org
All right this is finally going to make it! Thank you all so much! https://codereview.chromium.org/1996303002/diff/240001/tracing/tracing/ui/ext... File tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html (right): https://codereview.chromium.org/1996303002/diff/240001/tracing/tracing/ui/ext... tracing/tracing/ui/extras/side_panel/frame_data_side_panel.html:73: this.contexts.some(function(context) { On 2016/06/06 at 16:51:12, benjhayden_chromium wrote: > Can you integrate this into the instanceof conditionals above? Done.
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, petrcermak@chromium.org, benjhayden@chromium.org Link to the patchset: https://codereview.chromium.org/1996303002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996303002/260001
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Introduce Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test ========== to ========== [Tracing] Introduce Frame Data Side Panel This patch adds a minimum usable UI of FrameBlamer [1] in Trace Viewer. The UI is a side panel that contains a table, where each row represents either a frame context or the top level context of a renderer process. When selecting a row, all slices attributed to the row's context and the context itself will get selected, so that their aggregated information will become available in the bottom panel. Screenshot: https://drive.google.com/open?id=0B7U-s0-cBvuQSENhbjRkeUJLcEk This patch depends on the class hierarchy of blame context classes in Trace Viewer, which are implemented in another patch [2]. [1] https://docs.google.com/document/d/15BB-suCb9j-nFt55yCFJBJCGzLg2qUm3WaSOPb8AP... [2] https://codereview.chromium.org/2016213002 BUG=catapult:#2260 TEST=tracing.ui.extras.side_panel.fame_data_side_panel_test Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
