[webkit-reviews] review granted: [Bug 173396] Web Inspector: Support listing WebGL2 and WebGPU contexts : [Attachment 314179] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 173396: Web Inspector: Support listing WebGL2 and WebGPU contexts
https://bugs.webkit.org/show_bug.cgi?id=173396

Attachment 314179: Patch

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




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

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

Very nice!

> LayoutTests/inspector/canvas/resources/create-context-utilities.js:69
> +	       name: suite.name + "." + name,

I'm on the fence about this. I think its fine, you are generating tests. But
one of the things I've always tried to strive for is the ability to search our
code for the name of a failing test case:
"Canvas.CreateContextWebGPU.NoCanvases" and find the code associated with it.
This breaks that, but I'm probably overblowing the usefulness. I don't think
I've actually done this search many times.

> LayoutTests/inspector/canvas/resources/create-context-utilities.js:91
> +    window.addCSSCanvasTestCase = function(contextType) {

It is my understanding that you can't change the context inside of a
-webkit-canvas. Can you throw an error if this gets called more than once in a
test?

Basically something like this:

    let addedCSSCanvas = false;
    window.addCSSCanvasTestCase = function(contextType) {
	InspectorTest.assert(!addedCSSCanvas, "addCSSCanvasTestCase should only
be called once in a test");
	addedCSSCanvas = true;
	... suite.addTestCase ...
    });

> LayoutTests/platform/gtk/TestExpectations:1263
> +inspector/canvas/create-context-webpu.html [ Skip ]

This is a typo it should be webgpu.html

> LayoutTests/platform/ios/TestExpectations:45
> +inspector/canvas/create-context-webpu.html [ Skip ]

This is a typo it should be webgpu.html

> LayoutTests/platform/win/TestExpectations:2386
> +inspector/canvas/create-context-webpu.html [ Skip ]

Remove this line.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:49
> +#if ENABLE(WEBGL)
> +#if ENABLE(WEBGL2)
> +#include "WebGL2RenderingContext.h"
> +#endif
>  #include "WebGLContextAttributes.h"
> +#include "WebGLRenderingContext.h"
>  #include "WebGLRenderingContextBase.h"
> +#endif
> +
> +#if ENABLE(WEBGPU)
> +#include "WebGPURenderingContext.h"
> +#endif

This is a mess. I'd recommend not nesting WEBGL2 and WEBGL. Just:

    #if ENABLE(WEBGL)
    #include "WebGLRenderingContext.h"
    #endif

    #if ENABLE(WEBGL2)
    #include "WebGLContextAttributes.h"
    #include "WebGL2RenderingContext.h"
    #include "WebGLRenderingContextBase.h"
    #endif

    #if ENABLE(WEBGPU)
    #include "WebGPURenderingContext.h"
    #endif

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:52
>  #include <inspector/IdentifiersFactory.h>
>  #include <inspector/InspectorProtocolObjects.h>

Style: These would normally be next to line 36.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:283
> +#if ENABLE(WEBGL)
> +    else if (is<WebGLRenderingContext>(context))
>	   contextType = Inspector::Protocol::Canvas::ContextType::WebGL;
> +#if ENABLE(WEBGL2)
> +    else if (is<WebGL2RenderingContext>(context))
> +	   contextType = Inspector::Protocol::Canvas::ContextType::WebGL2;
> +#endif
> +#endif

Likewise this doesn't need to nest and it becomes much more readable:

    #if ENABLE(WEBGL)
	else if (is<WebGLRenderingContext>(context))
	    contextType = Inspector::Protocol::Canvas::ContextType::WebGL;
    #endif
    #if ENABLE(WEBGL2)
	else if (is<WebGL2RenderingContext>(context))
	    contextType = Inspector::Protocol::Canvas::ContextType::WebGL2;
    #endif
    #if ENABLE(WEBGPU)
	else if (is<WebGPURenderingContext>(context))
	    contextType = Inspector::Protocol::Canvas::ContextType::WebGPU;
    #endif

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:189
>      Canvas2D: "canvas-2d",
>      WebGL: "webgl",
> +    WebGL2: "webgl2",
> +    WebGPU: "webgpu",

Have we considered different icons for these?