[webkit-reviews] review granted: [Bug 178744] Web Inspector: add listing of Canvases/Programs/Recordings to the NavigationSidebar : [Attachment 333335] 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 333335: Patch

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




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

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

r=me.  There are a few things I'd change, but in the interest of time I think
we can separate them into followups.
 - I don't think we should default select a recording when looking at a canvas
 - we should hide the CanvasSidebarPanel when we are looking at the
CanvasOverviewContentView, as it doesn't really add anything
    - an alternative to hiding it would be to show the entire tree of all
Canvas (and related ShaderProgram and Recording), but this might be confusing
since we "narrow" down onto a specific Canvas when it's selected
 - if you click on the "Overview" path component after importing a recording,
all of the RecordingAction are still visible in the CanvasSidebarPanel, which
doesn't make much sense
 - the filter bar and "Only show visual actions" button are missing for
imported recordings
 - based on what I could gleam from setting a few breakpoints, we seem to be
going through `CanvasSidebarPanel.prototype.set action` multiple times when we
change actions

> Source/WebInspectorUI/UserInterface/Models/RecordingAction.js:95
>      get snapshot() { return this._snapshot; }
> +    set snapshot(snapshot) { this._snapshot = snapshot; }

Style: this should be in a separate code "block"

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.css:101
> +    transition: opacity 200ms ease-in-out;

I'm not sure if we have an official style guide on this, but I don't think we
use animations/transitions like this very often.  Personally, I think it's
unnecessary.

> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.css:40
> +    content: url(../Images/Canvas3D.svg);

This doesn't show for WebGL2, but I think that's fine for now.	You could add
`.webgl2` if you want, but we can do that as a followup.

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

I'm seeing this assertion get hit a lot.  I think the reason is that `set
recording` waits for `Recording.prototype.actions` to resolve, whereas this
doesn't, meaning that the TreeElement most likely isn't created till the next
frame.

> Source/WebInspectorUI/UserInterface/Views/CanvasSidebarPanel.js:282
> +	   this.recording = defaultSelectedRecording;

I personally think we shouldn't select a recording by default when looking at a
canvas, but we can discuss that in a followup.

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:55
> +	   this._recordShortcut = new WI.KeyboardShortcut(null,
WI.KeyboardShortcut.Key.Space, this._handleSpace.bind(this));
> +	   this._recordShortcut.implicitlyPreventsDefault = false;
> +
> +	   this._recordSingleFrameShortcut = new
WI.KeyboardShortcut(WI.KeyboardShortcut.Modifier.Shift,
WI.KeyboardShortcut.Key.Space, this._handleSpace.bind(this));
> +	   this._recordSingleFrameShortcut.implicitlyPreventsDefault = false;

These don't look like they do anything, because there is no
`CanvasSidebarPanel.prototype.get canvas` (used in `_handleSpace`).

> Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js:228
> +	   let canvas = this.navigationSidebarPanel.canvas;

See above (51-55).

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43
> +	   this._showRecordings = showRecordings;

I'm not sure that this is doing anything.  No matter what I try, I am always
shown the list of recordings for a given canvas.

> Source/WebInspectorUI/UserInterface/Views/RecordingContentView.js:458
> +	       this._action.snapshot = snapshot;

This isn't really the intended usage of `snapshot`, but I suppose its OK.  The
original intention of `snapshot` was to hold an image of the canvas' content so
we don't have to regenerate the actions for WebGL contexts in the frontend. 
Since we do regenerate each action for 2D contexts, this is never used, so I
guess it's fine (additionally, WebGL contexts don't have the same concept of
"state" as 2D, so it isn't really needed in the same way).

Since we already use supplementalRepresentedObjects, we might just want to
attach a new model object there that represents the context's state.