Bug 39027

The spec and Philip's test require that the pixels not covered by the source object should be cleared to (0,0,0,0). However, the behavior of the platform is to leave them unchanged. It seems to me it is arguable to go either way. We have to hack some code in WebCore layer to match the behavior in spec. Can anyone give me a signal if I should proceed? Thanks! Btw, Firefox works.

Created attachment 58044 [details] This patch fixes the problem I believe the bug should be fixed. Opera also passes the tests. The question is rather: will it break other apps like Dashboard widgets etc. Perhaps Apple folks who were involved in the original Canvas design and implementation can shed some light on this particular problem? Note that the patch above is still missing the unskipping of the passed tests. I just want to have preliminary review and/or whether we should go on or not.

Attachment 58044 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/html/canvas/CanvasRenderingContext2D.cpp:761: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:762: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:763: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:764: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:1056: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:1057: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:1058: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebCore/html/canvas/CanvasRenderingContext2D.cpp:1059: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 8 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 58044 [details] This patch fixes the problem WebCore/html/canvas/CanvasRenderingContext2D.cpp:762 + state().m_globalComposite == CompositeSourceOut || This is wrong coding style, the || should go on the other side

See also https://bugs.webkit.org/show_bug.cgi?id=39177

Comment on attachment 58044 [details] This patch fixes the problem Clear the review flag, this patch is not completely ready.

*** Bug 48294 has been marked as a duplicate of this bug. ***

Created attachment 92440 [details] Patch

Attachment 92440 [details] did not pass chromium-ews: Output: http://queues.webkit.org/results/8587027 New failing tests: fast/canvas/canvas-composite-alpha.html

