[webkit-reviews] review denied: [Bug 76732] [Chromium] Enable deferred canvas rendering in the skia port : [Attachment 123718] Same as previous patch. Reposted to show green on EWS, now that DEPS have rolled.

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Justin Novosad
<junov at chromium.org>'s request for review:
Bug 76732: [Chromium] Enable deferred canvas rendering in the skia port
https://bugs.webkit.org/show_bug.cgi?id=76732

Attachment 123718: Same as previous patch.  Reposted to show green on EWS, now
that DEPS have rolled.
https://bugs.webkit.org/attachment.cgi?id=123718&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123718&action=review


> Source/WebCore/platform/graphics/ImageBuffer.h:64
> +	   AcceleratedAndDeferred,

why does this need to be a separate mode in cross-platform code? What bits of
cross-platform code do you expect will have different behavior based on this
flag?

if this is a skia-only setting then we should be able to hide it in skia-only
code

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:138
> +    if (m_flushCallback.get())
> +	   m_flushCallback->flushCallback();

we do the ganesh flush on the grContext in paintContentsIfDirty(). why is this
different? do you need access to the GraphicsContext3D for this call? what
thread do you expect this to be called on?

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:47
> +    class FlushCallback {

"flush" is pretty ambiguous - what does this mean? when should it be called? at
a glance, i'd guess this has something to do with glFlush, but it definitely
doesn't. Needs documentation at the very least

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:121
> +AcceleratedDeviceContext AcceleratedDeviceContext::m_instance;

does this require a static initializer? those are no-nos