[webkit-reviews] review granted: [Bug 178744] Web Inspector: add listing of Canvases/Programs/Recordings to the NavigationSidebar : [Attachment 333406] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 8 14:03:03 PST 2018

Devin Rousso <webkit at devinrousso.com> has granted Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 178744: Web Inspector: add listing of Canvases/Programs/Recordings to the

Attachment 333406: Patch


--- Comment #31 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 333406
  --> https://bugs.webkit.org/attachment.cgi?id=333406

View in context: https://bugs.webkit.org/attachment.cgi?id=333406&action=review

r=me.  Super awesome job!  Looks great =D

I really like the change to `RecordingAction` with the saved `state`.  I think
it's a LOT cleaner.  Only thing I see that's "new" is that you're missing a
ChangeLog entry for the changes to RecordingAction.

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:158
> +	       this._previewImageElement.addEventListener("click",

Now that I think about it, is it necessary to have this listener?  Doesn't
CollectionContentView already attempt to show the representedObject when the
selected item changes?	The reason I mention this is because when I click on
one of the navigation items when in Overview, it drills down into that
particular canvas, meaning that it can be somewhat annoying to try to start
multiple recordings at the same time.  We should address that in a followup.

> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:108
> +	   console.assert(treeElement, "Missing tree element for recording
action.", action);

I'm not sure if you addressed this issue, so just in case, I am still seeing
this assertion hit a lot with the new patch.

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:159
> +	   // FIXME: Create tree elements/cards for recordings belonging to the
removed canvas.

We also need a card for imported recordings :)

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:463
> +	   this._action.state = applyActions(snapshot.index, this._index);

I don't think we want to reset this each time we reapply an action.  It should
never be different.  Maybe a `console.assert` with `Object.shallowEquals` would

More information about the webkit-reviews mailing list