Created attachment 92457 [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 on attachment 92440 [details] Patch Clearing the review flag while I investigate the failure.

Created attachment 92671 [details] Updated patch with tests passing

This change has the chance of breaking content designed to work with the CoreGraphics model and might break some WebKit-specific content. Chromium would be happy making this change to match other browsers so long as Apple was planning to make the change. We definitely don't want to diverge.

(In reply to comment #13) > This change has the chance of breaking content designed to work with the CoreGraphics model and might break some WebKit-specific content. Chromium would be happy making this change to match other browsers so long as Apple was planning to make the change. We definitely don't want to diverge. My opinion is that we should make this change, if it turns out to be problematic we can always shove it behind the dashboard compatibility flags -- my assumption is mostly that it's dashboard widgets that are going to rely on such quirks.

Comment on attachment 92671 [details] Updated patch with tests passing View in context: https://bugs.webkit.org/attachment.cgi?id=92671&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945 > + // FIXME: CompositeSourceAtop should follow the same behavior however I am not sure > + // our test coverage is enough. i'm not sure exactly what this FIXME means. should this reference a webkit bug? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:947 > + if (state().m_globalComposite == CompositeSourceIn > + || state().m_globalComposite == CompositeSourceOut) { this should be on one line > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068 > + // FIXME: CompositeSourceAtop should follow the same behavior however I am not sure > + // our test coverage is enough. > + if (state().m_globalComposite == CompositeSourceIn > + || state().m_globalComposite == CompositeSourceOut) { same comments as above > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1077 > + FloatRect canvasRect(0, 0, canvas()->width(), canvas()->height()); > + canvasRect = state().m_transform.inverse().mapRect(canvasRect); > + > + c->save(); > + c->clipOut(enclosingIntRect(rect)); > + c->setCompositeOperation(CompositeClear); > + c->fillRect(canvasRect); > + c->restore(); this smells like bad copy-pasta. can you refactor this into a helper?

Comment on attachment 92671 [details] Updated patch with tests passing View in context: https://bugs.webkit.org/attachment.cgi?id=92671&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:945 >> + // our test coverage is enough. > > i'm not sure exactly what this FIXME means. should this reference a webkit bug? It was more like a gut feeling as philip's test suite does not cover source-atop as it does with source-in or source-out (unless I missed something). I had a quick look and we cover some of source-atop in canvas-composition-alpha.html. I can either squash this change or use a FIXME with a new bug. Do you have a preference? >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:947 >> + || state().m_globalComposite == CompositeSourceOut) { > > this should be on one line Done. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1068 >> + || state().m_globalComposite == CompositeSourceOut) { > > same comments as above Done. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1077 >> + c->restore(); > > this smells like bad copy-pasta. can you refactor this into a helper? OK, I will have to use some template magic as rect and m_path are different types but it makes sense.

Created attachment 93725 [details] Patch

Comment on attachment 93725 [details] Patch Attachment 93725 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8701926 New failing tests: canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html

Created attachment 93728 [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

(In reply to comment #18) > (From update of attachment 93725 [details]) > Attachment 93725 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/8701926 > > New failing tests: > canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html > canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html I looked into those failures and I think this is just the EWS being confused. The diffs reported are the exact diffs that I included in the expected files as part of the patch. Also on a local Chromium build, I don't have the failures.

Jamesr asked me to handle the SourceAtop case as part of this patch. I had a look at our implementation and we are already matching the specification. Thus we don't need to clear the surface contrary to my assumption. I will post a patch without the FIXME for review.

Created attachment 94027 [details] Updated patch - no more SourceAtop reference as it was not needed

Comment on attachment 94027 [details] Updated patch - no more SourceAtop reference as it was not needed Attachment 94027 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8702985 New failing tests: canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html

Created attachment 94031 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 94133 [details] Same as previously with a comment as to why SourceAtop is covered

Comment on attachment 94133 [details] Same as previously with a comment as to why SourceAtop is covered Attachment 94133 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8722018 New failing tests: canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html

Created attachment 94147 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

The baselines look bad - it seems you have either an extra newline or are missing one. Can you generate good text baselines for those tests and re-upload?

Comment on attachment 94133 [details] Same as previously with a comment as to why SourceAtop is covered R- for failing test

(In reply to comment #28) > The baselines look bad - it seems you have either an extra newline or are missing one. Can you generate good text baselines for those tests and re-upload? Good catch, the latest patch is missing the good text baselines (the previous were not). This is due to a bug (bug 61162) in webkit-patch that I hit by mistake when doing some changes to the patch. Basically the newlines in the expected results are just discarded silently :-( Also please ignore the bots' failures as they are hitting the same bug. I will check locally that the baselines work on Chromium linux before manually landing the patch.

Created attachment 94161 [details] Same as previous attachment but with the expected results

Comment on attachment 94161 [details] Same as previous attachment but with the expected results Attachment 94161 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8722053 New failing tests: canvas/philip/tests/2d.composite.uncovered.pattern.source-in.html canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html

Created attachment 94163 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 94161 [details] Same as previous attachment but with the expected results R=me

Comment on attachment 94161 [details] Same as previous attachment but with the expected results View in context: https://bugs.webkit.org/attachment.cgi?id=94161&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1509 > + Is this assert really necessary?

Comment on attachment 94161 [details] Same as previous attachment but with the expected results View in context: https://bugs.webkit.org/attachment.cgi?id=94161&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1509 >> + > > Is this assert really necessary? It is not strictly necessary. This is just a safeguard against misuse and unneeded calls (I am sure we can find a platform where clipping out a path is expensive). I don't feel too strong about it though.

Committed r87307: <http://trac.webkit.org/changeset/87307>

http://trac.webkit.org/changeset/87307 seems to have broken fast/canvas/canvas-composite.html for Chromium/WebKit, see results here: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-composite.html

(In reply to comment #38) > http://trac.webkit.org/changeset/87307 seems to have broken fast/canvas/canvas-composite.html for Chromium/WebKit, see results here: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fcanvas%2Fcanvas-composite.html That's a progression (except for the antaliasing artifacts on source-out). Can you file a new bug about the antialiasing on source-out being broken and suppress? The bug is that we're seeing a 'ring' around the top left part of the circle.