[Webkit-unassigned] [Bug 137278] Web Inspector: add 2D/WebGL canvas instrumentation infrastructure

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=137278


Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #239073|review?                     |review-
               Flag|                            |




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org>  2014-10-01 19:50:34 PST ---
(From update of attachment 239073)
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.CanvasWasAdded, this._canvasWasAdded, this);
> +        WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasRemoved, 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.PageArchiveStarted, this._pageArchiveStarted, this);
> +            WebInspector.notifications.removeEventListener(WebInspector.Notification.PageArchiveEnded, 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.