Bug 183650 – Web Inspector: Canvas tab: create icons for recordings/shaders in the preview tile

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Instead of having a "View Recordings..." dropdown, we should just have two icons (one each for recordings and shaders) adopt a similar approach to path components (if there is only one, select it on click, otherwise show a dropdown).  This way, all related "resources" to the canvas are easily accessible from the Overview.

[Devin Rousso]

Created attachment 335830 [details]
Patch

[Devin Rousso]

Created attachment 335831 [details]
[Image] After Patch is applied

[EWS Watchlist]

Comment on attachment 335830 [details]
Patch

Attachment 335830 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6961032

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/websocket/tests/hybi/inspector/before-load.html
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html
http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html

[EWS Watchlist]

Created attachment 335833 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335830 [details]
Patch

Attachment 335830 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6961053

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html
http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/websocket/tests/hybi/inspector/before-load.html
http/tests/inspector/network/har/har-page.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html

[EWS Watchlist]

Created attachment 335834 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 335830 [details]
Patch

Attachment 335830 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6961062

New failing tests:
http/tests/inspector/network/resource-response-source-memory-cache.html
http/tests/websocket/tests/hybi/inspector/before-load.html
http/tests/inspector/network/resource-response-source-memory-cache-revalidate-expired-only.html
http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts.html
http/tests/inspector/network/set-resource-caching-disabled-memory-cache.html

[EWS Watchlist]

Created attachment 335835 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 335836 [details]
Patch

Oops.  Forgot some other inspector tests.

[EWS Watchlist]

Comment on attachment 335836 [details]
Patch

Attachment 335836 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6961737

New failing tests:
http/tests/inspector/network/har/har-page.html

[EWS Watchlist]

Created attachment 335839 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 335865 [details]
Patch

Splitting the changes to WI.Collection into a separate patch because of EWS failures

[Joseph Pecoraro]

Comment on attachment 335865 [details]
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.

[Devin Rousso]

Comment on attachment 335865 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1035
>> +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 =)

My only hesitance to (#) is that it might be confusing as to which it relates to.  I'd see a (#) and think "thats the total number of 'associated' things to this canvas (recordings + shaders)".  Also, I don't think many WebGL canvases will have more than one or two shaders, so it's probably pretty rare.

As a side note, I laughed at "o_o" 😛

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:440
>> +        }
> 
> 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?

I think that any time we can eliminate an action by the user, it's a good idea.  This follows the same pattern as path components, and I think we should do it.  Also, see my above comment about the number of shaders.

>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463
>> +        }
> 
> 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.

This actually shouldn't be the case.  Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured.

[Matt Baker]

Comment on attachment 335865 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463
>>> +        }
>> 
>> 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.
> 
> This actually shouldn't be the case.  Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured.

Are you referring to when we automatically show a recording once it completes? (which IIR is for any recording not started from the console). Clicking a canvas from the Overview should always show the same view, correct?

[Devin Rousso]

Comment on attachment 335865 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:463
>>>> +        }
>>> 
>>> 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.
>> 
>> This actually shouldn't be the case.  Recording's shouldn't be automatically selected when viewing a canvas unless the recording has just finished being captured.
> 
> Are you referring to when we automatically show a recording once it completes? (which IIR is for any recording not started from the console). Clicking a canvas from the Overview should always show the same view, correct?

Yes.  Clicking on a canvas in the overview should just show a `WI.CanvasContentView` (no recording selected) for that canvas.  When a non-console recording finishes, a `WI.RecordingContentView` should be shown with similar sidebars as `WI.CanvasContentView` (any shaders are shown), except with recording specific data (e.g. actions list, other details sidebars, etc).

[Devin Rousso]

Created attachment 346364 [details]
Patch

This patch is mainly a rebase with a few style changes.  My build is currently broken, so I was unable to test it.  :/

[Devin Rousso]

Created attachment 347227 [details]
Patch

Fixed a few bugs

[Joseph Pecoraro]

Comment on attachment 347227 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 347227 [details]
Patch

Clearing flags on attachment: 347227

Committed r235093: <https://trac.webkit.org/changeset/235093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43534413>