Bug 48292 – Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Mike Lawther]

This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506

[Mustafizur Rahaman (rahaman)]

Created attachment 97754 [details]
Propose Patch

As per the discussion on https://bugs.webkit.org/show_bug.cgi?id=48297, splitting the original patch & only submitting the relevant change for DestinationAtop here. Once this patch is accepted, will submit the remaining patch for image related change in https://bugs.webkit.org/show_bug.cgi?id=48297

[WebKit Review Bot]

Comment on attachment 97754 [details]
Propose Patch

Attachment 97754 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8908399

New failing tests:
canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html

[WebKit Review Bot]

Created attachment 97756 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mustafizur Rahaman (rahaman)]

Created attachment 97757 [details]
Patch after fix Chromium EWS tests

Fixed the tests issue reported by chromium EWS

[Mustafizur Rahaman (rahaman)]

Comment on attachment 97757 [details]
Patch after fix Chromium EWS tests

Cancelling the review because of style issue

[Mustafizur Rahaman (rahaman)]

Created attachment 97758 [details]
Fixed the style issue

[Julien Chaffraix]

Comment on attachment 97758 [details]
Fixed the style issue

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

LGTM.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1520
> +

A bit too verbose but at least we understand why we need different behaviors. Also if you put this comment, it is redundant with the previous one for the source-atop one but it should be OK.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #7)
> (From update of attachment 97758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97758&action=review
> 
> LGTM.
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1520
> > +
> 
> A bit too verbose but at least we understand why we need different behaviors. Also if you put this comment, it is redundant with the previous one for the source-atop one but it should be OK.

Yeah, I already told you so, that's why I had made my previous comment brief though knowing it is not explaining the reason in details :-)

Would any one please r+ this patch, if there is no other comment?

[James Robinson]

Comment on attachment 97758 [details]
Fixed the style issue

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1519
> +    // As per the HTML Canvas spec (http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html)
> +    // For source-atop, the expected behavior when S= Transparent(T) & D=Opaque(O) or vice-versa
> +    // S = O, D= T =>Expected Canvas Color = T
> +    // S = T, D= O =>Expected Canvas Color = O => i.e. we don't need to clear the Source => NO need for CompositeSourceAtop
> +    // But for destination-top,the expected behavior S= Transparent(T) & D=Opaque(O) or vice-versa
> +    // S = O, D= T =>Expected Canvas Color = T
> +    // S = T, D= O =>Expected Canvas Color = T => i.e. we need to clear the Source => Need for CompositeDestinationAtop

I don't think this comment is helpful, please remove it

[Mustafizur Rahaman (rahaman)]

Created attachment 98596 [details]
Patch to incorporate James Robinson's review comments

Updated patch after removing the redundant comment as per jamesr comment.

[Mustafizur Rahaman (rahaman)]

(In reply to comment #10)
> Created an attachment (id=98596) [details]
> Patch to incorporate James Robinson's review comments
> 
> Updated patch after removing the redundant comment as per jamesr comment.

Can any one please review this patch?

[James Robinson]

Comment on attachment 98596 [details]
Patch to incorporate James Robinson's review comments

OK

[WebKit Review Bot]

Comment on attachment 98596 [details]
Patch to incorporate James Robinson's review comments

Clearing flags on attachment: 98596

Committed r90723: <http://trac.webkit.org/changeset/90723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.