[webkit-reviews] review denied: [Bug 66036] Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors : [Attachment 103989] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Ben Wells
<benwells at chromium.org>'s request for review:
Bug 66036: Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut,
DestinationAtop and Copy have errors
https://bugs.webkit.org/show_bug.cgi?id=66036

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

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


Getting there!

> LayoutTests/fast/canvas/canvas-composite-transformclip.html:76
> +	   if (alpha) {
> +	     context.fillStyle = 'rgba(255,64,0,0.5)';
> +	   } else {
> +	     context.fillStyle = 'rgba(255,64,0,1)';

I know that the existing compositing tests use red, but could you alter this
test to use blue+green or something instead?  We typically try to avoid using
red in our pixel test expectations.  fast/canvas/canvas-composite.html was
imported, so we just left it as is, but for new tests we shouldn't have red.

> LayoutTests/platform/chromium/test_expectations.txt:3647
> +// Need rebaselining after canvas composite mode fixes
> +BUGWK66036 MAC WIN GPU : fast/canvas/canvas-composite.html = IMAGE
> +BUGWK66036 MAC WIN GPU : fast/canvas/canvas-composite-transformclip.html =
IMAGE

i would expect that these tests need new baselines for chromium linux as well,
but i don't see any on this patch. did you forget to git add the new
-expected.png files?

> Source/WebCore/ChangeLog:10
> +	   Canvas fill and fillRect with SourceIn, DestinationIn, SourceOut,
DestinationAtop and Copy have errors
> +	   https://bugs.webkit.org/show_bug.cgi?id=66036
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: fast/canvas/canvas-composite-transformclip.html
> +
> +	   * html/canvas/CanvasRenderingContext2D.cpp:

could you please explain what you are doing here? ideally there would be some
description of what the canvas 2d globalCompositeOperation semantics are, how
they are different from what the raster engines we use (CoreGraphics, skia,
cairo, etc) expect, and how this approach bridges the gap

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1505
> +    canvasRect = state().m_transform.inverse().mapRect(canvasRect);
> +
> +    GraphicsContext* c = drawingContext();
> +    c->clearRect(canvasRect);

rather than trying to invert the current transform, can you just temporarily
set the transform to identity?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1543
> +    OwnPtr<ImageBuffer> buffer =  ImageBuffer::create(bufferRect.size());

nit: extra space after the =

unfortunately it looks like this will not actually propagate the
accelerated-ness of the ImageBuffer in the way you'd expect. to make this work
properly, you'll need to add an interface to ImageBuffer to query its
accelerated state and pass that in. you should probably also patch in the same
color space, although i'm pretty sure all canvas ImageBuffers use the default
(ColorSpaceDeviceRGB)