[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]

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
NavigationSidebar
https://bugs.webkit.org/show_bug.cgi?id=178744

Attachment 333406: Patch

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




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

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",
this._handlePreviewImageClick.bind(this));

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
suffice.