Bug 137278 – Web Inspector: add 2D/WebGL canvas instrumentation infrastructure

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Implement canvas tooling in Inspector frontend and backend. Initial patches should allow inspection of WebGL canvas resources (shaders and textures).

[Radar WebKit Bug Importer]

<rdar://problem/18509072>

[Matt Baker]

Created attachment 239038 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 239038 [details]
Patch

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...

[Matt Baker]

Created attachment 239073 [details]
Patch

[Timothy Hatcher]

Comment on attachment 239073 [details]
Patch

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

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:84
> +void InspectorCanvasAgent::enable(ErrorString*)

ErrorString& now after Brian's change. Will need to rebase.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:77
> +        // Assign a unique number to the script object so it will stay the same.

Canvas not script.

[Joseph Pecoraro]

Comment on attachment 239073 [details]
Patch

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.

[Matt Baker]

Created attachment 239374 [details]
Patch

[Matt Baker]

Created attachment 239375 [details]
Patch

[Timothy Hatcher]

Comment on attachment 239375 [details]
Patch

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

Looking real good. Some more tweaks and it will be ready!

> Source/WebCore/html/HTMLCanvasElement.h:36
> +#include <wtf/HashSet.h>

Not needed now?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:49
> +    canvasesForFrame: function(frameId, callback)

Spell out frameIdentifier.

This can use promises. The callback system is on its way out. It should clean up this function a lot!

See: http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#UnderstandingandUsingPromises

Some examples are DebuggerManager.pause and ReplayManager.getSession.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:43
> +WebInspector.CanvasTreeElement.prototype = {
> +    constructor: WebInspector.CanvasTreeElement
> +};
> +
> +WebInspector.CanvasTreeElement.prototype.__proto__ = WebInspector.GeneralTreeElement.prototype;

__proto__ should move into the block next to constructor property.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:208
> +            for (var i = 0; i < canvases.length; ++i)
> +                this._addTreeElementForRepresentedObject(canvases[i]);

Should do:
for (var canvas of canvases)
    this._addTreeElementForRepresentedObject(canvas);

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:564
> +        if (representedObject instanceof WebInspector.ShaderProgram) {
> +            if (!this._shadersFolderTreeElement)
> +                this._shadersFolderTreeElement = createFolderTreeElement.call(this, "shaders", WebInspector.UIString("Shaders"));
> +            return this._shadersFolderTreeElement;
> +        }

Sounds like this will change, since Shaders will go under the owning Canvas.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:41
> +WebInspector.ShaderProgramTreeElement.prototype = {
> +    constructor: WebInspector.ShaderProgramTreeElement
> +};
> +
> +WebInspector.ShaderProgramTreeElement.prototype.__proto__ = WebInspector.GeneralTreeElement.prototype;

Ditto re: __proto__.

[Joseph Pecoraro]

Comment on attachment 239375 [details]
Patch

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

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:62
> +void InspectorCanvasAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason)

Seems this should call disable.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:72
> +    int id = idForCanvas(canvas);
> +    if (id)
> +        return;

Should this ASSERT(!id)? didCreate shouldn't happen for an existing canvas, right?

[Timothy Hatcher]

Comment on attachment 239375 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:62
>> +void InspectorCanvasAgent::willDestroyFrontendAndBackend(InspectorDisconnectReason)
> 
> Seems this should call disable.

We can probably just remove enable and disable in the agent! The places that use m_enabled already also null check m_frontendDispatcher.

Down with enable!

[Matt Baker]

Created attachment 241326 [details]
Patch

[Brian Burg]

Comment on attachment 241326 [details]
Patch

This patch doesn't seem to build. I'll have more comments after lunch.

[Matt Baker]

Created attachment 241372 [details]
Patch

[Brian Burg]

Comment on attachment 241372 [details]
Patch

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

Overall it's much better than last time I looked at this patch. There are a few style nits in the canvas agent.
I think the backend is almost ready to land. I haven't looked at the frontend part of this patch yet.

My main concerns (r- for missing tests):

* you should write some basic tests for the canvas protocol domain. It should hit all major code paths: add/remove canvas, shaders, programs; ensure proper reset across main frame navigations; ensure that graphics context<-->inspector agent association works correctly (by using nested iframes). This is really only like inspector-protocol 5 tests at most. You can pattern the test structure off the ones in LayoutTests/inspector-protocol/debugger/. You may have to reload the test page or dynamically append the canvas in order to reliably receive context creation events, though.
* i would prefer that the frontend side of this patch was split off from backend things, so that it requires less domain knowledge for each half to get reviewed. And, less thrashing on either side if you have to roll out a commit. You can use a local git branch to continue making progress on dependent patches.

> Source/WebCore/ChangeLog:10
> +

I would say a little something about what events are now instrumented, and what new data sources are available to the frontend (just shader sources, I think?)

Also, the instrumentation strategy for canvas DOM elements is a little unusual compared to other agents so I would call that out here.

> Source/WebInspectorUI/ChangeLog:10
> +        view (this may change in the future), however shader programs (and eventually textures) belongiong to the

typo: belonging

> Source/JavaScriptCore/inspector/protocol/Canvas.json:3
> +    "description": "Canvas and WebGL stuff.",

"Supports inspection of DOM elements rendered using 2D (canvas) and 3D (WebGL) graphics contexts."

> Source/JavaScriptCore/inspector/protocol/Canvas.json:14
> +                { "name": "objectId", "type": "integer", "description": "The program child object id within the canvas." }

