Bug 137278 - Web Inspector: add 2D/WebGL canvas instrumentation infrastructure
Summary: Web Inspector: add 2D/WebGL canvas instrumentation infrastructure
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on: 172624 143270 172623
Blocks: 140901
  Show dependency treegraph
 
Reported: 2014-09-30 17:14 PDT by Matt Baker
Modified: 2017-07-03 10:24 PDT (History)
18 users (show)

See Also:


Attachments
Patch (65.63 KB, patch)
2014-10-01 12:10 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (78.54 KB, patch)
2014-10-01 17:56 PDT, Matt Baker
joepeck: review-
Details | Formatted Diff | Diff
Patch (83.62 KB, application/octet-stream)
2014-10-06 18:27 PDT, Matt Baker
no flags Details
Patch (83.62 KB, patch)
2014-10-06 18:28 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (100.47 KB, patch)
2014-11-10 19:05 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (100.49 KB, patch)
2014-11-11 13:06 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch [backend] (44.72 KB, patch)
2014-11-14 16:16 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch [backend] (44.63 KB, patch)
2014-11-14 16:44 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch [frontend] (67.35 KB, patch)
2014-11-14 16:46 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (567.38 KB, application/zip)
2014-11-14 18:09 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (579.97 KB, application/zip)
2014-11-14 18:19 PST, Build Bot
no flags Details
backend (62.65 KB, patch)
2015-01-23 16:54 PST, Matt Baker
no flags Details | Formatted Diff | Diff
frontend (87.65 KB, patch)
2015-01-23 16:56 PST, Matt Baker
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (604.17 KB, application/zip)
2015-01-23 17:20 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-mavericks (613.15 KB, application/zip)
2015-01-23 17:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (724.90 KB, application/zip)
2015-01-23 18:04 PST, Build Bot
no flags Details
frontend (67.18 KB, patch)
2015-01-25 17:08 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (578.44 KB, application/zip)
2015-01-25 17:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (730.29 KB, application/zip)
2015-01-25 18:12 PST, Build Bot
no flags Details
[Patch] Backend (67.63 KB, patch)
2015-03-25 16:26 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Frontend (66.92 KB, patch)
2015-03-25 16:29 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Patch] Tests (24.66 KB, patch)
2015-03-25 16:32 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (702.35 KB, application/zip)
2015-03-25 17:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (697.64 KB, application/zip)
2015-03-25 17:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (628.65 KB, application/zip)
2015-03-25 17:10 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-mavericks (672.52 KB, application/zip)
2015-03-25 17:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (593.06 KB, application/zip)
2015-03-25 17:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (590.96 KB, application/zip)
2015-03-25 17:34 PDT, Build Bot
no flags Details
[Patch] Backend/Frontend/Tests (134.52 KB, patch)
2015-03-25 17:59 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (587.52 KB, application/zip)
2015-03-25 19:01 PDT, Build Bot
no flags Details
[PATCH] Backend/Frontend/Tests (134.48 KB, patch)
2015-03-25 22:19 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (623.86 KB, application/zip)
2015-03-25 23:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-mavericks (721.23 KB, application/zip)
2015-03-25 23:19 PDT, Build Bot
no flags Details
Canvas resource icon (280.12 KB, image/png)
2015-03-30 15:12 PDT, Matt Baker
no flags Details
[Patch] Backend/Frontend/Tests (121.74 KB, patch)
2015-03-30 21:48 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (583.45 KB, application/zip)
2015-03-31 03:07 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2014-09-30 17:14:16 PDT
Implement canvas tooling in Inspector frontend and backend. Initial patches should allow inspection of WebGL canvas resources (shaders and textures).
Comment 1 Radar WebKit Bug Importer 2014-09-30 17:14:29 PDT
<rdar://problem/18509072>
Comment 2 Matt Baker 2014-10-01 12:10:26 PDT
Created attachment 239038 [details]
Patch
Comment 3 Joseph Pecoraro 2014-10-01 13:02:02 PDT
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...
Comment 4 Matt Baker 2014-10-01 17:56:26 PDT
Created attachment 239073 [details]
Patch
Comment 5 Timothy Hatcher 2014-10-01 19:27:32 PDT
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.
Comment 6 Joseph Pecoraro 2014-10-01 19:50:34 PDT
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.
Comment 7 Matt Baker 2014-10-06 18:27:18 PDT
Created attachment 239374 [details]
Patch
Comment 8 Matt Baker 2014-10-06 18:28:56 PDT
Created attachment 239375 [details]
Patch
Comment 9 Timothy Hatcher 2014-10-07 14:42:57 PDT
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__.
Comment 10 Joseph Pecoraro 2014-10-07 16:57:14 PDT
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?
Comment 11 Timothy Hatcher 2014-10-09 09:58:55 PDT
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!
Comment 12 Matt Baker 2014-11-10 19:05:55 PST
Created attachment 241326 [details]
Patch
Comment 13 Brian Burg 2014-11-11 11:28:49 PST
Comment on attachment 241326 [details]
Patch

