[webkit-reviews] review granted: [Bug 201675] Web Inspector: Canvas: show WebGPU shader pipelines : [Attachment 378862] 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 201675: Web Inspector: Canvas: show WebGPU shader pipelines
https://bugs.webkit.org/show_bug.cgi?id=201675

Attachment 378862: Patch

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




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

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

r=me

Nice! Great tests.

The WebGPU Pipeline stuff went over my head so you might want to get a nod from
a WebGPU expert.

WK1 tests seem to have failed, maybe WebGPU is not enabled there?:
CONSOLE MESSAGE: line 83: Unhandled Promise Rejection: TypeError: undefined is
not an object (evaluating 'navigator.gpu.requestAdapter')

> Source/WebCore/inspector/InspectorShaderProgram.cpp:120
>	   return nullptr;

Seems we should also ASSERT_NOT_REACHED for WebGL here.

> Source/WebCore/inspector/InspectorShaderProgram.cpp:231
> +				   if (entryPoint)
> +				       shader.value().entryPoint = *entryPoint;

Is there any concern here about a malicious kind of entryPoint string?

> Source/WebCore/platform/graphics/gpu/GPUComputePipeline.h:70
> +    // Prserved for Web Inspector recompilation.

Typo: "Prserved" => "Preserved", This happens a few times.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:45
> +    static _contextTypeSupportsProgramType(contextType, programType)

These are static, might as well not give them an underscore prefix.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:118
> +	   if (this._canvas.contextType === WI.Canvas.ContextType.WebGPU)
> +	       return;
> +
> +	   console.assert(this._programType ===
ShaderProgram.ProgramType.Render);
> +	   console.assert(this._canvas.contextType ===
WI.Canvas.ContextType.WebGL || this._canvas.contextType ===
WI.Canvas.ContextType.WebGL2);

Maybe move the assertions above the early return?

That would help catch if we ever did this for a WebGPU context among other
issues.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:152
> +	   console.assert((!!entryPoint) === (this._canvas.contextType ===
WI.Canvas.ContextType.WebGPU));

Style: Could drop the parenthesis on the left side.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:63
> +		   textEditor.mimeType = "x-shader/x-vertex"; // Treat this
like a vertex shader for CodeMirror.

You could probably add something to CodeMirrorAdditions.js:

    // FIXME: Add a WHLSL specific mode.
    // Treat a computer shader like a vertex shader.
    CodeMirror.defineMIME("x-shader/x-compute", "x-shader/x-vertex");

And keep this code rather clean.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.js:195
> +	   case WI.Canvas.ContextType.WebGPU:
> +	       extension = WI.unlocalizedString(".whlsl");

Is this the preferred extension? Perhaps `.wsl`? I was unable to find any
concrete examples. whlsl seems find to me!

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:55
> +	       this.element.addEventListener("mouseover",
this._handleMouseOver.bind(this));
> +	       this.element.addEventListener("mouseout",
this._handleMouseOut.bind(this));

I assume we will want to / should be able to do this for WebGPU in the future.
So maybe add a FIXME.

> LayoutTests/ChangeLog:11
> +	   * inspector/canvas/requestShaderSource-expected.txt:

It kind stinks when we have:

    inspector/foo/bar.html
    inspector/foo/bar-a.html
    inspector/foo/bar-b.html

That sorting gets ugly for:

    inspector/foo/bar-a-expected.html
    inspector/foo/bar-b-expected.html
    inspector/foo/bar-expected.html

Maybe we should rename this common tests to something like:

    inspector/canvas/requestShaderSource-invalid.html
    inspector/canvas/requestShaderSource-common.html

> LayoutTests/ChangeLog:12
> +	   * inspector/canvas/updateShader.html:

Ditto

> LayoutTests/inspector/canvas/requestShaderSource-webgl.html:63
> +		   InspectorTest.log(source);

`InspectorTest.ShaderProgram.logSource` instead of just `InspectorTest.log`?

> LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js:113
> +	   InspectorTest.log("`" + source + "`");

Maybe "```\n" + source + "```"? That is more Markdown like output for generic
code.

> LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu-expected.txt:15
> +PASS: Added "compute ShaderProgram.

Nit: Unbalanced quote

> LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu.html:29
> +		   InspectorTest.expectThat(program instanceof
WI.ShaderProgram, `Added "${programType} ShaderProgram.`);

Style: Unbalanced quote around ${programType}.

> LayoutTests/inspector/canvas/updateShader-webgpu.html:96
> +    test({
> +	   name: "Canvas.updateShader.WebGPU.Fragment.Invalid",
> +	   programType: WI.ShaderProgram.ProgramType.Render,
> +	   shaderType: WI.ShaderProgram.ShaderType.Fragment,
> +	   source: "INVALID",
> +	   entryPoint: "INVALID",
> +	   shouldThrow: true,
> +    });

What happens if you attempt to update with only an invalid or missing
entryPoint?