RESOLVED FIXED 173397
Web Inspector: Allow users to log any tracked canvas context
https://bugs.webkit.org/show_bug.cgi?id=173397
Summary Web Inspector: Allow users to log any tracked canvas context
Devin Rousso
Reported 2017-06-14 18:36:38 PDT
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).
Attachments
[Image] After Patch is applied (490.14 KB, image/png)
2017-06-14 18:38 PDT, Devin Rousso
no flags
[Patch] WIP (16.74 KB, patch)
2017-06-14 22:54 PDT, Devin Rousso
hi: review-
hi: commit-queue-
Patch (28.21 KB, patch)
2017-07-03 13:31 PDT, Devin Rousso
joepeck: review+
Patch (27.35 KB, patch)
2017-07-05 10:32 PDT, Devin Rousso
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-elcapitan (996.02 KB, application/zip)
2017-07-05 11:31 PDT, Build Bot
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.10 MB, application/zip)
2017-07-05 11:37 PDT, Build Bot
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (1.75 MB, application/zip)
2017-07-05 12:13 PDT, Build Bot
no flags
Patch (26.98 KB, patch)
2017-07-05 12:57 PDT, Devin Rousso
no flags
Devin Rousso
Comment 1 2017-06-14 18:38:34 PDT
Created attachment 312944 [details] [Image] After Patch is applied
Devin Rousso
Comment 2 2017-06-14 22:54:18 PDT
Created attachment 312955 [details] [Patch] WIP
Joseph Pecoraro
Comment 3 2017-06-15 11:42:57 PDT
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?
Radar WebKit Bug Importer
Comment 4 2017-07-03 13:05:06 PDT
Devin Rousso
Comment 5 2017-07-03 13:31:33 PDT
Blaze Burg
Comment 6 2017-07-03 13:53:14 PDT
Joseph Pecoraro
Comment 7 2017-07-03 18:51:04 PDT
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.
Devin Rousso
Comment 8 2017-07-05 10:32:38 PDT
WebKit Commit Bot
Comment 9 2017-07-05 11:04:10 PDT
The commit-queue encountered the following flaky tests while processing attachment 314614 [details]: The commit-queue is continuing to process your patch.
Build Bot
Comment 10 2017-07-05 11:31:27 PDT
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
Build Bot
Comment 11 2017-07-05 11:31:28 PDT
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
Build Bot
Comment 12 2017-07-05 11:37:01 PDT
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
Build Bot
Comment 13 2017-07-05 11:37:02 PDT
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
Build Bot
Comment 14 2017-07-05 12:13:23 PDT
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
Build Bot
Comment 15 2017-07-05 12:13:24 PDT
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
Devin Rousso
Comment 16 2017-07-05 12:57:16 PDT
WebKit Commit Bot
Comment 17 2017-07-05 13:13:59 PDT
Comment on attachment 314643 [details] Patch Clearing flags on attachment: 314643 Committed r219150: <http://trac.webkit.org/changeset/219150>
WebKit Commit Bot
Comment 18 2017-07-05 13:14:01 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.