[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





--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org>  2014-10-01 13:02:02 PST ---
(From update of attachment 239038)
View in context: https://bugs.webkit.org/attachment.cgi?id=239038&action=review

Overall this is a great first patch! Very clean.

Nit: Needs ChangeLogs

> Source/WebCore/DerivedSources.make:1143
> +    $(WebCore)/inspector/protocol/Canvas.json \

Same will have to be done to the CMakeLists.txt, adding the JSON and new file to compile.
Adding the new files to Windows vsproj as well.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:15
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.

This is the wrong license. We have a 2 clause LICENSE now, not a 3 clause.

You can use the one from Source/WebCore/inspector/CommandLineAPIModule.h as an example (updating the year to 2014).

Ditto for other files.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:42
> +#include "InspectorWebFrontendDispatchers.h"

Nit: Unnecessary. This is already in the .h.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:46
> +#include <wtf/Vector.h>

Is Vector used below?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:103
> +void InspectorCanvasAgent::canvasDestroyed(HTMLCanvasElement& canvas)

Does this get called when navigating? To be sure we remove canvases from the previous page?

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

Style: You could make this "auto it", as the type is not really helpful here.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:150
> +    RefPtr<Inspector::Protocol::Canvas::Canvas> result = Inspector::Protocol::Canvas::Canvas::create()
> +        .setCanvasId(idForCanvas(canvas))
> +        .setFrameId(m_pageAgent->frameId(frame))
> +        .setIs2d(renderingContext->is2d())
> +        .setIs3d(renderingContext->is3d());
> +
> +    return result.release();

Style: You can one line this instead of assigning to a temporary RefPtr which may increment the retain count unnecessarily.

    return I:P:C:C::create()
        .setFoo().release();

> Source/WebCore/inspector/InspectorCanvasAgent.h:51
> +class InspectorCanvasAgent

Nit: "final"

> Source/WebCore/inspector/InspectorCanvasAgent.h:58
> +    explicit InspectorCanvasAgent(InstrumentingAgents*, InspectorPageAgent*);

Nit: explicit is not needed with a constructor with more than a single argument. It is only useful on constructors with 1 argument, to prevent implicit cast conversions.

> Source/WebCore/inspector/protocol/Canvas.json:57
> +                { "name": "canvasId", "$ref": "DOM.NodeId", "description": "Id of the canvas that was removed." }

This should be a CanvasId, not a DOM.NodeId.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:33
> +    this._canvasIdMap = {};

This can and probably should be a Map.

    this._canvasIdMap = new Map;

You would have to change the getters/setters/removal/iteration below. But at least then the key is kept an integer instead of strings.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:99
> +        this._canvasIdMap = {};

Just put this in reset() and call reset()?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:33
> +    this._is2d = false;
> +    this._is3d = false;

These should be set to the incoming parameters.

    this._is2d = is2d || false;
    this._is3d = is3d || false;

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:51
> +
> +    constructor: WebInspector.Canvas,

You can put the proto here and remove it from the bottom of the file:

    __proto__: WebInspector.Object.prototype,

See Probe.js as an example of the new, preferred, style.

Ditto for the other files.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:82
> +    get displayName()
> +    {
> +        // Assign a unique number to the script object so it will stay the same.
> +        if (!this._uniqueDisplayNameNumber)
> +            this._uniqueDisplayNameNumber = this.constructor._nextUniqueDisplayNameNumber++;
> +
> +        return WebInspector.UIString("Canvas %d").format(this._uniqueDisplayNameNumber);
> +    },

It may also be nice to have a better name for a canvas if it is unique. E.g. "Canvas Element #foo" for <canvas id="foo">. Or "CSS Canvas 'foo'" for -webkit-canvas(foo).

> Source/WebInspectorUI/UserInterface/Protocol/DebuggerObserver.js:39
> +        WebInspector.canvasManager.reset();

I don't think this is necessary. The MainFrame navigated event should be fine.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:44
> +    WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasAdded, this._canvasWasAdded, this);
> +    WebInspector.canvasManager.addEventListener(WebInspector.CanvasManager.Event.CanvasWasRemoved, this._canvasWasRemoved, this);

This is dangerous, it may be leaking the FrameTreeElement forever because it is adding itself as a strong listener of a singleton.

At some point this will have to be removed as a listener. This will require some thought.

In fact, the WebInspector.notifications code below might also cause a leak for all FrameTreeElements that are MainFrames...

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