Bug 178797 - Web Inspector: Canvas Tab: shader programs still not shown in canvas/recording path components
Summary: Web Inspector: Canvas Tab: shader programs still not shown in canvas/recordin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL: http://acko.net
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-10-25 09:43 PDT by Brian Burg
Modified: 2018-09-13 08:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (24.52 KB, patch)
2017-12-12 14:38 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (255.19 KB, image/png)
2017-12-12 14:38 PST, Devin Rousso
no flags Details
Patch (24.69 KB, patch)
2018-01-12 18:07 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] Looks good to me (431.87 KB, image/png)
2018-03-08 14:29 PST, Matt Baker
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-10-25 09:43:55 PDT
Steps to reproduce:

1. Visit about:blank
2. Go to acko.net
3. Open Canvas Tab
4. Select any canvas
=> Has no shader programs in path components

5. Create a recording
=> Has no shader programs in path components
Comment 1 Radar WebKit Bug Importer 2017-10-25 09:45:00 PDT
<rdar://problem/35175666>
Comment 2 Devin Rousso 2017-10-30 16:56:42 PDT
I think this was fixed by <https://webkit.org/b/178807>.
Comment 3 Brian Burg 2017-11-06 15:17:29 PST
You still can't get to a shader program via Canvas Tab without taking a recording. I think we should add shortcuts to them in each Canvas card. It could be in the title bar, or in one of the bottom corners with a dropdown.
Comment 4 Brian Burg 2017-11-06 15:17:42 PST
Reopening.
Comment 5 Devin Rousso 2017-11-06 16:02:27 PST
(In reply to Brian Burg from comment #3)
> You still can't get to a shader program via Canvas Tab without taking a
> recording. I think we should add shortcuts to them in each Canvas card. It
> could be in the title bar, or in one of the bottom corners with a dropdown.
What would you say to merging shader programs with recordings?  For example:

 - View Recordings... (1)
 - View Shader Programs... (2)
 - View Recordings and Shader Programs... (3)

We would decide what text to display depending on the number of recordings and shader programs.  Personally, I think "Shader Programs" is a bit wordy, and we could probably just shorten it to "Shaders", but I am not sure if that's entirely accurate.

Alternatively, we could have another select area right above the current "View Recordings... (X)" with similar text:

    |
    | View Shader Programs... (2)
    | View Recordings... (1)
    +--------------------------------

I would prefer the first option, though, as I'm not sure how nicely this will play with the rest of the CanvasContentView UI.  Also, if we add other children (such as WebGLTexture), it could get very cluttered/chaotic very fast.
Comment 6 Matt Baker 2017-11-08 13:22:01 PST
This issue is being resolved by:

<https://webkit.org/b/178744> Web Inspector: add listing of Canvases/Programs/Recordings to the NavigationSidebar
Comment 7 Brian Burg 2017-11-10 09:34:25 PST

*** This bug has been marked as a duplicate of bug 178744 ***
Comment 8 Devin Rousso 2017-12-12 14:12:49 PST
(In reply to Matt Baker from comment #6)
> This issue is being resolved by:
> 
> <https://webkit.org/b/178744> Web Inspector: add listing of
> Canvases/Programs/Recordings to the NavigationSidebar
I see this issue as separate from adding Shader Programs to the NavigationSidebar, seeing as we are back to square one if the sidebar is collapsed.  Since we provide a way of accessing recordings without the NavigationSidebar, we should do the same for Shader Programs.
Comment 9 Devin Rousso 2017-12-12 14:38:15 PST
Created attachment 329160 [details]
Patch
Comment 10 Devin Rousso 2017-12-12 14:38:31 PST
Created attachment 329161 [details]
[Image] After Patch is applied
Comment 11 Devin Rousso 2018-01-12 18:07:08 PST
Created attachment 331263 [details]
Patch
Comment 12 Joseph Pecoraro 2018-01-17 11:43:10 PST
Comment on attachment 331263 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:13
> +        Include Shader Programs in the "View Recordings... (#)" dropdown. Change the text depending
> +        on the number of Shader Programs and Recordings for the given canvas:
> +         - "View Shaders... (#)" when there are no Recordings
> +         - "View Recordings... (#)" when there are no Shader Programs
> +         - "View... (#)" when there are both (the dropdown will also group the options by type)

This strikes me as a cumbersome way to get to the shaders. Maybe an icon at the top when there are shaders, that when you click takes you directly to the shaders would be more obvious / convenient for users. I don't think I'd ever expect to use a drop down for something like this.

> Source/WebInspectorUI/UserInterface/Base/BiMap.js:37
> +    // Public

I think I'd expect a size getter, like Map:

    get size() { return this._keys.size; }
 
This would be very useful in the tests.

> Source/WebInspectorUI/UserInterface/Base/BiMap.js:76
> +    set(key, value)
> +    {
> +        this.deleteKey(key);
> +        this.deleteValue(value);
> +
> +        this._keys.set(key, value);
> +        this._values.set(value, key);
> +    }
> +
> +    deleteKey(key)
> +    {
> +        let value = this._keys.take(key);
> +
> +        let wasRemoved = this._values.delete(value);
> +        console.assert(wasRemoved);
> +
> +        return value;
> +    }
> +
> +    deleteValue(value)
> +    {
> +        let key = this._values.take(value);
> +
> +        let wasRemoved = this._keys.delete(key);
> +        console.assert(wasRemoved);
> +
> +        return key;
> +    }

These asserts seem overzealous. Since they assert that something was removed even if nothing existed.

    new BiMap([["key", "value"]]);
    // set("key", "value")
    //   deleteKey("key") => assert
    //   deleteValue("value") => assert

Or just someone calling the public API with a missing key:

    m.deleteKey("does-not-exist"); // => assert

Perhaps deleteKey/deleteValue should early return if the result of `take` was `undefined`?

You could also assert at the end of set() that the sizes are the same, which ensures the validity of the map:

    console.assert(this._values.size === this._keys.size);

> Source/WebInspectorUI/UserInterface/Base/BiMap.js:83
> +    [Symbol.iterator]()

Pro.

> LayoutTests/inspector/unit-tests/bimap.html:8
> +    let suite = InspectorTest.createSyncSuite("BiMap");

Great test!
Comment 13 Devin Rousso 2018-01-17 12:58:36 PST
Comment on attachment 331263 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:13
>> +         - "View... (#)" when there are both (the dropdown will also group the options by type)
> 
> This strikes me as a cumbersome way to get to the shaders. Maybe an icon at the top when there are shaders, that when you click takes you directly to the shaders would be more obvious / convenient for users. I don't think I'd ever expect to use a drop down for something like this.

Where do you suggest I put the icon?  Next to the record button with the [gl] document icon?  I feel that that is a bit more cluttered.  Also, if a WebGL/WebGL2 canvas has more than one Shader Program, would we just jump to the first one?  I did the dropdown so that if the user knows the order of creation for the Shader Programs then they can pick the shader they want directly.

>> Source/WebInspectorUI/UserInterface/Base/BiMap.js:76
>> +    }
> 
> These asserts seem overzealous. Since they assert that something was removed even if nothing existed.
> 
>     new BiMap([["key", "value"]]);
>     // set("key", "value")
>     //   deleteKey("key") => assert
>     //   deleteValue("value") => assert
> 
> Or just someone calling the public API with a missing key:
> 
>     m.deleteKey("does-not-exist"); // => assert
> 
> Perhaps deleteKey/deleteValue should early return if the result of `take` was `undefined`?
> 
> You could also assert at the end of set() that the sizes are the same, which ensures the validity of the map:
> 
>     console.assert(this._values.size === this._keys.size);

Ooo I didn't think of that case.  I'll adjust this.

>> Source/WebInspectorUI/UserInterface/Base/BiMap.js:83
>> +    [Symbol.iterator]()
> 
> Pro.

(☞゚ヮ゚)☞
Comment 14 Matt Baker 2018-01-17 13:36:49 PST
I think the awkwardness of incorporating Shaders into the current design is a sign that we need to rethink things. This is partly my doing, as I advocated early on for a combined Recordings/Shaders tab. I now think separate Canvas/Shaders tabs would be better.

1) Our shader inspection/debugging features, namely the ability to view, edit, and toggle shaders on/off is orthogonal to our recording story.

