Bug 48290 – Fix LayoutTests/canvas/philip/tests/2d.composite.operation.highlight.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

[Karthik Sarap]

    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.

[Karthik Sarap]

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.

[Karthik Sarap]

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.

[WebKit Review Bot]

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

[Karthik Sarap]

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()

[Mustafizur Rahaman (rahaman)]

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.

[WebKit Review Bot]

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

[Mustafizur Rahaman (rahaman)]

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

[Mustafizur Rahaman (rahaman)]

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.

[Darin Adler]

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.

[Mustafizur Rahaman (rahaman)]

(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?

[Karthik Sarap]

Comment on attachment 95588 [details]
Fix after build break reported in Chromium

Cancelling the review of this patch [95588]

[Mustafizur Rahaman (rahaman)]

(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?

[Matthew Delaney]

(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.

[Darin Adler]

Thanks, Matt, that’s exactly what I meant.

[Mustafizur Rahaman (rahaman)]

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.

[Mustafizur Rahaman (rahaman)]

Hi Darin,

Could you please review the patch to see whether I have done everything right this time? Thanks.

[Darin Adler]

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".

[Mustafizur Rahaman (rahaman)]

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 :-)

[Mustafizur Rahaman (rahaman)]

Comment on attachment 96063 [details]
Fix after Darin's comment to rectify the typo

Can someone please commit the changes?

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.