This patch doesn't seem to build. I'll have more comments after lunch.
Comment 14 Matt Baker 2014-11-11 13:06:11 PST
Created attachment 241372 [details]
Patch
Comment 15 Brian Burg 2014-11-11 13:46:02 PST
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
Comment 16 Brian Burg 2014-11-11 13:50:18 PST
Comment on attachment 241372 [details]
Patch

(Re: windows build failure, you need to add the new agent to InspectorAllInOne.cpp.)
Comment 17 Joseph Pecoraro 2014-11-11 14:05:28 PST
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.
Comment 18 Matt Baker 2014-11-14 16:02:52 PST
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.
Comment 19 Matt Baker 2014-11-14 16:02:55 PST
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.
Comment 20 Matt Baker 2014-11-14 16:16:09 PST
Created attachment 241639 [details]
Patch [backend]
Comment 21 Matt Baker 2014-11-14 16:44:55 PST
Created attachment 241643 [details]
Patch [backend]
Comment 22 Matt Baker 2014-11-14 16:46:51 PST
Created attachment 241644 [details]
Patch [frontend]
Comment 23 Brian Burg 2014-11-14 16:57:29 PST
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.
Comment 24 Brian Burg 2014-11-14 16:58:12 PST
Comment on attachment 241644 [details]
Patch [frontend]

(per IRC, This is a WIP patch)
Comment 25 Matt Baker 2014-11-14 17:23:14 PST
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.
Comment 26 Brian Burg 2014-11-14 17:35:51 PST
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")

