Bug 172623 - Web Inspector: Instrument 2D/WebGL canvas contexts in the backend
Summary: Web Inspector: Instrument 2D/WebGL canvas contexts in the backend
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: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 172624 WebInspectorCanvasRecording WebInspectorCanvasTab 137278 138941 180736 180770
  Show dependency treegraph
 
Reported: 2017-05-25 16:50 PDT by Matt Baker
Modified: 2017-12-13 15:14 PST (History)
9 users (show)

See Also:


Attachments
WIP - failing tests (68.29 KB, patch)
2017-06-06 16:15 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.50 MB, application/zip)
2017-06-06 17:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.62 MB, application/zip)
2017-06-06 17:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (2.07 MB, application/zip)
2017-06-06 17:24 PDT, Build Bot
no flags Details
Patch (69.72 KB, patch)
2017-06-08 18:01 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.17 MB, application/zip)
2017-06-08 19:06 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.92 MB, application/zip)
2017-06-08 19:22 PDT, Build Bot
no flags Details
Patch (69.45 KB, patch)
2017-06-08 20:52 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.10 MB, application/zip)
2017-06-08 21:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.10 MB, application/zip)
2017-06-08 21:58 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.89 MB, application/zip)
2017-06-08 22:16 PDT, Build Bot
no flags Details
Patch (69.45 KB, patch)
2017-06-09 12:50 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (2.05 MB, application/zip)
2017-06-09 14:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-elcapitan (1.24 MB, application/zip)
2017-06-09 15:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.23 MB, application/zip)
2017-06-09 17:58 PDT, Build Bot
no flags Details
Patch (67.33 KB, patch)
2017-06-12 15:14 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.03 MB, application/zip)
2017-06-12 15:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.91 MB, application/zip)
2017-06-12 16:19 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.78 MB, application/zip)
2017-06-12 16:23 PDT, Build Bot
no flags Details
Patch (66.25 KB, patch)
2017-06-14 18:36 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.08 MB, application/zip)
2017-06-14 19:21 PDT, Build Bot
no flags Details
Patch (66.43 KB, patch)
2017-06-15 16:46 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (66.44 KB, patch)
2017-06-15 18:33 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (66.28 KB, patch)
2017-06-16 14:35 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (66.33 KB, patch)
2017-06-16 14:38 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (66.42 KB, patch)
2017-06-16 17:46 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2017-05-25 16:50:24 PDT
Summary:
Instrument 2D/WebGL canvas contexts in the backend.

A basic Canvas protocol should include:

Types:
- Canvas
- ContextType
- ContextAttributes

Commands:
- getCanvases

Events:
- canvasAdded
- canvasRemoved
Comment 1 Radar WebKit Bug Importer 2017-05-25 16:52:10 PDT
<rdar://problem/32415986>
Comment 2 Matt Baker 2017-06-06 16:15:58 PDT
Created attachment 312135 [details]
WIP - failing tests
Comment 3 Build Bot 2017-06-06 16:17:31 PDT
Attachment 312135 [details] did not pass style-queue:


ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:54:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:55:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:67:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:68:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
ERROR: Source/WebCore/inspector/InspectorCanvasAgent.h:69:  "virtual" is redundant since function is already declared as "override"  [readability/inheritance] [4]
Total errors found: 5 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Build Bot 2017-06-06 17:03:21 PDT
Comment on attachment 312135 [details]
WIP - failing tests

