Bug 173965 - Web Inspector: Show all elements currently using a given CSS Canvas
Summary: Web Inspector: Show all elements currently using a given CSS Canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 138941
Blocks: 174272 174279
  Show dependency treegraph
 
Reported: 2017-06-29 00:57 PDT by Devin Rousso
Modified: 2017-07-07 14:42 PDT (History)
7 users (show)

See Also:


Attachments
[Patch] WIP (2.33 KB, patch)
2017-06-29 01:06 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Patch] WIP (26.91 KB, patch)
2017-06-29 01:07 PDT, Devin Rousso
drousso: review-
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] After Patch is applied (363.64 KB, image/png)
2017-06-29 01:10 PDT, Devin Rousso
no flags Details
Patch (39.32 KB, patch)
2017-06-29 18:38 PDT, Devin Rousso
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (979.39 KB, application/zip)
2017-06-29 19:38 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.75 MB, application/zip)
2017-06-29 20:05 PDT, Build Bot
no flags Details
[Patch] For bots (39.61 KB, patch)
2017-06-30 17:29 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.09 MB, application/zip)
2017-06-30 19:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (1.89 MB, application/zip)
2017-06-30 20:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.11 MB, application/zip)
2017-06-30 23:38 PDT, Build Bot
no flags Details
Patch (42.41 KB, patch)
2017-07-03 13:00 PDT, Devin Rousso
joepeck: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1020.69 KB, application/zip)
2017-07-03 14:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.74 MB, application/zip)
2017-07-03 14:27 PDT, Build Bot
no flags Details
Patch (41.78 KB, patch)
2017-07-07 13:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-06-29 00:57:39 PDT
Since the underlying canvas element of a -webkit-canvas context is not in the DOM, it is hard to see what nodes are affected by it.  It should be possible for the user to see the list of nodes actively using the canvas.
Comment 1 Devin Rousso 2017-06-29 01:06:38 PDT
Created attachment 314125 [details]
[Patch] WIP
Comment 2 Devin Rousso 2017-06-29 01:07:46 PDT
Created attachment 314126 [details]
[Patch] WIP

Oops.  Wrong patch :P
Comment 3 Devin Rousso 2017-06-29 01:10:01 PDT
Created attachment 314127 [details]
[Image] After Patch is applied
Comment 4 Devin Rousso 2017-06-29 18:38:00 PDT
Created attachment 314201 [details]
Patch
Comment 5 Build Bot 2017-06-29 18:41:14 PDT
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 6 Build Bot 2017-06-29 19:38:29 PDT
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
Comment 7 Build Bot 2017-06-29 19:38:30 PDT
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 8 Build Bot 2017-06-29 20:05:52 PDT
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
Comment 9 Build Bot 2017-06-29 20:05:53 PDT
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
Comment 10 Devin Rousso 2017-06-30 17:29:52 PDT
Created attachment 314333 [details]
[Patch] For bots
Comment 11 Build Bot 2017-06-30 17:33:00 PDT
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 12 Joseph Pecoraro 2017-06-30 18:34:39 PDT
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 13 Joseph Pecoraro 2017-06-30 18:55:05 PDT
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 14 Build Bot 2017-06-30 19:56:03 PDT
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
Comment 15 Build Bot 2017-06-30 19:56:04 PDT
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 16 Build Bot 2017-06-30 20:18:01 PDT
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
Comment 17 Build Bot 2017-06-30 20:18:02 PDT
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 18 Build Bot 2017-06-30 23:38:41 PDT
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
Comment 19 Build Bot 2017-06-30 23:38:43 PDT
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
Comment 20 Devin Rousso 2017-07-03 13:00:30 PDT
Created attachment 314508 [details]
Patch
Comment 21 Radar WebKit Bug Importer 2017-07-03 13:00:55 PDT
<rdar://problem/33111520>
Comment 22 Radar WebKit Bug Importer 2017-07-03 13:00:57 PDT
<rdar://problem/33111522>
Comment 23 Brian Burg 2017-07-03 13:55:40 PDT
<rdar://problem/33109079>
Comment 24 Build Bot 2017-07-03 14:02:43 PDT
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
Comment 25 Build Bot 2017-07-03 14:02:45 PDT
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 26 Build Bot 2017-07-03 14:27:21 PDT
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
Comment 27 Build Bot 2017-07-03 14:27:23 PDT
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 28 Joseph Pecoraro 2017-07-06 21:14:17 PDT
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.
Comment 29 Devin Rousso 2017-07-07 13:53:16 PDT
Created attachment 314872 [details]
Patch
Comment 30 WebKit Commit Bot 2017-07-07 14:30:50 PDT
Comment on attachment 314872 [details]
Patch

Clearing flags on attachment: 314872

Committed r219268: <http://trac.webkit.org/changeset/219268>
Comment 31 WebKit Commit Bot 2017-07-07 14:30:52 PDT
All reviewed patches have been landed.  Closing bug.