[webkit-reviews] review denied: [Bug 97104] [Chromium] Adding deferred canvas painting to the inspector timeline : [Attachment 164730] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Justin Novosad
<junov at google.com>'s request for review:
Bug 97104: [Chromium] Adding deferred canvas painting to the inspector timeline
https://bugs.webkit.org/show_bug.cgi?id=97104

Attachment 164730: Patch
https://bugs.webkit.org/attachment.cgi?id=164730&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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)