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

[bugzilla-daemon at webkit.org]

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

Brian Burg <burg at cs.washington.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #241372|review?                     |review-
              Flags|                            |

--- Comment #15 from Brian Burg <burg at cs.washington.edu> ---
Comment on attachment 241372
  --> https://bugs.webkit.org/attachment.cgi?id=241372
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141111/b525b404/attachment-0002.html>