Not sure what 'program child object id' is.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:30
> +                { "name": "is2d", "type": "boolean", "description": "True if this canvas is backed by a 2D rendering context." },

These are mutually exclusive, right? Then we should make it an enum.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:32
> +                { "name": "programs", "type": "array", "items": { "$ref": "Program" }, "description": "Array of shader program objects." }

This includes all programs, or just linked programs?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:79
> +    for (auto it = m_canvasEntryMap.begin(); it != m_canvasEntryMap.end(); ++it) {

New code (here and elsewhere) should use range-based for loop, unless there's a reason not to:

for (const auto& canvasEntry : m_canvasEntryMap)

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:84
> +    for (auto canvas : toRemove) {

According to mailing list discussions, in most uses of auto we should almost always have the pointer type (* or &) and constness as part of the local variable declaration. Otherwise, you might copy everything that's iterated over, rather than taking a reference. I think here we want |const auto*| to match the vector type parameter.

Nit: since we have several objects per canvas, maybe rename canvas to canvasElement?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:103
> +    canvas.addObserver(*this);

Its a little weird to use per-element observers like this rather than unconditionally calling InspectorInstrumentation, but I suppose there's nothing morally wrong with it since CanvasObservers cannot retain canvases. Most pages probably don't have > 100 canvases so it shouldn't be a performance problem.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:115
> +    auto it = canvasEntry->programIdMap.find(&program);

Nit: auto&, and I usually name the result of .find as 'findResult' or 'foundEntry'

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:154
> +void InspectorCanvasAgent::getShaderSource(ErrorString&, const RefPtr<Inspector::InspectorObject>& programId, const String& shaderType, String* shaderSource)

I would not include these two functions in the initial patch, since they don't do anything and aren't testable yet.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:156
> +    // FIXME: implement for <rdar://problem/18936194>

Nit: unless a feature is unannounced, you should prefer bugzilla links.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:219
> +PassRefPtr<Inspector::Protocol::Canvas::Canvas> InspectorCanvasAgent::buildObjectForCanvas(const CanvasEntry& entry, const HTMLCanvasElement& canvas)

These buildObjectFor* methods are only used by this file, so maybe they should be static methods. AFAICT m_pageAgent->frameId is the only member accessed.

> Source/WebCore/inspector/InspectorCanvasAgent.h:117
> +    virtual void canvasChanged(HTMLCanvasElement&, const FloatRect&) { }

Nit: 'override' keyword

[Brian Burg]

Comment on attachment 241372 [details]
Patch

(Re: windows build failure, you need to add the new agent to InspectorAllInOne.cpp.)

[Joseph Pecoraro]

Comment on attachment 241372 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/Canvas.json:2
> +    "domain": "Canvas",

This needs:

    "availability": "web",

Otherwise it will exist unnecessarily in JSContext inspection.

[Matt Baker]

Comment on attachment 241372 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:154
>> +void InspectorCanvasAgent::getShaderSource(ErrorString&, const RefPtr<Inspector::InspectorObject>& programId, const String& shaderType, String* shaderSource)
> 
> I would not include these two functions in the initial patch, since they don't do anything and aren't testable yet.

Shader related stubs will be removed and added back in https://bugs.webkit.org/show_bug.cgi?id=138593.

[Matt Baker]

Comment on attachment 241372 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:154
>> +void InspectorCanvasAgent::getShaderSource(ErrorString&, const RefPtr<Inspector::InspectorObject>& programId, const String& shaderType, String* shaderSource)
> 
> I would not include these two functions in the initial patch, since they don't do anything and aren't testable yet.

Shader related stubs will be removed and added back in https://bugs.webkit.org/show_bug.cgi?id=138593.

[Matt Baker]

Created attachment 241639 [details]
Patch [backend]

[Matt Baker]

Created attachment 241643 [details]
Patch [backend]

[Matt Baker]

Created attachment 241644 [details]
Patch [frontend]

[Brian Burg]

Comment on attachment 241643 [details]
Patch [backend]

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

Great work. Please fix the nits before landing.

> Source/JavaScriptCore/ChangeLog:8
> +        relocated Canvas protocol from WebCore.

Not sure this is accurate- we nuked the old Canvas.json, right?

> Source/WebCore/ChangeLog:17
> +        children of the DOM. In the absense of the inspector frontend the agent does only what

typo: absence

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:1665
> +#if ENABLE(INSPECTOR)

This should not be guarded. If compiled as !ENABLE(INSPECTOR), then we should still call out to the stub (which does ENABLE(INSPECTOR) test before forwarding to didDeleteProgramImpl())

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:84
> +    for (const auto& canvasElement : canvasesForFrame) {

Err, canvasesForFrame has element type HTMLCanvasElement*, not by reference. (Right?) I'm not sure why this compiles.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:147
> +    results = Inspector::Protocol::Array<Inspector::Protocol::Canvas::Canvas>::create();

Usually we assign at the end if !errorString (otherwise, early return). But since the sub-calls can't cause an error, this is fine as-is.

[Brian Burg]

Comment on attachment 241644 [details]
Patch [frontend]

(per IRC, This is a WIP patch)

[Matt Baker]

Comment on attachment 241643 [details]
Patch [backend]

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        relocated Canvas protocol from WebCore.
> 
> Not sure this is accurate- we nuked the old Canvas.json, right?

Technically it is the same protocol file; the one I created and which was originally located in WebCore. Are you referring to the older canvas inspector protocol from an earlier canvas inspection effort?

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:84
>> +    for (const auto& canvasElement : canvasesForFrame) {
> 
> Err, canvasesForFrame has element type HTMLCanvasElement*, not by reference. (Right?) I'm not sure why this compiles.

auto and auto* are semantically equivalent when the r-value is a pointer type, as are auto/auto& when it is a reference type. AFAIK, in this case we are essentially taking a reference to a pointer, but since it is a reference to a pointer to const, there is no risk of the pointer address being changed.

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:147
>> +    results = Inspector::Protocol::Array<Inspector::Protocol::Canvas::Canvas>::create();
> 
> Usually we assign at the end if !errorString (otherwise, early return). But since the sub-calls can't cause an error, this is fine as-is.

I've seen this pattern used in other agents.

[Brian Burg]

Comment on attachment 241644 [details]
Patch [frontend]

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

Almost ready!

> Source/WebInspectorUI/ChangeLog:10
> +        displayed as child nodes of their respective canvas. A canvas does not have an associated content

What about individual shaders, are they shown underneath the linked program? What if the shader is compiled but not linked?

How are shader compilation failures displayed?

> Source/WebInspectorUI/ChangeLog:13
> +

New tests are missing from Changelog. They should be listed here and in the itemization below. See some recent trac change sets for the format.

> Source/WebInspectorUI/UserInterface/Base/Test.js:98
> +//InspectorTest.dumpMessagesToConsole = false;

Oops.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:48
> +        if (this._didCompleteRequestCanvases) {

It would delight me if this returned a promise, and the payload handling was also implemented using promises. It's annoying to hook this up the first time, but simplifies this code considerably, especially the mechanism to notify multiple callbacks. (Feel free to defer to a later bug.)

Jono is working on a similar conversion for SourceCode.requestContent over here: https://bugs.webkit.org/show_bug.cgi?id=135777
And there are some good examples in ReplayManager, and good tips on the wiki (http://trac.webkit.org/wiki/WebInspectorCodingStyleGuide)

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:63
> +        this._pendingCanvasesForFrameCallbacks.set(frame.id, [callback]);

I would name this something like |this._pendingCallbacks| or |this._waitingCallbacks|. I had to read it several times to figure out why it's named that.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:78
> +            // NOTE: Should a separate canvases array be created for each callback,

Historically we have not done defensive programming like this in the inspector, but I actually prefer doing this for arrays. Plus, this code is not on any hot paths, so it shouldn't make too much garbage.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:81
> +                var frameId = keyValue[0];

var [frameId, callbacks] = keyValue;

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:84
> +                for (var callback of callbacks)

callbacks.map(canvases)

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:94
> +    {

So, I take it that the previous method canvasesForFrame is handling the case where we open the inspector after the page is already loaded, right? Once we've fetched all canvases once, do we ever need to ask the backend again for all canvases?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:115
> +        if (canvas) {

I would turn this into two early-returns to avoid nesting.

if (!canvas)
    return;

...

if (!program)
return;

... do stuff

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:140
> +        delete this._pendingCanvasesForFrameCallbacks;

Not sure whether this is the right thing to do, or explicitly call each callback with an error code (cancelled?).

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:149
> +        for (var canvas of this._canvasIdMap.values()) {

return this._canvasIdMap.values().filter(function(canvas) { ... })

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:110
> +        var programs = [];

You can use no-args Array.prototype.slice() here instead to copy it.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:121
> +        // FIXME: It may also be nice to have a better name for a canvas if it is unique.

sounds like a good idea. Maybe this could go in a subtitle, since the ID could be generated and very long...

please file a follow-up bugzilla bug and add the link in the FIXME.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:134
> +        if (exists)

Hmm, this is interesting. Can a shader be linked multiple times? I don't see how something could ever see both events. I don't see any users of the WasLinked event.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:148
> +        // Do nothing. Canvases are not persisted when the inspector is

This is surprising... why not?

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:40
> +        function(representedObject) { return representedObject instanceof WebInspector.ShaderProgram; },

please add a name to the function after the 'function' keyword so we can know what the predicate is for without cross-referencing.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:42
> +        WebInspector.ShaderProgramTreeElement

We almost never break lines between arguments. Feel free to extract the functions to make this readable on one line.

> LayoutTests/inspector/canvas/canvas-add-remove-events-expected.txt:1
> +FAIL: Timed out waiting for notifyDone to be called

oops

> LayoutTests/inspector/canvas/canvas-add-remove-events.html:30
> +    if (window.GCController)

Add comment to explain why we do this.

> LayoutTests/inspector/canvas/canvas-add-remove-events.html:40
> +        if (contextId == "2d")

===

[Joseph Pecoraro]

Comment on attachment 241643 [details]
Patch [backend]

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

> Source/WebCore/inspector/InspectorCanvasAgent.h:89
> +    unsigned m_canvasId = 0;
> +    unsigned m_objectId = 0;

Is this new syntax that will always make these 0 in construction? I haven't seen it before.

[Build Bot]

Comment on attachment 241644 [details]
Patch [frontend]

Attachment 241644 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5546699971887104

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html
inspector/canvas/canvas-add-remove-events.html

[Build Bot]

Created attachment 241649 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 241644 [details]
Patch [frontend]

Attachment 241644 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5280541922295808

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html
inspector/canvas/canvas-add-remove-events.html

[Build Bot]

Created attachment 241652 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Matt Baker]

Comment on attachment 241643 [details]
Patch [backend]

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

>> Source/WebCore/inspector/InspectorCanvasAgent.h:89
>> +    unsigned m_objectId = 0;
> 
> Is this new syntax that will always make these 0 in construction? I haven't seen it before.

C++11 allows non-static data members to be initialized where they are declared.

[Brian Burg]

(In reply to comment #27)
> Comment on attachment 241643 [details]
> Patch [backend]
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241643&action=review
> 
> > Source/WebCore/inspector/InspectorCanvasAgent.h:89
> > +    unsigned m_canvasId = 0;
> > +    unsigned m_objectId = 0;
> 
> Is this new syntax that will always make these 0 in construction? I haven't
> seen it before.

According to andersca, his preferred syntax for this feature is ... = {0}; (I think it's because that syntax is uniform across scalars, arrays and structs.. ?)

[Matt Baker]

Created attachment 245260 [details]
backend

[Matt Baker]

Created attachment 245262 [details]
frontend

[Jonathan Wells]

Comment on attachment 245262 [details]
frontend

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

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44
> +    canvasesForFrame: function(frame, callback)

Could we use promises here? Should we?

[Matt Baker]

(In reply to comment #36)
> Comment on attachment 245262 [details]
> frontend
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245262&action=review
> 
> > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44
> > +    canvasesForFrame: function(frame, callback)
> 
> Could we use promises here? Should we?

I'm planning on doing a refactor to use promises throughout the CanvasManager (and associated classes) as a separate issue.

[Jonathan Wells]

(In reply to comment #37)
> (In reply to comment #36)
> > Comment on attachment 245262 [details]
> > frontend
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=245262&action=review
> > 
> > > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:44
> > > +    canvasesForFrame: function(frame, callback)
> > 
> > Could we use promises here? Should we?
> 
> I'm planning on doing a refactor to use promises throughout the
> CanvasManager (and associated classes) as a separate issue.

Oh yes, you'd said that before. Very good.

[Build Bot]

Comment on attachment 245260 [details]
backend

Attachment 245260 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5734140733292544

New failing tests:
webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html

[Build Bot]

Created attachment 245265 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245262 [details]
frontend

Attachment 245262 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5636481766916096

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/canvas/canvas-context-attributes.html
inspector/css/selector-dynamic-specificity.html
inspector/canvas/shader-program-linked-events.html
inspector/canvas/shader-compiled-events.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html
inspector/canvas/canvas-add-remove-events.html
inspector/test-harness-trivially-works.html

[Build Bot]

Created attachment 245266 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245262 [details]
frontend

Attachment 245262 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5745949745872896

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/canvas/canvas-context-attributes.html
inspector/css/selector-dynamic-specificity.html
inspector/canvas/shader-program-linked-events.html
inspector/canvas/shader-compiled-events.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html
inspector/canvas/canvas-add-remove-events.html
inspector/test-harness-trivially-works.html

[Build Bot]

Created attachment 245267 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Timothy Hatcher]

You might need to disable local file restrictions in the develop menu for it to work.

[Timothy Hatcher]

Wrong bug. Ignore the previous comment.

[Matt Baker]

Comment on attachment 245262 [details]
frontend

Marked obsolete, need to fix a bug and add "expanded setting" persistence to CanvasTreeElement.

[Matt Baker]

Created attachment 245318 [details]
frontend

[Build Bot]

Comment on attachment 245318 [details]
frontend

Attachment 245318 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5182615929749504

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/css/selector-dynamic-specificity.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 245320 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245318 [details]
frontend

Attachment 245318 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6399674906836992

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/css/selector-dynamic-specificity.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 245322 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Joseph Pecoraro]

Comment on attachment 245260 [details]
backend

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

Nice and clean! Looks good to me, but I want to see the frontend portion.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:31
> +            "id": "InfoLog",

Maybe "BuildLog"? Or do we expect it to have non-build type information.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:71
> +            "name": "getCanvases",
> +            "returns": [
> +                { "name": "result", "type": "array", "items": { "$ref": "Canvas" }, "description": "Array of canvas objects." }
> +            ],
> +            "description": "Returns available canvases for all frames."

Nit: Lets move "description" up above "returns". Same with a few of the events below. Careful of commas when moving things around.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:89
> +                { "name": "infoLog", "$ref": "InfoLog", "description": "Array of <code>ShaderInfoLog</code> objects." }

Nit: Description is now wrong. Seems the type changed to just InfoLog.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:124
> +            "description": "Fired when a program is linked. Does not imply that link was successful."

Should this say if it was successful or not? And if it was not successful, just include the Canvas.InfoLog output?

In other words, when will the frontend request get*InfoLog()? Can it just be included as part of an event?

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Nit: 2015

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:28
> +#if ENABLE(INSPECTOR)

ENABLE(INSPECTOR) was removed in trunk. You'll need to remove these.

> Source/WebCore/inspector/InspectorCanvasAgent.h:93
> +            if (!value->getInteger("canvasId", m_canvasId))

Nit: ASCIILiteral("canvasId")

> Source/WebCore/inspector/InspectorCanvasAgent.h:111
> +        PassRefPtr<ID> asProtocolValue() const

We want to move away from PassRefPtr. This can probably just be Ref<ID>.

> Source/WebCore/inspector/InspectorCanvasAgent.h:142
> +    struct ProgramEntry {
> +        CanvasEntry* canvasEntry;
> +        unsigned programId = {0};
> +        WebGLProgram* program;
> +        WebGLShader* vertexShader;
> +        WebGLShader* fragmentShader;
> +
> +        ProgramEntry() { }
> +
> +        ProgramEntry(CanvasEntry* canvasEntry, unsigned id, WebGLProgram* program)
> +            : canvasEntry(canvasEntry)
> +            , programId(id)
> +            , program(program)
> +        {
> +        }

Shouldn't these constructors nullptr initialize the WebGL pointers?

> Source/WebCore/inspector/InspectorCanvasAgent.h:179
> +    unsigned m_nextCanvasId;
> +    HashMap<HTMLCanvasElement*, CanvasEntry> m_canvasEntries;
> +    HashMap<const WebGLShader*, HashSet<const ProgramEntry*>> m_shaderToProgramEntries;
> +    Timer m_timer;
> +    unsigned m_lastRemovedCanvasId;

Nit: Put the two unsigned members next to eachother for better readability and potentially better memory packing.

[Joseph Pecoraro]

Comment on attachment 245318 [details]
frontend

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

> Source/WebInspectorUI/ChangeLog:38
> +        * UserInterface/Images/Canvas2D.png: Added.
> +        * UserInterface/Images/[e-mail]: Added.
> +        * UserInterface/Images/Canvas3D.png: Added.
> +        * UserInterface/Images/[e-mail]: Added.
> +        * UserInterface/Images/ShaderProgram.png: Added.
> +        * UserInterface/Images/[e-mail]: Added.

Is there a follow-up for better images?

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:45
> +    canvasesForFrame: function(frame, callback)
> +    {

This should probably early return if the Canvas domain was not supported.

    if (!window.CanvasAgent)
        return;

Otherwise, I would expect you get an exception inspecting iOS 6 / 7 / 8 when this is called.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:47
> +            program._canvas = this;

Interesting. Normally we would just have a setter, but I like this approach so we don't expose a setter as public API.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:58
> +// Keep these in sync with the "CanvasType" enum defined by the "Canvas" domain.

Is this comment necessary? Also, I think you renamed CanvasType to ContextType.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:81
> +        case "canvas-2d":

Note that the enum in the protocol generates values here.

    CanvasAgent.ContextType.Canvas2d
    CanvasAgent.ContextType.Webgl

Both of these could be fixed up in scripts/codegen to produce nicer enum names like:

    CanvasAgent.ContextType.Canvas2D
    CanvasAgent.ContextType.WebGL

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:43
> +// Keep these in sync with the "ShaderType" enum defined by the "Canvas" domain.

Ditto.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:68
> +    case "vertex":
> +        return WebInspector.ShaderProgram.ShaderType.Vertex;
> +    case "fragment":
> +        return WebInspector.ShaderProgram.ShaderType.Fragment;

CanvasAgent.ShaderType.Vertex, CanvasAgent.ShaderType.Fragment

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:81
> +    case WebInspector.ShaderProgram.ShaderType.Vertex:
> +        return "vertex";
> +    case WebInspector.ShaderProgram.ShaderType.Fragment:
> +        return "fragment";

And more importantly, here.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:164
> +    shaderWasCompiled: function(shaderTypePayload)
> +    {
> +        var shaderType = WebInspector.ShaderProgram.ShaderTypeFromPayload(shaderTypePayload);
> +        this._getCachedShaderInfo(shaderType).isCurrent = false;

I think this is a great reason to just include build info along with the shader when it was compiled and failed. We would know immediately.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:186
> +            .filter(function(msg) { return msg && msg.trim().length; })
> +            .map(function(msg) { return msg.trim(); });

Flip these lines and you only have to do trim() once per line, instead of once per line and again per-non-empty line.

Also, Is the "msg &&" necessary in the filter? Lets drop it.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Nit: 2015

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:36
> +    this._updateExpandedSetting();

I think this could be inlined. I'd expect to see the WebInspector.Setting in the constructor.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:54
> +WebInspector.CanvasTreeElement.CanvasIconStyle2dClassName = "canvas-2d-icon";
> +WebInspector.CanvasTreeElement.CanvasIconStyle3dClassName = "canvas-3d-icon";

Sometimes I wonder if we should just inline these for use once class names. This is so verbose, it has a negligible runtime cost, and it makes searching a two step process.

> Source/WebInspectorUI/UserInterface/Views/FrameTreeElement.js:269
> +                function() {
> +                    // FIXME: child count callback needs to update with current count.
> +                    // Having WebInspector.Frame listen to CanvasManager events and retain
> +                    // a list of canvases is a possible solution.
> +                    return canvases.length;
> +                }.bind(this),

Unnecessary bind at the moment.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:2
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Nit: 2015

[Brian Burg]

Comment on attachment 245260 [details]
backend

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

> Source/WebCore/ChangeLog:16
> +        practical way to identify those rendering contexts that belong to a frame but are not

Can the frame keep a set of contexts? In some other code, I've seen code use a static global HashMap<Frame*, Set<T>> to track such things. Clearly, this can be fixed at a later time.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:27
> +            "enum": ["fragment", "vertex"],

This is fine as-is. In the future we may need to prefix with the webgl version.

> Source/JavaScriptCore/inspector/protocol/Canvas.json:36
> +                { "name": "lastError", "type": "string", "description": "Diagnostic messages, warning messages, and other information about the compile or link operation." }

Is this optional/possibly empty?

Is this a list of errors or just one big error string? The 'last' prefix probably comes from GL but not sure it's semantically useful here. You can always choose a better name than GL and note the mapping (as above).

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY

I believe the most recent copyright block includes "and its contributors".

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:60
> +    , m_lastRemovedCanvasId(0)

could use default initializers in the header file, like so:

unsigned m_lastRemovedCanvasId {0};

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:217
> +    CanvasEntry* canvasEntry = getCanvasEntry(context.canvas());

These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks?
Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions.

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:295
> +    // Due to the single-process model used in WebKit1, the event must be dispatched from a timer to prevent

(Gross. I thought even WK1 forces async message receives, but it doesn't look like it.)

> Source/WebCore/inspector/InspectorCanvasAgent.cpp:298
> +    m_timer.startOneShot(0);

This mechanism probably won't work if more than one canvas is destroyed per GC, right? You would need a vector of pending removed ids.
An alternative is to create a runnable and call RunLoop::main()->dispatch() but I don't see this approach used very often.

[Brian Burg]

Comment on attachment 245260 [details]
backend

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

>> Source/WebCore/inspector/InspectorCanvasAgent.cpp:217
>> +    CanvasEntry* canvasEntry = getCanvasEntry(context.canvas());
> 
> These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks?
> Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions.

In particular, I'm concerned about the shader/program instrumentation callbacks, which can come at any time and contain the same assertions. The CanvasObserver callbacks should be immune to this.

[Brian Burg]

Comment on attachment 245318 [details]
frontend

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

>> Source/WebInspectorUI/UserInterface/Models/Canvas.js:81
>> +        case "canvas-2d":
> 
> Note that the enum in the protocol generates values here.
> 
>     CanvasAgent.ContextType.Canvas2d
>     CanvasAgent.ContextType.Webgl
> 
> Both of these could be fixed up in scripts/codegen to produce nicer enum names like:
> 
>     CanvasAgent.ContextType.Canvas2D
>     CanvasAgent.ContextType.WebGL

I believe that the enum keys are already formatted with special cases. The values are intentionally not, so as to avoid guessing the capitalization regime. To use them, you would need to make an if-else chain, since arbitrary expressions can't go in case labels.

>> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:36
>> +    this._updateExpandedSetting();
> 
> I think this could be inlined. I'd expect to see the WebInspector.Setting in the constructor.

+1

[Joseph Pecoraro]

(In reply to comment #56)
> Comment on attachment 245260 [details]
> backend
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245260&action=review
> 
> >> Source/WebCore/inspector/InspectorCanvasAgent.cpp:217
> >> +    CanvasEntry* canvasEntry = getCanvasEntry(context.canvas());
> > 
> > These assertions are nice, but what happens if the inspector agents initialize after several contexts already exist and we start getting callbacks?
> > Is there any way to enumerate existing contexts/programs/etc when the agent is initialized? If not (or if this fix goes into its own bug), we may have to remove the assertions.
> 
> In particular, I'm concerned about the shader/program instrumentation
> callbacks, which can come at any time and contain the same assertions. The
> CanvasObserver callbacks should be immune to this.

InspectorCanvasAgent always exists on the InspectorController, which always exists on the WebCore::Page. No context can be created "pre-initialization", the CanvasAgent will always exist and be tracking Canvases.

This is akin to ConsoleAgent which always exists, to store up a queue of console.log/* messages before the inspector has been opened for a page.

[Brian Burg]

(In reply to comment #58)
> InspectorCanvasAgent always exists on the InspectorController, which always
> exists on the WebCore::Page. No context can be created "pre-initialization",
> the CanvasAgent will always exist and be tracking Canvases.
> 
> This is akin to ConsoleAgent which always exists, to store up a queue of
> console.log/* messages before the inspector has been opened for a page.

Ah, ok. We should clean up the agent lifecycles so this is obvious.

[Brian Burg]

I think these patches are pretty much ready to go, pending the comments already posted. Please post an updated patch for EWS, I will add r+, and we can try to land it.

[Brent Fulgham]

Comment on attachment 245260 [details]
backend

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

> Source/WebCore/inspector/InspectorAllInOne.cpp:34
> +#include "InspectorCanvasAgent.cpp"

It would be very helpful if these InspectorCanvasAgent files were also added to the Visual Studio project as "excluded from build". They will be built using this InspectorAllInOne file, but having them in the VS solution makes it easy to search and locate this code when debugging in the future.

[Timothy Hatcher]

Lets try to get these rebased, building and landed this week.

[Matt Baker]

> > Source/WebCore/inspector/InspectorAllInOne.cpp:34
> > +#include "InspectorCanvasAgent.cpp"
> 
> It would be very helpful if these InspectorCanvasAgent files were also added
> to the Visual Studio project as "excluded from build". They will be built
> using this InspectorAllInOne file, but having them in the VS solution makes
> it easy to search and locate this code when debugging in the future.

I agree. Let's do this as part of https://bugs.webkit.org/show_bug.cgi?id=137326.

[Matt Baker]

Created attachment 249439 [details]
[Patch] Backend

[Matt Baker]

Created attachment 249440 [details]
[Patch] Frontend

[Matt Baker]

Created attachment 249442 [details]
[Patch] Tests

[Build Bot]

Comment on attachment 249440 [details]
[Patch] Frontend

Attachment 249440 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6262774938206208

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 249445 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249440 [details]
[Patch] Frontend

Attachment 249440 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5981299961495552

New failing tests:
inspector/model/parse-script-syntax-tree.html
inspector/css/matched-style-properties.html
inspector/protocol-promise-result.html
inspector/event-listener.html
inspector/test-harness-trivially-works.html
inspector/css/pseudo-element-matches.html
inspector/css/selector-specificity.html
inspector/event-listener-set.html

[Build Bot]

Created attachment 249446 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249442 [details]
[Patch] Tests

Attachment 249442 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4898797477953536

New failing tests:
inspector/canvas/canvas-css-name.html
inspector/canvas/canvas-context-attributes.html
inspector/canvas/shader-program-linked-events.html
inspector/canvas/canvas-add-remove-events.html
inspector/canvas/shader-compiled-events.html

[Build Bot]

Created attachment 249447 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249439 [details]
[Patch] Backend

Attachment 249439 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5277612519718912

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html

[Build Bot]

Created attachment 249450 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249439 [details]
[Patch] Backend

Attachment 249439 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5840562473140224

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html

[Build Bot]

Created attachment 249451 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249442 [details]
[Patch] Tests

Attachment 249442 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5586719705399296

New failing tests:
inspector/canvas/canvas-css-name.html
inspector/canvas/canvas-context-attributes.html
inspector/canvas/shader-program-linked-events.html
inspector/canvas/canvas-add-remove-events.html
inspector/canvas/shader-compiled-events.html

[Build Bot]

Created attachment 249452 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Matt Baker]

Created attachment 249455 [details]
[Patch] Backend/Frontend/Tests

[Build Bot]

Comment on attachment 249455 [details]
[Patch] Backend/Frontend/Tests

Attachment 249455 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6199132112814080

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html

[Build Bot]

Created attachment 249463 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Matt Baker]

Created attachment 249472 [details]
[PATCH] Backend/Frontend/Tests

[Build Bot]

Comment on attachment 249472 [details]
[PATCH] Backend/Frontend/Tests

Attachment 249472 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5517818229424128

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html

[Build Bot]

Created attachment 249475 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249472 [details]
[PATCH] Backend/Frontend/Tests

Attachment 249472 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5933383158857728

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-D_G.html
webgl/1.0.2/conformance/more/glsl/arrayOutOfBounds.html
webgl/1.0.2/conformance/more/glsl/uniformOutOfBounds.html
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-A.html

[Build Bot]

Created attachment 249477 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Timothy Hatcher]

Comment on attachment 249472 [details]
[PATCH] Backend/Frontend/Tests

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

Looks good. The layout test failures seem real though. Some minor suggestions / comments.

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:141
> +    getProgramInfoLog(program, callback)
> +    {
> +        CanvasAgent.getProgramInfoLog(program.id, callback);
> +    }
> +
> +    getShaderInfoLog(program, shaderType, callback)
> +    {
> +        CanvasAgent.getShaderInfoLog(program.id, WebInspector.ShaderProgram.shaderTypeToPayload(shaderType), callback);
> +    }

Not used yet. See above.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:46
> +            this._contextAttributes = contextAttributes;

This leaks data from the protocol by just using the data directly. We should have a class or controlled object for attributes that isn't a direct use of the protocol object.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:51
> +                program._canvas = this;

Should have a setter or updateCanvas function, but I did this for Resource's parentFrame so I suck too.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:93
> +    static resetUniqueDisplayNameNumbers()

resetUniqueDisplayNameNumber?

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:141
> +            return WebInspector.UIString("CSS canvas '%s'").format(this._name);

Might want to use curly quotes (“”) instead of single straight quotes since it is a UI string.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:136
> +    getInfoLog(callback)
> +    {
> +        if (this._isCurrent) {
> +            callback(this._linkStatus, this._errorMessages);
> +            return;
> +        }
> +
> +        function processInfoLog(error, status, messages)
> +        {
> +            this._linkStatus = this._statusFromBoolean(status);
> +            this._errorMessages = messages;
> +            this._isCurrent = true;
> +
> +            callback(this._linkStatus, this._errorMessages);
> +        }
> +
> +        WebInspector.canvasManager.getProgramInfoLog(this, processInfoLog.bind(this));
> +    }
> +
> +    getShaderInfoLog(shaderType, callback)
> +    {
> +        var shaderInfo = this._getCachedShaderInfo(shaderType);
> +        if (shaderInfo.isCurrent) {
> +            callback(shaderInfo.compileStatus, shaderInfo.errorMessages);
> +            return;
> +        }
> +
> +        function processShaderInfoLog(error, status, messages)
> +        {
> +            var shaderInfo = this._getCachedShaderInfo(shaderType);
> +            shaderInfo.compileStatus = this._statusFromBoolean(status);
> +            shaderInfo.errorMessages = messages;
> +            shaderInfo.isCurrent = true;
> +
> +            callback(shaderInfo.compileStatus, shaderInfo.errorMessages);
> +        }
> +
> +        WebInspector.canvasManager.getShaderInfoLog(this, shaderType, processShaderInfoLog.bind(this));
> +    }

These aren't used yet. We talked about doing errors differently by sending things through the ConsoleAgent, so errors work even if you aren't looking at the canvas yet. ShaderProgram or the shader sub-objects should be a SourceCode subclass to make that work.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:166
> +        this.dispatchEventToListeners(WebInspector.ShaderProgram.Event.ShaderWasCompiled, { program: this, shaderType: shaderType, status: compileStatus });

Nit: No spaces padding the inside of the {}.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:196
> +    Fragment: "shader-type-fragment",
> +    Vertex: "shader-type-vertex"

These can be Symbol("shader-type-fragment"), etc.

> Source/WebInspectorUI/UserInterface/Models/ShaderProgram.js:202
> +    Succeeded: "shader-program-status-succeded",
> +    Failed: "shader-program-status-failed",
> +    None: "shader-program-status-none"

Ditto.

> Source/WebInspectorUI/UserInterface/Protocol/CanvasObserver.js:34
> +WebInspector.CanvasObserver = function()
> +{
> +    // FIXME: Convert this to a WebInspector.Object subclass, and call super().
> +    // WebInspector.Object.call(this);
> +};
> +
> +WebInspector.CanvasObserver.prototype = {
> +    constructor: WebInspector.CanvasObserver,
> +    __proto__: WebInspector.Object.prototype,

I converted the other observers today. They are super simple and clean now.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:30
> +WebInspector.CanvasTreeElement = function(canvas)
> +{
> +    console.assert(canvas instanceof WebInspector.Canvas);
> +
> +    WebInspector.FolderizedTreeElement.call(this, WebInspector.CanvasTreeElement.StyleClassName, canvas.displayName, null, canvas, false);

This will need to convert to ES6 to work in TOT.

You should inline the value for WebInspector.CanvasTreeElement.StyleClassName and remove it. We are switching how we do one-off class names.

WebInspector.CanvasTreeElement.StyleClassName isn't even defined! You are passing undefined here. It works because of the addStyleClass you do later.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:32
> +    this.addClassName(WebInspector.CanvasTreeElement.CanvasIconStyleClassName);

Don't need this since you can pass it in the super call.

> Source/WebInspectorUI/UserInterface/Views/CanvasTreeElement.js:57
> +WebInspector.CanvasTreeElement.CanvasIconStyleClassName = "canvas-icon";

I don't see any uses of this. Should this have been WebInspector.CanvasTreeElement.CanvasIconStyleClassName?

> Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:55
> +    content: url(../Images/Canvas.svg);

We typically use PNGs for these so they all have a similar style. What does this look like?

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:26
> +WebInspector.ShaderProgramTreeElement = function(program)

Will need to convert to ES6 now.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:29
> +    WebInspector.GeneralTreeElement.call(this, [WebInspector.ShaderProgramTreeElement.StyleClassName, WebInspector.ShaderProgramTreeElement.ShaderProgramIconStyleClassName], program.displayName, null, program, false);

I think you are referencing undefined here. WebInspector.ShaderProgramTreeElement.StyleClassName does not exist.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramTreeElement.js:34
> +WebInspector.ShaderProgramTreeElement.ShaderProgramIconStyleClassName = "shader-program-icon";

Inline where this is used if it does not need to be shared.

[Matt Baker]

> Looks good. The layout test failures seem real though. Some minor
> suggestions / comments.

The failures were caused by a bug which allowed null WebGLShader pointers to be used in a HashMap. Fixed.

> > Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:141
> > +    getProgramInfoLog(program, callback)
> > +    {
> > +        CanvasAgent.getProgramInfoLog(program.id, callback);
> > +    }
> > +
> > +    getShaderInfoLog(program, shaderType, callback)
> > +    {
> > +        CanvasAgent.getShaderInfoLog(program.id, WebInspector.ShaderProgram.shaderTypeToPayload(shaderType), callback);
> > +    }
> 
> Not used yet. See above.

Code and tests related to shader compilation and linking have been removed and will be merged into https://bugs.webkit.org/show_bug.cgi?id=143236. 

> > Source/WebInspectorUI/UserInterface/Models/Canvas.js:93
> > +    static resetUniqueDisplayNameNumbers()
> 
> resetUniqueDisplayNameNumber?

The name was chosen to be consistent with an identical idiom used by Script and CSSStyleSheet.

> > Source/WebInspectorUI/UserInterface/Models/Canvas.js:141
> > +            return WebInspector.UIString("CSS canvas '%s'").format(this._name);
> 
> Might want to use curly quotes (“”) instead of single straight quotes since
> it is a UI string.

Agreed.

> > Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css:55
> > +    content: url(../Images/Canvas.svg);
> 
> We typically use PNGs for these so they all have a similar style. What does
> this look like?

See attachment.

[Matt Baker]

Created attachment 249770 [details]
Canvas resource icon

[Matt Baker]

Created attachment 249798 [details]
[Patch] Backend/Frontend/Tests

[Build Bot]

Comment on attachment 249798 [details]
[Patch] Backend/Frontend/Tests

Attachment 249798 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4955296430555136

New failing tests:
media/invalid-media-url-crash.html
inspector-protocol/dom/remove-multiple-nodes.html

[Build Bot]

Created attachment 249814 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[WebKit Commit Bot]

Comment on attachment 249798 [details]
[Patch] Backend/Frontend/Tests

Clearing flags on attachment: 249798

Committed r182186: <http://trac.webkit.org/changeset/182186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #93)
> Comment on attachment 249798 [details]
> [Patch] Backend/Frontend/Tests
> 
> Clearing flags on attachment: 249798
> 
> Committed r182186: <http://trac.webkit.org/changeset/182186>

It broke the Apple Windows debug build, see the bot for details.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143270

[Dean Jackson]

I had to roll this out because all the WebGL tests crash on Apple Debug bots
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/3189

[Blaze Burg]

As evidenced by the bajillion comments here, this bug and patch are too big in scope. We should split up into at least the following parts (each with own tests):

- Instrument canvas2d contexts in backend
- Instrument webgl contexts in backend
- Instrument shader programs
- Add frontend support for listing contexts
- Add frontend support for showing shaders

[Blaze Burg]

I think this bug has outlived its usefulness. Separate parts that still need to be landed are in other bugs/radars.