[webkit-reviews] review granted: [Bug 201650] Web Inspector: Canvas: instrument WebGPUDevice instead of GPUCanvasContext : [Attachment 378480] 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 201650: Web Inspector: Canvas: instrument WebGPUDevice instead of
GPUCanvasContext
https://bugs.webkit.org/show_bug.cgi?id=201650

Attachment 378480: Patch

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




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 378480
  --> https://bugs.webkit.org/attachment.cgi?id=378480
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!