Bug 48292 - Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html
Summary: Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.destination-a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 46506
  Show dependency treegraph
 
Reported: 2010-10-25 22:21 PDT by Mike Lawther
Modified: 2011-07-11 00:06 PDT (History)
6 users (show)

See Also:


Attachments
Propose Patch (12.71 KB, patch)
2011-06-19 23:47 PDT, Mustafizur Rahaman (rahaman)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.41 MB, application/zip)
2011-06-20 00:03 PDT, WebKit Review Bot
no flags Details
Patch after fix Chromium EWS tests (12.20 KB, patch)
2011-06-20 00:05 PDT, Mustafizur Rahaman (rahaman)
no flags Details | Formatted Diff | Diff
Fixed the style issue (12.22 KB, patch)
2011-06-20 00:16 PDT, Mustafizur Rahaman (rahaman)
jamesr: review-
Details | Formatted Diff | Diff
Patch to incorporate James Robinson's review comments (11.59 KB, patch)
2011-06-25 10:49 PDT, Mustafizur Rahaman (rahaman)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2010-10-25 22:21:10 PDT
This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506
Comment 1 Mustafizur Rahaman (rahaman) 2011-06-19 23:47:32 PDT
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
Comment 2 WebKit Review Bot 2011-06-20 00:03:05 PDT
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
Comment 3 WebKit Review Bot 2011-06-20 00:03:10 PDT
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
Comment 4 Mustafizur Rahaman (rahaman) 2011-06-20 00:05:25 PDT
Created attachment 97757 [details]
Patch after fix Chromium EWS tests

Fixed the tests issue reported by chromium EWS
Comment 5 Mustafizur Rahaman (rahaman) 2011-06-20 00:09:43 PDT
Comment on attachment 97757 [details]
Patch after fix Chromium EWS tests

Cancelling the review because of style issue
Comment 6 Mustafizur Rahaman (rahaman) 2011-06-20 00:16:16 PDT
Created attachment 97758 [details]
Fixed the style issue
Comment 7 Julien Chaffraix 2011-06-20 19:39:29 PDT
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.
Comment 8 Mustafizur Rahaman (rahaman) 2011-06-20 21:40:03 PDT
(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?
Comment 9 James Robinson 2011-06-24 14:13:31 PDT
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
Comment 10 Mustafizur Rahaman (rahaman) 2011-06-25 10:49:46 PDT
Created attachment 98596 [details]
Patch to incorporate James Robinson's review comments

Updated patch after removing the redundant comment as per jamesr comment.
Comment 11 Mustafizur Rahaman (rahaman) 2011-07-05 10:38:06 PDT
(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?
Comment 12 James Robinson 2011-07-10 23:23:48 PDT
Comment on attachment 98596 [details]
Patch to incorporate James Robinson's review comments

OK
Comment 13 WebKit Review Bot 2011-07-11 00:06:39 PDT
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>
Comment 14 WebKit Review Bot 2011-07-11 00:06:46 PDT
All reviewed patches have been landed.  Closing bug.