[webkit-reviews] review denied: [Bug 183650] Web Inspector: Canvas tab: create icons for recordings/shaders in the preview tile : [Attachment 335865] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 183650: Web Inspector: Canvas tab: create icons for recordings/shaders in
the preview tile
https://bugs.webkit.org/show_bug.cgi?id=183650

Attachment 335865: Patch

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




--- Comment #13 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 335865
  --> https://bugs.webkit.org/attachment.cgi?id=335865
Patch

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

I think this is a good idea. Often it can be useful to just see which context
has shaders to know which is likely to be the important context.

r- just so that we can see an updated patch. Otherwise, this is an r+ able
patch.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1035
> -localizedStrings["View Recordings... (%d)"] = "View Recordings... (%d)";
> +localizedStrings["View Recording"] = "View Recording";
> +localizedStrings["View Shader"] = "View Shader";

Hmm, what if the UI looked like this:

    Shaders, No Recordings:
    [@]

    Shaders, 1 Recording:
    [@] o_o (1)

    Shaders, 2 Recordings:
    [@] o_o (2)

I think the (#) gives extra information that wasn't available but might be
useful to the user... "oh this is the one I took multiple recordings on".
However if we think most users will only ever do a single recording then it
probably won't 

Having a button to quickly get to the shaders seems nice. Seems like a good way
to make use of an extra 20px that would otherwise not be used =)

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:276
> +	   let {canvas} = event.data;

Heh. I just ran some tests, and the perf of this is (as it should be) identical
to the direct access.

    let {canvas} = event.data;
    let canvas = event.data.canvas;

Seems like an interesting pattern that is a few less characters =)

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:419
> +    _updateViewRelated()

I feel like this name needs another word. Maybe:

    _updateViewRelatedItems()

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:440
> +	   if (shaderPrograms.length === 1) {
> +	       WI.showRepresentedObject(shaderPrograms[0]);
> +	       return;
> +	   }

I'd have to experience a picker. Is it worth having a picker or just getting
the user to the UI and then letting them select the shader?

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463
> +	   if (recordings.length === 1) {
> +	       WI.showRepresentedObject(recordings[0]);
> +	       return;
> +	   }

Likewise. Just clicking the entire canvas gets the user into this. Is it worth
having them select the recording here, or from the normal UI?

I see these icons more as indicators than useful for actions.