Bug 201650

Created attachment 378480 [details] Patch

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

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!

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

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

Created attachment 378521 [details] Patch

Created attachment 378530 [details] Patch Fix win build

Comment on attachment 378530 [details] Patch Clearing flags on attachment: 378530 Committed r249753: <https://trac.webkit.org/changeset/249753>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55249232>

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.

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>

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

Comment on attachment 378590 [details] Patch Clearing flags on attachment: 378590 Committed r249786: <https://trac.webkit.org/changeset/249786>

All reviewed patches have been landed. Closing bug.