[webkit-reviews] review denied: [Bug 137278] Web Inspector: add 2D/WebGL canvas instrumentation infrastructure : [Attachment 239073] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 137278: Web Inspector: add 2D/WebGL canvas instrumentation infrastructure
https://bugs.webkit.org/show_bug.cgi?id=137278

Attachment 239073: Patch
https://bugs.webkit.org/attachment.cgi?id=239073&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=239073&action=review


Looking good, some more substantial comments now. Like Tim said, you'll need to
rebase and tweak to get the bots building with some of the recent changes.

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

This is covered by idForCanvas in the next statement. So you can remove it
here. Also, we always expect a canvas, so maybe we should convert to
HTMLCanvasElement references, or ASSERT(canvas). It would be nice to move to
references.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:115
> +    HashMap<int, const HTMLCanvasElement*>::const_iterator it =
m_idToCanvas.find(id);

Nit: Could be auto here as well.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:135
> +PassRefPtr<Inspector::Protocol::Canvas::Canvas>
InspectorCanvasAgent::buildObjectForCanvas(const HTMLCanvasElement* canvas)
> +{
> +    if (!canvas)
> +	   return nullptr;

Another place where switching to references we can avoid an unnecessary null
check.

> Source/WebCore/inspector/InspectorInstrumentation.h:277
> +    static void didCreateRenderingContextForCanvas(Document*,
HTMLCanvasElement*);

If we move to references, this would be the starting point. HTMLCanvasElement&
instead of HTMLCanvasElement*.

> Source/WebCore/inspector/protocol/Canvas.json:17
> +	       "id": "ShaderType",
> +	       "type": "string",
> +	       "enum": ["FragmentShader", "VertexShader"],
> +	       "description": "Shader type. WebGL 1.0 supports VERTEX_SHADER
and FRAGMENT_SHADER types."

Is the word Shader in the enum redundant? Could it just be "Fragment" and
"Vertex"?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:30
> +WebInspector.Canvas = function(id, parentFrame, is2d, is3d)
> +{
> +    WebInspector.Object.call(this);
> +
> +    this._id = id || null;

Nit: We should assert we have an id and a parentFrame, those we always expect.

    console.assert(id);
    console.assert(parentFrame);

> Source/WebInspectorUI/UserInterface/Models/Shader.js:30
> +    this._id = id || null;

Same thing here, the "id || null" seems sketchy because they are different
types. A null id would likely cause problems.

> Source/WebInspectorUI/UserInterface/Models/Shader.js:49
> +	   return this._documentNodeIdentifier + ":" + this._name;

This is wrong. Should just be:

    return this._id;

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:111
> +	  
WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.Ca
nvasWasAdded, this._canvasWasAdded, this);
> +	  
WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.Ca
nvasWasRemoved, this._canvasWasRemoved, this);

Given that when we attach we start listening for NEW canvases. Then at some
point we need to get the list of CURRENT canvases.

One place you could do that is in FrameTreeElement.prototype.onpopulate. It
look up the current list of canvases for this frame. I'd expect something like:


    var canvases = WebInspector.canvasManager.canvases;
    for (var canvas of canvases) {
	if (canvas.parentFrame === this)
	    this._addTreeElementForRepresentedObject(canvas);
    }

Perhaps it will always be the case that this is empty. But maybe not (I think
reshuffling of Resources elements into folders might do this).

Test case would be:
1. A page with a <canvas>
2. Load 1 resource a second until resources get sorted into folders
  => does the Canvas still show up or not?

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:129
> +	   if (this._frame.isMainFrame()) {
> +	      
WebInspector.notifications.removeEventListener(WebInspector.Notification.PageAr
chiveStarted, this._pageArchiveStarted, this);
> +	      
WebInspector.notifications.removeEventListener(WebInspector.Notification.PageAr
chiveEnded, this._pageArchiveEnded, this);
> +	   }

Nit: Should also reset state. Either here or in onattach.

    this._downloadingPage = false;

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:301
> +    _canvasWasAdded: function(event)
> +    {
> +	   this._addRepresentedObjectToNewChildQueue(event.data.canvas);
> +    },
> +
> +    _canvasWasRemoved: function(event)
> +    {
> +	   this._removeChildForRepresentedObject(event.data.canvas);
> +    },

These need to check that the canvas is part of this frame. For example:

    var canvas = event.data.canvas;
    if (canvas.parentFrame === this)
	this._addRepresentedObjectToNewChildQueue(canvas);
   
A test case would be a page with a <canvas> in an <iframe>. Seems right now the
canvas would show up in both the iframe and main frame.