Attachment 312135 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3884423

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/canvas/create-canvas-contexts.html
Comment 5 Build Bot 2017-06-06 17:03:22 PDT
Created attachment 312140 [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 6 Build Bot 2017-06-06 17:15:11 PDT
Comment on attachment 312135 [details]
WIP - failing tests

Attachment 312135 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3884454

New failing tests:
inspector/dom/highlightSelector.html
inspector/dom/highlightQuad.html
inspector/canvas/create-canvas-contexts.html
Comment 7 Build Bot 2017-06-06 17:15:12 PDT
Created attachment 312142 [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 8 Build Bot 2017-06-06 17:24:41 PDT
Comment on attachment 312135 [details]
WIP - failing tests

Attachment 312135 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3884432

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightSelector.html
inspector/dom/highlightQuad.html
inspector/canvas/create-canvas-contexts.html
inspector/dom/highlightRect.html
Comment 9 Build Bot 2017-06-06 17:24:42 PDT
Created attachment 312143 [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 10 Matt Baker 2017-06-08 18:01:04 PDT
Created attachment 312365 [details]
Patch
Comment 11 Build Bot 2017-06-08 19:06:40 PDT
Comment on attachment 312365 [details]
Patch

Attachment 312365 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3897053

New failing tests:
inspector/canvas/create-canvas-contexts.html
Comment 12 Build Bot 2017-06-08 19:06:41 PDT
Created attachment 312370 [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 13 Build Bot 2017-06-08 19:22:16 PDT
Comment on attachment 312365 [details]
Patch

Attachment 312365 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3897057

New failing tests:
inspector/dom/highlightRect.html
Comment 14 Build Bot 2017-06-08 19:22:18 PDT
Created attachment 312372 [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
Comment 15 Matt Baker 2017-06-08 20:52:07 PDT
Created attachment 312375 [details]
Patch
Comment 16 Matt Baker 2017-06-08 20:58:35 PDT
Note: context attributes were removed to reduce the complexity of this patch
Comment 17 Build Bot 2017-06-08 21:54:21 PDT
Comment on attachment 312375 [details]
Patch

Attachment 312375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3897634

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/canvas/create-canvas-contexts.html
inspector/dom/highlightRect.html
Comment 18 Build Bot 2017-06-08 21:54:22 PDT
Created attachment 312376 [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 19 Build Bot 2017-06-08 21:58:28 PDT
Comment on attachment 312375 [details]
Patch

Attachment 312375 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3897637

New failing tests:
inspector/dom/highlightSelector.html
Comment 20 Build Bot 2017-06-08 21:58:29 PDT
Created attachment 312378 [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 21 Build Bot 2017-06-08 22:16:50 PDT
Comment on attachment 312375 [details]
Patch

Attachment 312375 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3897653

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightQuad.html
Comment 22 Build Bot 2017-06-08 22:16:51 PDT
Created attachment 312380 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 23 Matt Baker 2017-06-09 11:07:50 PDT
Fixing. These are not false positives:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000011251a1ee WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 222 (HTMLCanvasElement.cpp:138)
1   com.apple.WebCore             	0x000000011251a995 WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 21 (HTMLCanvasElement.cpp:143)
2   com.apple.WebCore             	0x000000011251a9b9 WebCore::HTMLCanvasElement::~HTMLCanvasElement() + 25 (HTMLCanvasElement.cpp:136)
3   com.apple.WebCore             	0x0000000111be7ea8 WebCore::removeDetachedChildrenInContainer(WebCore::ContainerNode&) + 264 (ContainerNodeAlgorithms.cpp:202)
4   com.apple.WebCore             	0x0000000111bd4c4e WebCore::ContainerNode::removeDetachedChildren() + 110 (ContainerNode.cpp:105)
5   com.apple.WebCore             	0x0000000111bd5620 WebCore::ContainerNode::~ContainerNode() + 96 (ContainerNode.cpp:163)
...
Comment 24 Matt Baker 2017-06-09 12:50:34 PDT
Created attachment 312475 [details]
Patch
Comment 25 Build Bot 2017-06-09 14:17:49 PDT
Comment on attachment 312475 [details]
Patch

Attachment 312475 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3902356

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightQuad.html
Comment 26 Build Bot 2017-06-09 14:17:50 PDT
Created attachment 312489 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 27 Devin Rousso 2017-06-09 14:33:54 PDT
Comment on attachment 312475 [details]
Patch

Overall, this looks good to me.  Not really sure why the tests keep failing, but I am also kinda new to working with the inspector backend.

View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1224
> +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context)
> +{
> +    if (!context)
> +        return nullptr;
> +
> +    HTMLCanvasElement& canvasElement = context->canvas();
> +    return instrumentingAgentsForDocument(canvasElement.document());
> +}
> +

Is this ever used?  Is it necessary for something that I am unaware of?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:56
> +    canvasesForFrame(frame)
> +    {
> +        let canvases = [];
> +        for (let canvas of this._canvasIdentifierMap.values()) {
> +            if (canvas.parentFrame.id === frame.id)
> +                canvases.push(canvas);
> +        }
> +
> +        return canvases;
> +    }

FYI I removed this in my followup patches, but I think it's fine to leave for now.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
> +WebInspector.Canvas.ContextType = {
> +    Canvas2D: "canvas-context-type-canvas-2d",
> +    WebGL: "canvas-context-type-webgl",
> +};

I think that adding the prefix to these values is unnecessary.

WebInspector.Canvas.ContextType = {
    Canvas2D: "canvas-2d",
    WebGL: "webgl",
};

> LayoutTests/inspector/canvas/create-canvas-contexts.html:6
> +let contexts = [];

I personally like having values that should always be accessible be attached to window.  I can go either way with this, though.

window.contexts = [];

> LayoutTests/inspector/canvas/create-canvas-contexts.html:35
> +    if (window.GCController)

I think you can get rid of the if check here, as the test might function differently without GCController.  I'd rather get an error saying "GCController.collect is undefined" instead of wondering why the test is failing due to a lack of timely GC.

> LayoutTests/inspector/canvas/create-canvas-contexts.html:84
> +                resolve();

NIT: you can move this to be another `.then`

.then(resolve, reject);

> LayoutTests/inspector/canvas/create-canvas-contexts.html:93
> +            InspectorTest.evaluateInPage("createCanvas('2d')");

I think that using template strings makes reading these easier, especially when using evaluateInPage.

InspectorTest.evaluateInPage(`createCanvas("2d")`);
Comment 28 Joseph Pecoraro 2017-06-09 14:53:02 PDT
Comment on attachment 312475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review

Nice tests!

> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
> +                { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },

The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55
> +#include "InspectorInstrumentation.h"

Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:70
> +    if (!m_frontendDispatcher)
> +        return;

We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr.

> Source/WebCore/inspector/InspectorCanvasAgent.h:34
> +#include <wtf/HashSet.h>

Unused.

> Source/WebCore/inspector/InspectorCanvasAgent.h:45
> +typedef String ErrorString;

This shouldn't be needed here, an include should handle it.

> Source/WebCore/inspector/InspectorCanvasAgent.h:66
> +    // CanvasObserver implementation

We can drop the "implementation" here.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:996
> +void InspectorInstrumentation::didCreateCSSCanvasImpl(InstrumentingAgents* instrumentingAgents, HTMLCanvasElement& canvasElement, const String& name)

Style: Lets put this above InspectorInstrumentation::networkStateChangedImpl

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1216
> +InstrumentingAgents* InspectorInstrumentation::instrumentingAgentsForRenderingContext(WebGLRenderingContextBase* context)

Unused in this patch.

> Source/WebCore/inspector/InspectorInstrumentation.h:248
> +    static void didCreateCSSCanvas(HTMLCanvasElement&, const String&);

Style: Lets try to keep the order of functions consistent. Here these are above "networkStateChanged" so lets keep that order.

> Source/WebCore/inspector/InspectorInstrumentation.h:423
> +    static void didCreateCSSCanvasImpl(InstrumentingAgents*, HTMLCanvasElement&, const String&);

Style: Move these above networkStateChangedImpl

> Source/WebCore/inspector/InspectorInstrumentation.h:1097
> +inline void InspectorInstrumentation::didCreateCSSCanvas(HTMLCanvasElement& canvasElement, const String& name)

Style: Move these above InspectorInstrumentation::networkStateChanged

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51
> +            if (canvas.parentFrame.id === frame.id)

Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:102
> +    CanvasWasRemoved: "canvas-manager-canvas-was-removed"

Style: Include the trailing comma to make future changes nicer.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:50
> +            case CanvasAgent.ContextType.Canvas2D:

Style: dedent cases.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:67
> +            case WebInspector.Canvas.ContextType.Canvas2D:

Style: dedent cases.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
> +WebInspector.Canvas.ContextType = {
> +    Canvas2D: "canvas-context-type-canvas-2d",
> +    WebGL: "canvas-context-type-webgl",
> +};

Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging.

> LayoutTests/inspector/canvas/create-canvas-contexts-expected.txt:9
> +Canvas added.

The others all say "Added canvas" this one feels weird.

> LayoutTests/inspector/canvas/create-canvas-contexts.html:9
> +if (window.testRunner)
> +    testRunner.overridePreference("WebKitWebGLEnabled", "1");

I do not think this is necessary. It looks like all ports default this as enabled.

> LayoutTests/inspector/canvas/create-canvas-contexts.html:158
> +                    if (!canvas) {
> +                        reject();
> +                        return;
> +                    }

Doesn't look like this can happen. Probably would have gone to the catch path anyways.
Comment 29 Build Bot 2017-06-09 15:22:56 PDT
Comment on attachment 312475 [details]
Patch

Attachment 312475 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3902788

New failing tests:
inspector/dom/highlightSelector.html
inspector/dom/highlightFrame.html
inspector/canvas/create-canvas-contexts.html
Comment 30 Build Bot 2017-06-09 15:22:58 PDT
Created attachment 312500 [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 31 Matt Baker 2017-06-09 17:23:08 PDT
Comment on attachment 312475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review

>> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
>> +                { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },
> 
> The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.

Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend.

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:55
>> +#include "InspectorInstrumentation.h"
> 
> Unused. In fact all of the WebGLRenderingContextBase includes also seem unused. Seems this was intended for a later change?

Removed, but I'm not sure what "all of the WebGLRenderingContextBase" includes means.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:70
>> +        return;
> 
> We shouldn't need to do this check (everywhere in this file). FrontendDispatcher will always be alive in our unique_ptr.

It looks like InspectorDOMAgent::focusNode() is the only other place making this check. Is it also unnecessary?

>> Source/WebCore/inspector/InspectorCanvasAgent.h:45
>> +typedef String ErrorString;
> 
> This shouldn't be needed here, an include should handle it.

I don't think ErrorString is included from anywhere. Every agent defines ErrorString this way in it's header.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1224
>> +
> 
> Is this ever used?  Is it necessary for something that I am unaware of?

Leftover from before splitting up the patch. Good eye.

>> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:51
>> +            if (canvas.parentFrame.id === frame.id)
> 
> Nit: Can we just compare `canvas.parentFrame === frame` instead of getting the ids?

Yes, but I'm going to remove this per Devin's comment.

>>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:119
>>> +};
>> 
>> I think that adding the prefix to these values is unnecessary.
>> 
>> WebInspector.Canvas.ContextType = {
>>     Canvas2D: "canvas-2d",
>>     WebGL: "webgl",
>> };
> 
> Style: Might consider making these Symbols or simpler names. I've been moving these enum types (when not events) to simple strings, like just "canvas-2d" and "webgl". It makes it much easier when debugging.

I like shortening them. I'm (irrationally) paranoid about collisions with other enums, so I'll go with Symbols.

>> LayoutTests/inspector/canvas/create-canvas-contexts.html:9
>> +    testRunner.overridePreference("WebKitWebGLEnabled", "1");
> 
> I do not think this is necessary. It looks like all ports default this as enabled.

Nice.

>> LayoutTests/inspector/canvas/create-canvas-contexts.html:93
>> +            InspectorTest.evaluateInPage("createCanvas('2d')");
> 
> I think that using template strings makes reading these easier, especially when using evaluateInPage.
> 
> InspectorTest.evaluateInPage(`createCanvas("2d")`);

Joe has been doing this too, since it makes adding template literals easier in the future. Will adopt the pattern (at least for all evaluateInPage).
Comment 32 Matt Baker 2017-06-09 17:40:47 PDT
Comment on attachment 312475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312475&action=review

>>> Source/JavaScriptCore/inspector/protocol/Canvas.json:24
>>> +                { "name": "name", "type": "string", "description": "The CSS canvas identifier, or the canvas element id attribute." },
>> 
>> The canvas element id attribute? This overloading feels weird how about just making it "cssName" and optional? Seems like this could make make the `isCSSCanvas` unnecessary.
> 
> Will remove the overload. The element ID was only used for building a display name in the frontend (Canvas #my-canvas"). Adding a DOM.NodeId property to Canvas would allow it to be retrieved in the frontend.

Also removing `isCSSCanvas`. Replacing with an optional property `cssCanvasName`. Adding optional property `nodeId` as well.
Comment 33 Build Bot 2017-06-09 17:58:00 PDT
Comment on attachment 312475 [details]
Patch

Attachment 312475 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3903674

New failing tests:
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightQuad.html
Comment 34 Build Bot 2017-06-09 17:58:01 PDT
Created attachment 312519 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 35 Matt Baker 2017-06-12 15:14:28 PDT
Created attachment 312706 [details]
Patch
Comment 36 Build Bot 2017-06-12 15:54:43 PDT
Comment on attachment 312706 [details]
Patch

Attachment 312706 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3919663

Number of test failures exceeded the failure limit.
Comment 37 Build Bot 2017-06-12 15:54:45 PDT
Created attachment 312714 [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 38 Devin Rousso 2017-06-12 16:07:09 PDT
Comment on attachment 312706 [details]
Patch

r=me with the one issue on InspectorCanvasAgent.cpp:222.

View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:251
> +        InspectorInstrumentation::didCreateCanvasRenderingContext(*this);

NIT: Does this need to be called after `invalidateStyleAndLayerComposition()`?  I am not sure of the process here, but it seems like that is an important step (it always occurs for WebGL contexts).

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:222
> +        if (!nodeId) {
> +            ErrorString ignored;
> +            nodeId = domAgent->pushNodeToFrontend(ignored, domAgent->boundNodeId(&canvasElement.document()), &canvasElement);
> +        }
> +        canvas->setNodeId(nodeId);

I think there is an edge-case that might cause this code to behave badly.  When I was testing, I found that calling InspectorCanvasAgent::enable on a page with active <canvas> contexts would cause this code to run before the main <document> is sent to the frontend (via `WebInspector.domTreeManager.requestDocument`, which calls `DOMAgent.getDocument`).  Since "nodeId" is already optional, I think we shouldn't even set a value in the case that the <document> hasn't already been sent to the frontend.

        int nodeId = domAgent->boundNodeId(&canvasElement);
        if (!nodeId) {
            if (int documentNodeId = domAgent->boundNodeId(&canvasElement.document())) {
                ErrorString ignored;
                nodeId = domAgent->pushNodeToFrontend(ignored, documentNodeId, &canvasElement);
            }
        }
        
        if (nodeId)
            canvas->setNodeId(nodeId);

> Source/WebCore/inspector/InspectorInstrumentation.h:1095
> +    

Style: remove spaces.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:34
> +        console.assert(frame);

Is there a reason to not assert that the frame is a WebInspector.Frame?

console.assert(frame instanceof WebInspector.Frame);

> LayoutTests/inspector/canvas/create-canvas-contexts.html:86
> +            InspectorTest.evaluateInPage(`createCanvas('2d')`);

Style: should use " instead of '

InspectorTest.evaluateInPage(`createCanvas("2d")`);

> LayoutTests/inspector/canvas/create-canvas-contexts.html:117
> +            expression: "createCanvas('2d')",

Ditto.

expression: `createCanvas("2d")`,

> LayoutTests/inspector/canvas/create-canvas-contexts.html:122
> +            expression: "createCanvas('webgl')",

Ditto.

expression: `createCanvas("webgl")`,

> LayoutTests/inspector/canvas/create-canvas-contexts.html:127
> +            expression: "createOffscreenCanvas('2d')",

Ditto.

expression: `createOffscreenCanvas("2d")`,

> LayoutTests/inspector/canvas/create-canvas-contexts.html:132
> +            expression: "createOffscreenCanvas('webgl')",

Ditto.

expression: `createOffscreenCanvas("webgl")`,

> LayoutTests/inspector/canvas/create-canvas-contexts.html:155
> +                InspectorTest.evaluateInPage(`createCSSCanvas('${contextId}', '${cssCanvasIdentifier}')`);

Ditto.

InspectorTest.evaluateInPage(`createCSSCanvas("${contextId}", "${cssCanvasIdentifier}")`);
Comment 39 Build Bot 2017-06-12 16:19:21 PDT
Comment on attachment 312706 [details]
Patch

Attachment 312706 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3919765

Number of test failures exceeded the failure limit.
Comment 40 Build Bot 2017-06-12 16:19:22 PDT
Created attachment 312718 [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 41 Build Bot 2017-06-12 16:23:12 PDT
Comment on attachment 312706 [details]
Patch

Attachment 312706 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3919679

Number of test failures exceeded the failure limit.
Comment 42 Build Bot 2017-06-12 16:23:14 PDT
Created attachment 312719 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 43 Devin Rousso 2017-06-12 19:03:26 PDT
Comment on attachment 312706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312706&action=review

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:110
> +        cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path || "";

This causes an error.  It should be ":", not "||".

cookie[WebInspector.Canvas.NodePathCookieKey] = this._domNode ? this._domNode.path : "";
Comment 44 Matt Baker 2017-06-14 18:36:58 PDT
Created attachment 312943 [details]
Patch
Comment 45 Build Bot 2017-06-14 19:21:40 PDT
Comment on attachment 312943 [details]
Patch

Attachment 312943 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3932764

New failing tests:
inspector/canvas/create-canvas-contexts.html
Comment 46 Build Bot 2017-06-14 19:21:42 PDT
Created attachment 312945 [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 47 Matt Baker 2017-06-15 16:46:19 PDT
Created attachment 313025 [details]
Patch
Comment 48 Matt Baker 2017-06-15 18:33:03 PDT
Created attachment 313041 [details]
Patch
Comment 49 Devin Rousso 2017-06-15 18:56:51 PDT
Comment on attachment 313041 [details]
Patch

r=me

View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:111
> +    if (!m_enabled) {
> +        m_canvasEntries.clear();
> +        return;
> +    }
> +
> +    for (auto* canvasElement : canvasesForFrame) {
> +        auto canvasEntry = m_canvasEntries.take(canvasElement);
> +        m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
> +    }

I don't think we want to do this.  We should only be clearing the canvases for the frame that navigated:

    for (auto* canvasElement : canvasesForFrame) {
        auto canvasEntry = m_canvasEntries.take(canvasElement);
        if (m_enabled)
            m_frontendDispatcher->canvasRemoved(canvasEntry.identifier);
    }

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
> +        newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.get(&canvasElement);
> +        m_canvasToCSSCanvasId.remove(&canvasElement);

I think you can just use `take` here like above:

    if (m_canvasToCSSCanvasId.contains(&canvasElement))
        newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
> +        m_frontendDispatcher->canvasRemoved(identifier);

I think that it might be possible for this to run even after `disable()` has been called.  I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
Comment 50 Joseph Pecoraro 2017-06-15 19:32:24 PDT
Comment on attachment 313041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:90
> +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader)

Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:132
>> +        m_canvasToCSSCanvasId.remove(&canvasElement);
> 
> I think you can just use `take` here like above:
> 
>     if (m_canvasToCSSCanvasId.contains(&canvasElement))
>         newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);

Looks like this could just be:

    newCanvasEntry.cssCanvasName = m_canvasToCSSCanvasId.take(&canvasElement);
Comment 51 WebKit Commit Bot 2017-06-15 19:55:41 PDT
Comment on attachment 313041 [details]
Patch

Clearing flags on attachment: 313041

Committed r218376: <http://trac.webkit.org/changeset/218376>
Comment 52 WebKit Commit Bot 2017-06-15 19:55:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 54 Matt Lewis 2017-06-16 09:47:26 PDT
Reverted r218376 for reason:

The patch cause multiple Layout Test Crashes.

Committed r218395: <http://trac.webkit.org/changeset/218395>
Comment 55 Joseph Pecoraro 2017-06-16 13:52:03 PDT
Comment on attachment 313041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150
> +    canvasElement.removeObserver(*this);

HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers:

    for (auto& observer : m_observers)
        observer->canvasDestroyed(*this);

If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over.

I suspect this is what the ASSERT is catching and causing crashes.
Comment 56 Matt Baker 2017-06-16 14:23:37 PDT
Comment on attachment 313041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:90
>> +void InspectorCanvasAgent::frameNavigated(DocumentLoader* loader)
> 
> Why doesn't this just take a Frame& / Frame*? This only uses the DocumentLoader to access the Frame.

I think this was left over from an older implementation. Other agents that define frameNavigated take a Frame&. Will fix.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150
>> +    canvasElement.removeObserver(*this);
> 
> HTMLCanvasElement triggers canvasDestroyed in its destructor while iterating its observers:
> 
>     for (auto& observer : m_observers)
>         observer->canvasDestroyed(*this);
> 
> If CanvasAgent::canvasDestroyed then tries to remove itself as an observer here then we will be mutating the HashMap that we are iterating over.
> 
> I suspect this is what the ASSERT is catching and causing crashes.

It's safe for CanvasObservers to assume that the observer is removed as a side effect of canvasDestroyed. Will the call to removeObserver in InspectorCanvasAgent::canvasDestroyed.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
>> +        m_frontendDispatcher->canvasRemoved(identifier);
> 
> I think that it might be possible for this to run even after `disable()` has been called.  I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.

Good catch. Stopping the timer in `disable()` feels cleaner. Will fix.
Comment 57 Devin Rousso 2017-06-16 14:26:37 PDT
Comment on attachment 313041 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313041&action=review

>>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:173
>>> +        m_frontendDispatcher->canvasRemoved(identifier);
>> 
>> I think that it might be possible for this to run even after `disable()` has been called.  I think that either this should be guarded by a `m_enabled` check or `m_timer.stop()` should be called in `disable()`.
> 
> Good catch. Stopping the timer in `disable()` feels cleaner. Will fix.

If you go that route, you're also going to have to make sure to call `m_removedCanvasIdentifiers.clear();` so that if the agent is enabled again later on there aren't any leftover removed canvases that dispatch events.
Comment 58 Matt Baker 2017-06-16 14:35:17 PDT
Created attachment 313138 [details]
Patch
Comment 59 Matt Baker 2017-06-16 14:38:42 PDT
Created attachment 313140 [details]
Patch
Comment 60 WebKit Commit Bot 2017-06-16 17:33:43 PDT
Comment on attachment 313140 [details]
Patch

Rejecting attachment 313140 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 313140, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 00c58440e2558cd462d45456051fbb940ff4883b df50ec9c69de9742bac5f1e63c6813cd5d3d421b M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3944929
Comment 61 Matt Baker 2017-06-16 17:46:13 PDT
Created attachment 313168 [details]
Patch for landing
Comment 62 WebKit Commit Bot 2017-06-16 19:58:28 PDT
Comment on attachment 313168 [details]
Patch for landing

Clearing flags on attachment: 313168

Committed r218440: <http://trac.webkit.org/changeset/218440>
Comment 63 WebKit Commit Bot 2017-06-16 19:58:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Frédéric Wang (:fredw) 2017-06-19 07:04:29 PDT
@Matt: inspector/canvas/create-canvas-contexts.html seems flacky on the mac bot (timeout).