[webkit-reviews] review granted: [Bug 172623] Web Inspector: Instrument 2D/WebGL canvas contexts in the backend : [Attachment 312706] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Matt Baker <mattbaker at apple.com>'s
request for review:
Bug 172623: Web Inspector: Instrument 2D/WebGL canvas contexts in the backend
https://bugs.webkit.org/show_bug.cgi?id=172623

Attachment 312706: Patch

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




--- Comment #38 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 312706
  --> https://bugs.webkit.org/attachment.cgi?id=312706
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}")`);