Bug 48290 - Fix LayoutTests/canvas/philip/tests/2d.composite.operation.highlight.html
Summary: Fix LayoutTests/canvas/philip/tests/2d.composite.operation.highlight.html
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:19 PDT by Mike Lawther
Modified: 2011-06-06 00:27 PDT (History)
7 users (show)

See Also:


Attachments
Patch to fix the "highlight" issue as per analysis above (3.98 KB, patch)
2011-06-01 04:05 PDT, Karthik Sarap
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix after build break reported in Chromium (1.90 KB, patch)
2011-06-01 05:48 PDT, Karthik Sarap
no flags Details | Formatted Diff | Diff
Fix after discussion with Darin Adler @ https://lists.webkit.org/pipermail/webkit-dev/2011-June/016911.html (6.32 KB, patch)
2011-06-01 11:33 PDT, Mustafizur Rahaman (rahaman)
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch after discussion with Darin Adler & fixing the build issue in Win/Chromium (7.43 KB, patch)
2011-06-01 13:55 PDT, Mustafizur Rahaman (rahaman)
darin: review+
Details | Formatted Diff | Diff
Incorporating review comments of Darin & Matthew (10.97 KB, patch)
2011-06-02 23:05 PDT, Mustafizur Rahaman (rahaman)
darin: review+
Details | Formatted Diff | Diff
Fix after Darin's comment to rectify the typo (10.97 KB, patch)
2011-06-05 21:38 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:19:27 PDT
This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506
Comment 1 Karthik Sarap 2011-06-01 01:37:30 PDT
    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.
Comment 2 Karthik Sarap 2011-06-01 02:14:18 PDT
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.
Comment 3 Karthik Sarap 2011-06-01 04:05:27 PDT
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 4 WebKit Review Bot 2011-06-01 04:19:02 PDT
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
Comment 5 Karthik Sarap 2011-06-01 05:48:29 PDT
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()
Comment 6 Mustafizur Rahaman (rahaman) 2011-06-01 11:33:25 PDT
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 7 WebKit Review Bot 2011-06-01 11:46:42 PDT
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 8 Mustafizur Rahaman (rahaman) 2011-06-01 13:12:33 PDT
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
Comment 9 Mustafizur Rahaman (rahaman) 2011-06-01 13:55:10 PDT
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 10 Darin Adler 2011-06-01 15:38:46 PDT
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.
Comment 11 Mustafizur Rahaman (rahaman) 2011-06-01 22:12:44 PDT
(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 12 Karthik Sarap 2011-06-02 00:03:33 PDT
Comment on attachment 95588 [details]
Fix after build break reported in Chromium

Cancelling the review of this patch [95588]
Comment 13 Mustafizur Rahaman (rahaman) 2011-06-02 10:20:33 PDT
(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?
Comment 14 Matthew Delaney 2011-06-02 10:34:30 PDT
(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.
Comment 15 Darin Adler 2011-06-02 17:20:08 PDT
Thanks, Matt, that’s exactly what I meant.
Comment 16 Mustafizur Rahaman (rahaman) 2011-06-02 23:05:39 PDT
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.
Comment 17 Mustafizur Rahaman (rahaman) 2011-06-05 11:24:16 PDT
Hi Darin,

Could you please review the patch to see whether I have done everything right this time? Thanks.
Comment 18 Darin Adler 2011-06-05 18:07:11 PDT
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".
Comment 19 Mustafizur Rahaman (rahaman) 2011-06-05 21:38:40 PDT
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 20 Mustafizur Rahaman (rahaman) 2011-06-05 22:19:59 PDT
Comment on attachment 96063 [details]
Fix after Darin's comment to rectify the typo

Can someone please commit the changes?
Comment 21 WebKit Review Bot 2011-06-06 00:27:41 PDT
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>
Comment 22 WebKit Review Bot 2011-06-06 00:27:47 PDT
All reviewed patches have been landed.  Closing bug.