[webkit-reviews] review granted: [Bug 50591] Adopt new CG API for canvas : [Attachment 76354] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 50591: Adopt new CG API for canvas
https://bugs.webkit.org/show_bug.cgi?id=50591

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=76354&action=review

r=me with commenets addressed

> WebCore/html/canvas/CanvasRenderingContext2D.cpp:148
> +#if PLATFORM(MAC) && PLATFORM(CA) && !defined(BUILDING_ON_TIGER) &&
!defined(BUILDING_ON_LEOPARD) && !defined(BUILDING_ON_SNOW_LEOPARD)
> +    return true;

Why not #if defined(USE_IOSURFACE)?

> WebCore/platform/graphics/ImageBuffer.h:126
> +	   ImageBuffer(const IntSize&, ColorSpace colorSpace, bool
accelerateRendering, bool& success);

I think the constructor should use RenderingMode. create methods almost always
just pass their arguments directly to the constructor.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:114
>      success = false;  // Make early return mean failure.

You should assert that accelerateRendering is Unaccelerated if USE_IOSURFACE is
not defined.

> WebCore/platform/graphics/cg/ImageBufferCG.cpp:230
>	   RefPtr<Image> copy = copyImage();

Either add a comment to say why copying is OK, or file a bug to investigate.