[Chromium] Adding deferred canvas painting to the inspector timeline
Created attachment 164730 [details] Patch
senorblanco: please review changes under platform pfeldman: please review changes under inspector
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.
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
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?
> 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.
Comment on attachment 164730 [details] Patch Attachment 164730 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13898575
(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?
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. :)
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?
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)
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.
*** Bug 97103 has been marked as a duplicate of this bug. ***