Bug 201650 – Web Inspector: Canvas: instrument WebGPUDevice instead of GPUCanvasContext

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

[Devin Rousso]

Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.  A `GPUCanvasContext` is basically just a display "client" of the device, and isn't even required (e.g. compute pipeline).  We should treat the `GPUCanvasContext` almost like a `-webkit-canvas` client of a `WebGPUDevice`.

This change should also carry to the frontend (e.g. use "Device" instead of "Canvas"/"Context").

[Devin Rousso]

Created attachment 378480 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Joseph Pecoraro]

Comment on attachment 378480 [details]
Patch

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

r=me - How does this look in the UI? Internals seem good.

> Source/JavaScriptCore/ChangeLog:8
> +        Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.

These repeated details only make sense inside of the WebCore ChangeLog. Not JavaScriptCore / WebInspectorUI.

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:81
> +    ~WebGPUDevice();

virtual?

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:124
> +            if (context && existsInCurrentPage(context->canvasBase().scriptExecutionContext()))

Is this `context` check necessary? It seems unlikely that `CanvasRenderingContext::instances` will ever return a nullptr, and if so that should probably be addressed.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:133
> +            if (device && existsInCurrentPage(device->scriptExecutionContext()))

Is this `device` check necessary? It seems unlikely that `WebGPUDevice::instances` will ever return a nullptr, and if so that should probably be addressed.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:636
> +    // FIXME: notify old device

Still relevant?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:117
> +        WI.Canvas._nextContextUniqueDisplayNameNumber = 1;
> +        WI.Canvas._nextDeviceUniqueDisplayNameNumber = 1;

Nit: We could drop the `WI.` prefix for these.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:166
> +                this._uniqueDisplayNameNumber = this.constructor._nextDeviceUniqueDisplayNameNumber++;

Style: Instead of `this.constructor.` I think we can just use `Canvas.`.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:81
> +    // COMPATIBILITY (iOS 13): Canvas.events.cssCanvasClientNodesChanged was renamed to Canvas.events.clientNodesChanged.
> +    cssCanvasClientNodesChanged(canvasId)

Nice!

[Devin Rousso]

Comment on attachment 378480 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Most of the actual "work" done with Web GPU actually uses a `WebGPUDevice`.
> 
> These repeated details only make sense inside of the WebCore ChangeLog. Not JavaScriptCore / WebInspectorUI.

I personally like including them in the various ChangeLogs, as it provides an overarching context to the changes.  This way, when reading JavaScriptCore changes, you can still see what's generally going on.

>> Source/WebCore/Modules/webgpu/WebGPUDevice.h:81
>> +    ~WebGPUDevice();
> 
> virtual?

YES!!!  I think this is partially the reason for some of the test issues I was encountering :D

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:124
>> +            if (context && existsInCurrentPage(context->canvasBase().scriptExecutionContext()))
> 
> Is this `context` check necessary? It seems unlikely that `CanvasRenderingContext::instances` will ever return a nullptr, and if so that should probably be addressed.

Fair point.

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:636
>> +    // FIXME: notify old device
> 
> Still relevant?

Yes, but I actually meant to rework this and kinda forgot -.-

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:117
>> +        WI.Canvas._nextDeviceUniqueDisplayNameNumber = 1;
> 
> Nit: We could drop the `WI.` prefix for these.

I love it <3

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:166
>> +                this._uniqueDisplayNameNumber = this.constructor._nextDeviceUniqueDisplayNameNumber++;
> 
> Style: Instead of `this.constructor.` I think we can just use `Canvas.`.

Ditto

[Devin Rousso]

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

(In reply to Joseph Pecoraro from comment #3)
> r=me - How does this look in the UI? Internals seem good.
The UI is the exact same, except that "Canvas" is replaced with "Device" :P

Hovering interactions also work, since we treat the "related" `HTMLCanvasElement` (via `GPUCanvasContext`) as a "client".

[Devin Rousso]

Created attachment 378521 [details]
Patch

[Devin Rousso]

Created attachment 378530 [details]
Patch

Fix win build

[WebKit Commit Bot]

Comment on attachment 378530 [details]
Patch

Clearing flags on attachment: 378530

Committed r249753: <https://trac.webkit.org/changeset/249753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55249232>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/249753/webkit

broke inspector/canvas/shaderProgram-add-remove-webgl.html and caused it to crash on all Mac.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fcanvas%2FshaderProgram-add-remove-webgl.html

Log:
https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r249760%20(6563)/inspector/canvas/shaderProgram-add-remove-webgl-crash-log.txt

Can this be fixed today? otherwise we will need to roll this out.

[Truitt Savell]

Reverted r249753 for reason:

caused inspector/canvas/shaderProgram-add-remove-webgl.html to crash on all Mac platforms.

Committed r249776: <https://trac.webkit.org/changeset/249776>

[Devin Rousso]

Created attachment 378590 [details]
Patch

The values in `WebGLProgram::instances` aren't always set, as `WebGLRenderingContextBase::~WebGLRenderingContextBase` can change it to be `nullptr`.  `WebGLRenderingContextBase::~WebGLRenderingContextBase` doesn't want to remove itself from the `WebGLProgram::instances` (as in removing the key `WebGLProgram` too) because otherwise `WebGLProgram::~WebGLProgram` may fail to remove (and we would have a `WebGLProgram` that exists that isn't recorded in the instances list, which we don't want).

[WebKit Commit Bot]

Comment on attachment 378590 [details]
Patch

Clearing flags on attachment: 378590

Committed r249786: <https://trac.webkit.org/changeset/249786>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.