2) Showing recordings in the sidebar may work (but has been awkward to design, since recordings have their own sidebar). Showing both in the sidebar feels like an odd mix, especially if clicking a recording switches immediately to a new sidebar and clicking a shader does not.

What does everyone think?
Comment 15 Matt Baker 2018-01-17 13:39:52 PST
I'll also add

3) In the absence of the navigation sidebar in the Canvas tab, we're forced to include a shader menu similar to recordings, in the canvas tile. It adds to clutter as Devin mentioned, and extends the, IMO, already awkward UI we use for accessing recordings.
Comment 16 Devin Rousso 2018-01-17 13:48:08 PST
I personally think that we should keep shader programs in this tab, as it is already named "Canvas" and they are entirely a canvas concept (specifically WebGL and WebGL2).  I agree that it is hard to have both recordings and shader programs in the same UI, but I think it can be done in a way that isn't unintuitive.  Regardless of this, however, we need something *now* so that people who use STP have a way of viewing/editing shader programs.

Currently, the only way to view a shader program is to record a WebGL canvas, view the recording, and click the path component in the navigation area to select the shader program.  WebGL2 has no way of doing this.

This patch isn't meant to be a solution per se, but something that can be used in the meantime while we hash out ideas.  I think that including an icon in the canvas tile isn't a bad idea, but definitely needs some fine tuning.
Comment 17 Matt Baker 2018-03-08 14:29:31 PST
Created attachment 335346 [details]
[Image] Looks good to me
Comment 18 Matt Baker 2018-03-08 14:30:36 PST
I don't think this is an issue, with the recent changes to CanvasSidebarPanel/CanvasTabContentView. I suggest closing this, unless someone is opposed.
Comment 19 Devin Rousso 2018-03-08 23:42:11 PST
(In reply to Matt Baker from comment #18)
> I don't think this is an issue, with the recent changes to
> CanvasSidebarPanel/CanvasTabContentView. I suggest closing this, unless
> someone is opposed.
My only reluctance to closing this issue is that from the Overview, it is still not possible to view Shaders, while you can see Recordings (via the dropdown on the Canvas "card").  We might consider either adding the Shaders to that card somewhere (this patch merges them into the dropdown) or removing the Recording dropdown so it's more consistent.  It's more of a NIT though, so if people feel strongly otherwise and have a good reason, I am not against it.
Comment 20 Devin Rousso 2018-03-14 17:32:16 PDT
Path component support for shaders was added in r228301 <https://webkit.org/b/178744>.

I have created a separate bug for the "remainder" of the work, namely adding some sort of UI for shaders in the canvas tile.  <https://webkit.org/b/183650>