[webkit-reviews] review denied: [Bug 48291] Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.copy.html : [Attachment 97332] Incorporated review comments of Julien(comment#23) & Darin(comment#24)

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Mustafizur Rahaman
<mustaf.here at gmail.com>'s request for review:
Bug 48291: Fix
LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.copy.html
https://bugs.webkit.org/show_bug.cgi?id=48291

Attachment 97332: Incorporated review comments of Julien(comment#23) &
Darin(comment#24)
https://bugs.webkit.org/attachment.cgi?id=97332&action=review

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

I'd be interesting in seeing perf comparisons that do not cover the entire
canvas.  If the entire canvas is being copied then the clipOut() will exclude
the whole canvas so that fill will be a no-op, so it's not surprising that
performance is unaffected.  I'm also not sure about using clipOut for copy - it
seems that for a path that isn't exactly on pixel boundaries this will cause
issues with antialiasing.

If the clip region is small relative to the canvas, would it be more efficient
to just clear the whole canvas without setting a clipOut at all before the
actual draw?  On GPU implementations I could imagine this doing quite well.

This will change the pixel output for fast/canvas/canvas-composite.html, at
least.	Could you update the pixel expectations for at least one platform so we
can see that it's correct?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1514
> +    // As per the drawing model mentioned in
http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.h
tml#drawing-model
> +    // We should display transparency for everywhere else for CompositeCopy,
too

This comment does not seem useful, the comment on 1510 explains why we are
displaying transparency elsewhere