This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506
I was going through the bug https://bugs.webkit.org/show_bug.cgi?id=48290, which passes is FF but fails in Safari/Winlauncher( r87771) As per http://www.w3.org/TR/2011/WD-2dcontext-20110525/#compositing, "highlight" is NOT a valid composite operation. but it is being mentioned in platform/graphics/GraphicsTypes.cpp in compositeOperatorNames[] & that's why this issue is present. When I removed "highlight" from here & also removed "CompositeHighLight from enum CompositeOperator in GraphicsType.h this issue is resolved (I also had to comment out all other code using CompositeHighLight & CSSValueHighlight. However, the following comment was mentioned in GraphicsType.h (" // Note: These constants exactly match the NSCompositeOperator constants of // AppKit on Mac OS X Tiger. If these ever change, we'll need to change the // Mac OS X Tiger platform code to map one to the other. "). So I am little unclear what is the purpose of this CompositeHighLight.
http://philip.html5.org/tests/canvas/suite/tests/2d.composite.operation.highlight.html passes in FF but fails in Safari/Winlauncher( r87771) As per http://www.w3.org/TR/2011/WD-2dcontext-20110525/#compositing, "highlight" is NOT a valid composite operation. but it is being mentioned in platform/graphics/GraphicsTypes.cpp in compositeOperatorNames[] & that's why this issue is present. When I removed "highlight" from here & also removed "CompositeHighLight from enum CompositeOperator in GraphicsType.h this issue is resolved (I also had to comment out all other code using CompositeHighLight & CSSValueHighlight. However, the following comment was mentioned in GraphicsType.h (" // Note: These constants exactly match the NSCompositeOperator constants of // AppKit on Mac OS X Tiger. If these ever change, we'll need to change the // Mac OS X Tiger platform code to map one to the other. "). So I am little unclear what is the purpose of this CompositeHighLight.
Created attachment 95580 [details] Patch to fix the "highlight" issue as per analysis above "highlight" is not a valid globalComposite operation as per http://www.w3.org/TR/2011/WD-2dcontext-20110525/#compositing, but the code was still supporting "highlight". So, I removed "highlight" & all the related code.
Comment on attachment 95580 [details] Patch to fix the "highlight" issue as per analysis above Attachment 95580 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8753588
Created attachment 95588 [details] Fix after build break reported in Chromium I have made an alternate/simpler fix after my previous fix broke the Chromium build. In stead of removing "highlight"/CompositeHighlight we are just checking for "highlight" in CanvasRenderingContext2D::setGlobalCompositeOperation() & returning false so that it does not go to parseCompositeOperator()
Created attachment 95634 [details] Fix after discussion with Darin Adler @ https://lists.webkit.org/pipermail/webkit-dev/2011-June/016911.html Please ignore the previous patch (id=95588). We will make that Obsolete. Removed "highlight" & CompositeHighlight after discussion with Darin Adler as these are no longer used anywhere.
Comment on attachment 95634 [details] Fix after discussion with Darin Adler @ https://lists.webkit.org/pipermail/webkit-dev/2011-June/016911.html Attachment 95634 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8757214
Comment on attachment 95634 [details] Fix after discussion with Darin Adler @ https://lists.webkit.org/pipermail/webkit-dev/2011-June/016911.html I will submit another patch with the chromium build issue fixed
Created attachment 95655 [details] Patch after discussion with Darin Adler & fixing the build issue in Win/Chromium The patch contains removing the "highlight" & CompositeHighlight option as well.
Comment on attachment 95655 [details] Patch after discussion with Darin Adler & fixing the build issue in Win/Chromium View in context: https://bugs.webkit.org/attachment.cgi?id=95655&action=review > Source/WebCore/ChangeLog:13 > + Tests: No new tests required. > + LayoutTests\canvas\philip\tests\2d.composite.operation.highlight.html can be used for testing. I don’t understand why this patch doesn’t include new passing test results for that test.
(In reply to comment #10) > (From update of attachment 95655 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95655&action=review > > > Source/WebCore/ChangeLog:13 > > + Tests: No new tests required. > > + LayoutTests\canvas\philip\tests\2d.composite.operation.highlight.html can be used for testing. > > I don’t understand why this patch doesn’t include new passing test results for that test. I did not completely understand what you meant by the above statement. The existing expected result include "Passed", that's why I did not add any new test case/update the test result. Do you want me to fix this Or, would someone commit these changes?
Comment on attachment 95588 [details] Fix after build break reported in Chromium Cancelling the review of this patch [95588]
(In reply to comment #10) > (From update of attachment 95655 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95655&action=review > > > Source/WebCore/ChangeLog:13 > > + Tests: No new tests required. > > + LayoutTests\canvas\philip\tests\2d.composite.operation.highlight.html can be used for testing. > > I don’t understand why this patch doesn’t include new passing test results for that test. I could not completely understand the above. The test case is already existing in WebKit repo, therefore I did not add any test case. Where should I include the test result?
(In reply to comment #13) > (In reply to comment #10) > > (From update of attachment 95655 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=95655&action=review > > > > > Source/WebCore/ChangeLog:13 > > > + Tests: No new tests required. > > > + LayoutTests\canvas\philip\tests\2d.composite.operation.highlight.html can be used for testing. > > > > I don’t understand why this patch doesn’t include new passing test results for that test. > > I could not completely understand the above. The test case is already existing in WebKit repo, therefore I did not add any test case. Where should I include the test result? If this change makes all ports pass this test, then you should remove the test from all ports' Skipped lists. For example, one such Skipped list is at LayoutTests/platform/mac/Skipped. I checked and this test is on that skipped list, and thus won't be run. It's also best to remove any expected failing results files under LayoutTests/platform for all ports that may have been running the test before (i.e. not on their Skipped lists) but expecting it to fail.
Thanks, Matt, that’s exactly what I meant.
Created attachment 95855 [details] Incorporating review comments of Darin & Matthew I have removed this particular tests from the Chromium/test_expectations.txt [expected to fail] I have also removed the test case from the Skipped list of GTK, QT & Mac.
Hi Darin, Could you please review the patch to see whether I have done everything right this time? Thanks.
Comment on attachment 95855 [details] Incorporating review comments of Darin & Matthew View in context: https://bugs.webkit.org/attachment.cgi?id=95855&action=review > LayoutTests/ChangeLog:7 > + from failied/skipped test list Typo: "failied".
Created attachment 96063 [details] Fix after Darin's comment to rectify the typo Fixed the typo mentioned by Darin. Darin, could you please review it once. I hope everything is right this time :-)
Comment on attachment 96063 [details] Fix after Darin's comment to rectify the typo Can someone please commit the changes?
Comment on attachment 96063 [details] Fix after Darin's comment to rectify the typo Clearing flags on attachment: 96063 Committed r88144: <http://trac.webkit.org/changeset/88144>
All reviewed patches have been landed. Closing bug.