catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 564 forks source link

devtools Object Snapshot are not display in tracing #2223

Open fmeawad opened 8 years ago

fmeawad commented 8 years ago

Record a trace with "disabled-by-default-devtools.timeline.picture" category enabled. Click on one of the green dots that represents an Object Snapshot Ouptut: Details panel is empty Expected output: Details panel showing the Object Snapshot

JS Error:

Uncaught TypeError: Cannot read property 'forEach' of undefined  @ display_item_debugger.html:208
set displayItemList @ display_item_debugger.html:208
updateContents @ display_item_list_view.html:38
set objectSnapshot @ object_snapshot_view.html:42
set modelEvent @ object_snapshot_view.html:29
set selection @ single_object_snapshot_sub_view.html:83
onSelectionChanged_ @ analysis_view.html:179
dispatchEvent @ event_target.html:111
dispatchChangeEvent_ @ brushing_state_controller.html:41
set currentBrushingState @ brushing_state_controller.html:163
changeSelectionFromTimeline @ brushing_state_controller.html:225
onEndSelection_ @ timeline_track_view.html:1019
onMouseUp_ @ mouse_mode_selector.html:378
cleanupAndDispatchToMouseUp @ mouse_tracker.html:101
chiniforooshan commented 8 years ago

Could it be because of this bug? If you send me the trace I can check if patching that CL fixes the issue.

fmeawad commented 8 years ago

I do not think so, the C++ side should populate the "items" list here. But I am suspecting that @primiano changes in this CL may have caused it not to be written properly.

You can find the trace I am testing with shared in drive.

primiano commented 8 years ago

Hmm that was 6 months ago. @petrcermak any ideas?

petrcermak commented 8 years ago

I don't think it's @primiano's change (there doesn't seem to be dot expansion involved here).

From what I can see, the items array might not be added to the dictionary if include_items is false, which is the case when the "cc.debug.display_items" tracing category is not enabled.

Perhaps the include_items flag should also be set to true when "disabled-by-default-devtools.timeline.picture" is enabled, but I think it would be better to just make Trace Viewer handle missing items graciously. Is there any relationship between "disabled-by-default-devtools.timeline.picture" and "cc.debug.display_items"?

fmeawad commented 8 years ago

I confirm that you need both categories to get that displayed properly, it is counter-intuitive. We can:

  1. Require both categories to generate the list in the first place
  2. Update the UI to display the Pictures only without the list if the list is not available (+ a warning)
  3. Update the UI to indicate that both categories should be enabled to get the correct display list.

Regardless, if you are trying to reproduce this:

petrcermak commented 8 years ago
  1. Not sure what the precedent is here, but I would personally avoid introducing cross-dependencies between categories if possible.
  2. This seems like the best option to me: Make the UI more robust and tell the user what they need to do to get the list.
  3. I don't see any benefit of this option over 2.