Bug 97104 – [Chromium] Adding deferred canvas painting to the inspector timeline

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Justin Novosad]

[Chromium] Adding deferred canvas painting to the inspector timeline

[Justin Novosad]

Created attachment 164730 [details]
Patch

[Justin Novosad]

senorblanco: please review changes under platform
pfeldman: please review changes under inspector

[Justin Novosad]

Question about inspector: Is there a unit testing framework somewhere in webkit to verify timeline event recording? I didn't find anything, so I just tested the feature manually in a Chrome build.

[WebKit Review Bot]

Comment on attachment 164730 [details]
Patch

Attachment 164730 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13891589

New failing tests:
inspector/timeline/timeline-enum-stability.html

[Stephen White]

Comment on attachment 164730 [details]
Patch

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

Leaving for Pavel.  Thanks for the extra work to preserve layering.  I'm ok with this going in as-is, so consider the comments as optional.

The fact that InspectorInstrumentation needs a Document makes me sad:  this patch would be a lot smaller if it didn't.  I think the TRACE_EVENT macros now magically make stuff show up in the Inspector as well as tracing?  Or is that different than this use case?

> Source/WebCore/ChangeLog:10
> +        the event is passed from Canvas2DLayer bridge down to HTMLCanvasElement,

Nit:  "Canvas2DLayer bridge" -> Canvas2DLayerBridge

> Source/WebCore/html/HTMLCanvasElement.cpp:582
> +        m_imageBuffer->setObserver(DeferredCanvasClient::create(this));

You should probably pick either observer or client, and name everything similarly.  Client seems to be more common in WebKit.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:56
> +    , m_imageBuffer(0)

Kind of unfortunate that we now have an explicit 3-way dependency:  Canvas2DLayerBridge -> ImageBuffer -> ImageBufferData -> Canvas2DLayerBridge.  Perhaps it was always there (since you didn't need to add an #include), but sad nonetheless.  One way to break it (at least for this use case) would be to pull ImageBufferClient out into its own .h, and #include it from both ImageBuffer and Canvas2DLayerBridge, and pass only the ImageBufferClient to the bridge, instead of the whole ImageBuffer.  Does that work, or is it "too late" since the bridge is already constructed by the time we get the observer?

[Nat Duca]

> The fact that InspectorInstrumentation needs a Document makes me sad:  this patch would be a lot smaller if it didn't.  I think the TRACE_EVENT macros now magically make stuff show up in the Inspector as well as tracing?  Or is that different than this use case?

I think in this case, inspector has the right approach: remember, in trace event, you dont know what tab/view/frame/document a given event comes from. Thats convenient for geeks like us. But, OTOH, when you're a webdev and were accidentllaly put in the same process as newgrounds, for instance, it matters a whole lot whose canvas work is whose.

[Build Bot]

Comment on attachment 164730 [details]
Patch

Attachment 164730 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13898575

[Stephen White]

(In reply to comment #6)
> > The fact that InspectorInstrumentation needs a Document makes me sad:  this patch would be a lot smaller if it didn't.  I think the TRACE_EVENT macros now magically make stuff show up in the Inspector as well as tracing?  Or is that different than this use case?
> 
> I think in this case, inspector has the right approach: remember, in trace event, you dont know what tab/view/frame/document a given event comes from. Thats convenient for geeks like us. But, OTOH, when you're a webdev and were accidentllaly put in the same process as newgrounds, for instance, it matters a whole lot whose canvas work is whose.

Understood for tracing; I was referring to this:

http://trac.webkit.org/changeset/125769

where calls to TRACE_EVENT were replaced with calls to PlatformInstrumentation (which doesn't seem to require a Document).

However, I guess InspectorInstrumentation is different from PlatformInstrumentation?

I understand that the Inspector* flavours end up the timeline, which is why they need the Document; I'm guessing PlatformInstrumentation doesn't show up in the timeline, which is why we can't use it in this case?

[Nat Duca]

Ah, there's a bit of magic there: when we paint, you set a client for platform instrumentation to be the active webviewimpl. Then the actual webview-flavored platform instrumentation client is the one to bind into the inspector's document-specific calls. Or so I gather. :)

[Justin Novosad]

Argh! I wish I had found out about this class earlier.  PlatformInstrumentation definitely seems like a cleaner bootstrap.  However, I don't see how it would manage to filter-out events from background tabs that are owned by the same render process.  Pavel, is this ever an issue?

[Pavel Feldman]

Comment on attachment 164730 [details]
Patch

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

> Source/WebCore/html/HTMLCanvasElement.cpp:82
> +class DeferredCanvasClient : public ImageBufferClient {

If you know for sure that willFlush and didFlush are running synchronously within another instrumented event (paint / layout / anything), you should use PlatformInstrumentation instead. It will use appropriate InspectorTimelineAgent instance and you will not need to care about the document pointer. It is supposed to be a universal solution for making instrumentation calls from within platform/.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:134
> +        m_imageBuffer->didFlushDeferred();

It is not obvious from the code whether willFlushDeferred and didFlushDeferred are running synchronously on the same stack depth. You can only use didCompleteCurrentRecord in case they do. If they don't, you should push them to the timeline as two atomic events and glue them in the UI later.

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:71
> +    void setImageBuffer(ImageBuffer* imageBuffer) {m_imageBuffer = imageBuffer;}

This seems to be consistent with the rest of this class, but is not consistent with the general WebKit style. Should be
{ m_imageBuffer = imageBuffer; }

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:99
> +    (void)imageBuffer;

UNUSED_PARAM(imageBuffer)

[Andrey Kosyakov]

Comment on attachment 164730 [details]
Patch

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

>> Source/WebCore/html/HTMLCanvasElement.cpp:82
>> +class DeferredCanvasClient : public ImageBufferClient {
> 
> If you know for sure that willFlush and didFlush are running synchronously within another instrumented event (paint / layout / anything), you should use PlatformInstrumentation instead. It will use appropriate InspectorTimelineAgent instance and you will not need to care about the document pointer. It is supposed to be a universal solution for making instrumentation calls from within platform/.

I don't think that's the case here -- the whole point of using deferred canvas is that it happens after paint.

[Andrey Kosyakov]

*** Bug 97103 has been marked as a duplicate of this bug. ***