Since contexts can sometimes be hard to retrieve, we should make it possible for users to log the context object to the console (the <canvas> element can be accessed via the context's `.canvas` getter).
Created attachment 312944 [details] [Image] After Patch is applied
Created attachment 312955 [details] [Patch] WIP
Comment on attachment 312955 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=312955&action=review Looking good. Some simple comments for the next iteration. > LayoutTests/inspector/canvas/resolveCanvasContext.html:10 > + window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("2d"); > + window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgl"); > + window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgl2"); > + window.context2d = document.body.appendChild(document.createElement("canvas")).getContext("webgpu"); Whats up here? Shouldn't they be distinct variables? > LayoutTests/inspector/canvas/resolveCanvasContext.html:21 > + suite.addTestCase({ > + name: "Canvas.resolveCanvasContext.validIdentifier.Canvas2D", > + description: "Should return a valid object for the given canvas identifier.", Most of these tests are near direct copies. You can create a helper and simplify the tests: function addTestCase({name, description, type, className}) { suite.addTestCase({ name, description, test(resolve, reject) { let canvas = WebInspector.canvasManager.canvases.find((canvas) => canvas.contextType === type); ... CanvasAgent.resolveCanvasContext(canvas.ientifier, objectGroup, (error, object) => { ... InspectorTest.expectEqual(object.className, className, `Payload should have className "${className}".`); }); } }); } So with all the boilerplate packaged up the repeated tests can look very simple. addTestCase({ name: "Canvas.resolveCanvasContext.Canvas2D", description: "...", type: WebInspector.Canvas.ContextType.Canvas2D, className: "CanvasRenderingContext2D", }); addTestCase({ name: "Canvas.resolveCanvasContext.WebGPU", description: "...", type: WebInspector.Canvas.ContextType.WebGPU, className: "WebGPURenderingContext", }); > Source/WebCore/inspector/InspectorCanvasAgent.cpp:46 > +#if ENABLE(WEBGL) > +#include "JSWebGLRenderingContext.h" > +#include "JSWebGL2RenderingContext.h" > +#endif > + > +#if ENABLE(WEBGPU) > +#include "JSWebGPURenderingContext.h" > +#endif The usual style for includes would look like: #inculde "config.h" #include "Foo.h" #include "A.h" #include "B.h" #include "C.h" #include <wtf/AA.h> #include <wtf/BB.h> #if ENABLE(FOO) #include "FooA.h" #endif #if ENABLE(BAR) #include "FooB.h" #endif However in this case I don't think you even need the ENABLE blocks. We always generate these files and inside the files are enable guards, so it tends to be easier and clearer to just include them unconditionally. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:188 > + String objectGroupName = objectGroup ? *objectGroup : String(); This could be pushed down to right before the wrapObject line since its not used before then. > Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:54 > + populateContextMenu(contextMenu, event) Should this be calling super.populateContextMenu?
<rdar://problem/33111581>
Created attachment 314512 [details] Patch
<rdar://problem/33109380>
Comment on attachment 314512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314512&action=review Nice! r=me > LayoutTests/platform/mac/TestExpectations:1145 > +webkit.org/b/174066 inspector/canvas/resolveCanvasContext-webgl2.html [ Pass Timeout ] > +webkit.org/b/174066 inspector/canvas/resolveCanvasContext-webgpu.html [ Pass Timeout ] Do these really need to be pass timeout as well? > Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:-494 > -localizedStrings["Layer"] = "Layer"; Whoa, when did this go away?! Weird.
Created attachment 314614 [details] Patch
The commit-queue encountered the following flaky tests while processing attachment 314614 [details]: The commit-queue is continuing to process your patch.
Comment on attachment 314614 [details] Patch Attachment 314614 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4057579 New failing tests: inspector/canvas/resolveCanvasContext-webgpu.html
Created attachment 314626 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 314614 [details] Patch Attachment 314614 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4057602 New failing tests: inspector/canvas/resolveCanvasContext-webgpu.html
Created attachment 314628 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 314614 [details] Patch Attachment 314614 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4057649 New failing tests: inspector/canvas/resolveCanvasContext-webgpu.html
Created attachment 314634 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 314643 [details] Patch
Comment on attachment 314643 [details] Patch Clearing flags on attachment: 314643 Committed r219150: <http://trac.webkit.org/changeset/219150>
All reviewed patches have been landed. Closing bug.