Summary: | Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.destination-atop.html | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Lawther <mikelawther> | ||||||||||||
Component: | Canvas | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | darin, jchaffraix, kling, mdelaney7, mustaf.here, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 46506 | ||||||||||||||
Attachments: |
|
Description
Mike Lawther
2010-10-25 22:21:10 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 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 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
Created attachment 97757 [details]
Patch after fix Chromium EWS tests
Fixed the tests issue reported by chromium EWS
Comment on attachment 97757 [details]
Patch after fix Chromium EWS tests
Cancelling the review because of style issue
Created attachment 97758 [details]
Fixed the style issue
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. (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 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 Created attachment 98596 [details]
Patch to incorporate James Robinson's review comments
Updated patch after removing the redundant comment as per jamesr comment.
(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 on attachment 98596 [details]
Patch to incorporate James Robinson's review comments
OK
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> All reviewed patches have been landed. Closing bug. |