Bug 173965

Created attachment 314125 [details] [Patch] WIP

Created attachment 314126 [details] [Patch] WIP Oops. Wrong patch :P

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

Created attachment 314201 [details] Patch

Attachment 314201 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLCanvasElement.cpp:50: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 314201 [details] Patch Attachment 314201 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4023322 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314209 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 314201 [details] Patch Attachment 314201 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4023341 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314216 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 314333 [details] [Patch] For bots

Attachment 314333 [details] did not pass style-queue: ERROR: Source/WebCore/html/HTMLCanvasElement.cpp:50: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 314201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314201&action=review > LayoutTests/inspector/canvas/css-canvas-clients.html:122 > + <p>Test that CanvasAgent is able to send context attributes.</p> This description seems wrong. You are testing css canvas clients. > Source/JavaScriptCore/inspector/protocol/Canvas.json:112 > + { > + "name": "cssCanvasClientNodesChanged", > + "parameters": [ > + { "name": "canvasId", "$ref": "CanvasId", "description": "Identifier of canvas that changed." }, > + { "name": "clientNodeIds", "type": "array", "items": { "$ref": "DOM.NodeId" }, "description": "Identifier of each node that uses this canvas via -webkit-canvas." } > + ] > } I think this should not include clientNodeIds. It should just say "hey, this Canvas has a dirty list" and the frontend can request that list when it needs it (if the sidebar is visible). This pattern allows the backend to avoid constantly pushing nodes to the frontend that the frontend may never need.

Comment on attachment 314333 [details] [Patch] For bots View in context: https://bugs.webkit.org/attachment.cgi?id=314333&action=review > Source/WebCore/css/CSSCanvasValue.h:115 > + static bool isType(const WebCore::CanvasObserver& canvasObserver) { return canvasObserver.isCSSCanvasValueObserver(); } Shouldn't we name this `isCanvasObserverProxy` instead of `isCSSCanvasValueObserver`? > Source/WebCore/html/HTMLCanvasElement.cpp:50 > #include "RenderHTMLCanvas.h" > +#include "RenderElement.h" Nit: Sort > Source/WebCore/html/HTMLCanvasElement.cpp:196 > + for (auto it = clients.begin(); it != clients.end(); ++it) { Style: Could modernize this for (auto& entry : clients) { if (Element* element = entry.key->element()) ...; } How likely is it that something will be in this list multiple times? Could we just use a Vector instead of a HashSet? > Source/WebCore/inspector/InspectorCanvasAgent.cpp:145 > + result = buildObjectForCSSCanvasClientNodes(*canvasEntry); This should pass errorString on, to be populated by pushNodeToFrontend, if there is an issue. > Source/WebCore/inspector/InspectorCanvasAgent.cpp:182 > + m_frontendDispatcher->cssCanvasClientNodesChanged(canvasEntry->identifier, buildObjectForCSSCanvasClientNodes(*canvasEntry)); And here is where I'm recommending dropping the buildNodes. > Source/WebCore/inspector/InspectorInstrumentation.h:251 > static void didCreateCSSCanvas(HTMLCanvasElement&, const String&); > + static void didChangeCSSCanvasClientNodes(HTMLCanvasElement&); > static void didCreateCanvasRenderingContext(HTMLCanvasElement&); > static void didChangeCanvasMemory(HTMLCanvasElement&); Heh, I think all of these should probably be `const HTMLCanvasElement&` because I don't think we expect to modify them. But that cleanup can be done later. > Source/WebInspectorUI/UserInterface/Models/Canvas.js:183 > + WebInspector.domTreeManager.requestDocument((document) => { Something we've done elsewhere was: WebInspector.domTreeManager.requestDocument(function(){}); We don't need to defer the requestCSSCanvasChildNodes until we get a response for requestDocument. Maybe we should make a simple: WebInspector.domTreeManager.ensureDocument(); > Source/WebInspectorUI/UserInterface/Views/CanvasDetailsSidebarPanel.js:113 > + this._cssCanvasClientsRow = new WebInspector.DetailsSectionSimpleRow(WebInspector.UIString("%s Nodes").format("-webkit-canvas")); I think just "Nodes" may be clear enough given this is in a "CSS" section.

Comment on attachment 314333 [details] [Patch] For bots Attachment 314333 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4030787 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314355 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 314333 [details] [Patch] For bots Attachment 314333 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4030860 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314359 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 314333 [details] [Patch] For bots Attachment 314333 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4032217 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314373 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Created attachment 314508 [details] Patch

<rdar://problem/33111520>

<rdar://problem/33111522>

<rdar://problem/33109079>

Comment on attachment 314508 [details] Patch Attachment 314508 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4046611 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314514 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 314508 [details] Patch Attachment 314508 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4046636 New failing tests: inspector/canvas/css-canvas-clients.html

Created attachment 314519 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 314508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314508&action=review r=me The tests are failing, so you may have to mark them as flakey. We should really investigate that. > Source/JavaScriptCore/inspector/protocol/Canvas.json:79 > + { "name": "canvasId", "$ref": "CanvasId", "description": "Canvas identifier." } Nit: These descriptions are unnecessary. I think we can probably eliminate them for most $ref's. If someone needed this description they could get it by looking at the description of the Type itself. > LayoutTests/inspector/canvas/css-canvas-clients-expected.txt:13 > +PASS: Client node has valid ID. Why not just log the selector? Thats more useful.

Created attachment 314872 [details] Patch

Comment on attachment 314872 [details] Patch Clearing flags on attachment: 314872 Committed r219268: <http://trac.webkit.org/changeset/219268>

All reviewed patches have been landed. Closing bug.