[webkit-reviews] review denied: [Bug 45476] [Chromium] Minimize uploads in canvas 2d mixed mode rendering : [Attachment 67070] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Vincent Scheib
<scheib at chromium.org>'s request for review:
Bug 45476: [Chromium] Minimize uploads in canvas 2d mixed mode rendering
https://bugs.webkit.org/show_bug.cgi?id=45476

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
review- for the build break.  Overall looks awesome.


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

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1534
>  #if ENABLE(ACCELERATED_2D_CANVAS) && USE(ACCELERATED_COMPOSITING)
> +    if (isAccelerated())
> +	   drawingContext()->addDirtyRect(enclosingIntRect(dirtyRect));
The addDirtyRect() call should only be guarded by
ENABLE(ACCELERATED_2D_CANVAS).	We still have to make this call even if
accelerated compositing is not enabled in order to render correctly.

> WebCore/html/canvas/CanvasRenderingContext2D.h:272
> +	   CanvasDidDrawApplyNone = 0
nit: this would be better at the beginning of the enum definition

> WebCore/platform/graphics/GraphicsContext.h:427
> +	   void addDirtyRect(const IntRect& rect);
You have to add a stub implementation of this to GraphicsContext.cpp for
!PLATFORM(SKIA) or this won't compile on other platforms.  See how
setSharedGraphicsContext3D and syncSoftwareCanvas() are handled.

Don't name the parameter here.

Also, this name is less than ideal.  It appears to have a much broader
application than it really does.

> WebCore/platform/graphics/gpu/Texture.cpp:147
> +
> +
remove the extra new lines here

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:818
> +    switch (m_backingStoreState) {
> +    case Software:
> +    case Mixed:
> +	   m_softwareDirtyRect.unite(rect);
> +	   return;
> +    case Hardware:
> +	   return;
> +    default:
> +	   ASSERT_NOT_REACHED();
This is probably clearer as an if() instead of a switch.  The other cases are
not likely to get interesting.

> WebCore/platform/graphics/skia/PlatformContextSkia.h:197
> +    void addDirtyRect(const IntRect& rect);
don't name the parameter here