===
Comment 27 Joseph Pecoraro 2014-11-14 17:48:38 PST
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.
Comment 28 Build Bot 2014-11-14 18:09:45 PST
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
Comment 29 Build Bot 2014-11-14 18:09:52 PST
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
Comment 30 Build Bot 2014-11-14 18:19:14 PST
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
Comment 31 Build Bot 2014-11-14 18:19:24 PST
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
Comment 32 Matt Baker 2014-11-14 18:42:52 PST
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.
Comment 33 Brian Burg 2014-12-13 13:26:45 PST
(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.. ?)
Comment 34 Matt Baker 2015-01-23 16:54:31 PST
Created attachment 245260 [details]
backend
Comment 35 Matt Baker 2015-01-23 16:56:18 PST
Created attachment 245262 [details]
frontend
Comment 36 jonowells 2015-01-23 17:03:10 PST
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?
Comment 37 Matt Baker 2015-01-23 17:15:03 PST
(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.
Comment 38 jonowells 2015-01-23 17:18:56 PST
(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.
Comment 39 Build Bot 2015-01-23 17:19:57 PST
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
Comment 40 Build Bot 2015-01-23 17:20:04 PST
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
Comment 41 Build Bot 2015-01-23 17:44:13 PST
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
Comment 42 Build Bot 2015-01-23 17:44:20 PST
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
Comment 43 Build Bot 2015-01-23 18:04:00 PST
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
Comment 44 Build Bot 2015-01-23 18:04:11 PST
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
Comment 45 Timothy Hatcher 2015-01-24 19:03:59 PST
You might need to disable local file restrictions in the develop menu for it to work.
Comment 46 Timothy Hatcher 2015-01-24 19:05:29 PST
Wrong bug. Ignore the previous comment.
Comment 47 Matt Baker 2015-01-25 17:02:01 PST
Comment on attachment 245262 [details]
frontend

Marked obsolete, need to fix a bug and add "expanded setting" persistence to CanvasTreeElement.
Comment 48 Matt Baker 2015-01-25 17:08:15 PST
Created attachment 245318 [details]
frontend
Comment 49 Build Bot 2015-01-25 17:53:34 PST
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
Comment 50 Build Bot 2015-01-25 17:53:40 PST
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
Comment 51 Build Bot 2015-01-25 18:11:54 PST
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
Comment 52 Build Bot 2015-01-25 18:12:01 PST
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
Comment 53 Joseph Pecoraro 2015-01-26 11:57:40 PST
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.
Comment 54 Joseph Pecoraro 2015-01-26 12:26:03 PST
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/Canvas2D@2x.png: Added.
> +        * UserInterface/Images/Canvas3D.png: Added.
> +        * UserInterface/Images/Canvas3D@2x.png: Added.
> +        * UserInterface/Images/ShaderProgram.png: Added.
> +        * UserInterface/Images/ShaderProgram@2x.png: 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
Comment 55 Brian Burg 2015-01-26 14:21:37 PST
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.
Comment 56 Brian Burg 2015-01-26 14:26:23 PST
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.
Comment 57 Brian Burg 2015-01-26 14:36:23 PST
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
Comment 58 Joseph Pecoraro 2015-01-26 20:00:55 PST
(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.
Comment 59 Brian Burg 2015-02-03 07:36:22 PST
(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.
Comment 60 Brian Burg 2015-02-03 07:38:52 PST
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.
Comment 61 Brent Fulgham 2015-02-05 10:00:31 PST
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.
Comment 62 Timothy Hatcher 2015-02-28 15:47:17 PST
Lets try to get these rebased, building and landed this week.
Comment 63 Matt Baker 2015-03-25 13:58:12 PDT
> > 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.
Comment 64 Matt Baker 2015-03-25 16:26:14 PDT
Created attachment 249439 [details]
[Patch] Backend
Comment 65 Matt Baker 2015-03-25 16:29:55 PDT
Created attachment 249440 [details]
[Patch] Frontend
Comment 66 Matt Baker 2015-03-25 16:32:00 PDT
Created attachment 249442 [details]
[Patch] Tests
Comment 67 Build Bot 2015-03-25 17:00:01 PDT
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
Comment 68 Build Bot 2015-03-25 17:00:08 PDT
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
Comment 69 Build Bot 2015-03-25 17:05:37 PDT
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
Comment 70 Build Bot 2015-03-25 17:05:46 PDT
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
Comment 71 Build Bot 2015-03-25 17:10:12 PDT
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
Comment 72 Build Bot 2015-03-25 17:10:22 PDT
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
Comment 73 Build Bot 2015-03-25 17:26:01 PDT
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
Comment 74 Build Bot 2015-03-25 17:26:10 PDT
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
Comment 75 Build Bot 2015-03-25 17:27:05 PDT
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
Comment 76 Build Bot 2015-03-25 17:27:13 PDT
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
Comment 77 Build Bot 2015-03-25 17:34:19 PDT
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
Comment 78 Build Bot 2015-03-25 17:34:26 PDT
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
Comment 79 Matt Baker 2015-03-25 17:59:26 PDT
Created attachment 249455 [details]
[Patch] Backend/Frontend/Tests
Comment 80 Build Bot 2015-03-25 19:00:52 PDT
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
Comment 81 Build Bot 2015-03-25 19:01:00 PDT
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
Comment 82 Matt Baker 2015-03-25 22:19:04 PDT
Created attachment 249472 [details]
[PATCH] Backend/Frontend/Tests
Comment 83 Build Bot 2015-03-25 23:06:50 PDT
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
Comment 84 Build Bot 2015-03-25 23:07:02 PDT
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
Comment 85 Build Bot 2015-03-25 23:19:01 PDT
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
Comment 86 Build Bot 2015-03-25 23:19:09 PDT
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
Comment 87 Timothy Hatcher 2015-03-27 00:12:39 PDT
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.
Comment 88 Matt Baker 2015-03-30 15:11:54 PDT
> 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.
Comment 89 Matt Baker 2015-03-30 15:12:57 PDT
Created attachment 249770 [details]
Canvas resource icon
Comment 90 Matt Baker 2015-03-30 21:48:19 PDT
Created attachment 249798 [details]
[Patch] Backend/Frontend/Tests
Comment 91 Build Bot 2015-03-31 03:07:27 PDT
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
Comment 92 Build Bot 2015-03-31 03:07:39 PDT
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
Comment 93 WebKit Commit Bot 2015-03-31 10:24:01 PDT
Comment on attachment 249798 [details]
[Patch] Backend/Frontend/Tests

Clearing flags on attachment: 249798

Committed r182186: <http://trac.webkit.org/changeset/182186>
Comment 94 WebKit Commit Bot 2015-03-31 10:24:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 95 Csaba Osztrogonác 2015-03-31 11:04:59 PDT
(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.
Comment 96 WebKit Commit Bot 2015-03-31 11:57:49 PDT
Re-opened since this is blocked by bug 143270
Comment 97 Dean Jackson 2015-03-31 11:58:13 PDT
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
Comment 98 Brian Burg 2016-11-05 10:10:47 PDT
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
Comment 99 Brian Burg 2017-07-03 10:24:06 PDT
I think this bug has outlived its usefulness. Separate parts that still need to be landed are in other bugs/radars.