Bug 178797 – Web Inspector: Canvas Tab: shader programs still not shown in canvas/recording path components

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

[Blaze Burg]

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

[Radar WebKit Bug Importer]

<rdar://problem/35175666>

[Devin Rousso]

I think this was fixed by <https://webkit.org/b/178807>.

[Blaze Burg]

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.

[Blaze Burg]

Reopening.

[Devin Rousso]

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

[Matt Baker]

This issue is being resolved by:

<https://webkit.org/b/178744> Web Inspector: add listing of Canvases/Programs/Recordings to the NavigationSidebar

[Blaze Burg]

*** This bug has been marked as a duplicate of bug 178744 ***

[Devin Rousso]

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

[Devin Rousso]

Created attachment 329160 [details]
Patch

[Devin Rousso]

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

[Devin Rousso]

Created attachment 331263 [details]
Patch

[Joseph Pecoraro]

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!

[Devin Rousso]

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.

(☞ﾟヮﾟ)☞

[Matt Baker]

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?

[Matt Baker]

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.

[Devin Rousso]

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.

[Matt Baker]

Created attachment 335346 [details]
[Image] Looks good to me

[Matt Baker]

I don't think this is an issue, with the recent changes to CanvasSidebarPanel/CanvasTabContentView. I suggest closing this, unless someone is opposed.

[Devin Rousso]

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

[Devin Rousso]

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>