Bug 129840 – [CSS Shapes] polygon default fill-rule should be omitted from the serialization

• Component: CSS
• Status: RESOLVED FIXED

[Rebecca Hauck]

The attached test file tests:

polygon(1px 2px, 3px 4px, 5px 6px)

Which should serialize to the same string.

However, the default fill-rule is added to the serialized string:

polygon(nonzero, 1px 2px, 3px 4px, 5px 6px)


Note: This uses testharness.js and the relative path in the test assumes the test is run from LayoutTests/fast/shapes/

[Rebecca Hauck]

Created attachment 226054 [details]
Test case for bug

[Rebecca Hauck]

Correction: Test is assumed to be run from test is run from LayoutTests/fast/shapes/parsing

[Rob Buis]

I have a Blink fix in the works :

https://codereview.chromium.org/191353002/

Feel free to grab it, or maybe after it lands...

[Bear Travis]

Created attachment 226348 [details]
Initial Patch

[Bear Travis]

Porting Rob's patch mentioned above.

[Build Bot]

Comment on attachment 226348 [details]
Initial Patch

Attachment 226348 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6708807400947712

New failing tests:
fast/shapes/parsing/parsing-shape-outside.html
fast/shapes/parsing/parsing-shape-inside.html

[Build Bot]

Created attachment 226352 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226348 [details]
Initial Patch

Attachment 226348 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5471260892987392

New failing tests:
fast/shapes/parsing/parsing-shape-outside.html
fast/shapes/parsing/parsing-shape-inside.html

[Build Bot]

Created attachment 226353 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226348 [details]
Initial Patch

Attachment 226348 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4793961730277376

New failing tests:
fast/shapes/parsing/parsing-shape-outside.html
fast/shapes/parsing/parsing-shape-inside.html

[Build Bot]

Created attachment 226360 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226348 [details]
Initial Patch

Attachment 226348 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6482811590541312

New failing tests:
fast/shapes/parsing/parsing-shape-outside.html
fast/shapes/parsing/parsing-shape-inside.html

[Build Bot]

Created attachment 226362 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Bear Travis]

Created attachment 226434 [details]
Updating patch

Forgot to update test results after the last merge. This should fix the failing tests.

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:-335
> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);

How about changing this to >= rather than dumping the assertion entirely?

[Darin Adler]

Comment on attachment 226434 [details]
Updating patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review

>>>>>>>> Source/WebCore/css/CSSBasicShapes.cpp:-335
>>>>>>>> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);
>>>>>>> 
>>>>>>> How about changing this to >= rather than dumping the assertion entirely?
>>>>>> 
>>>>>> How about changing this to >= rather than dumping the assertion entirely?
>>>>> 
>>>>> How about changing this to >= rather than dumping the assertion entirely?
>>>> 
>>>> How about changing this to >= rather than dumping the assertion entirely?
>>> 
>>> How about changing this to >= rather than dumping the assertion entirely?
>> 
>> How about changing this to >= rather than dumping the assertion entirely?
> 
> How about changing this to >= rather than dumping the assertion entirely?

How about changing this to >= rather than dumping the assertion entirely?

[Dirk Schulze]

(In reply to comment #22)
> (From update of attachment 226434 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226434&action=review
> 
> >>>>>>>> Source/WebCore/css/CSSBasicShapes.cpp:-335
> >>>>>>>> -    COMPILE_ASSERT(sizeof(evenOddOpening) == sizeof(nonZeroOpening), polygon_string_openings_have_same_length);
> >>>>>>> 
> >>>>>>> How about changing this to >= rather than dumping the assertion entirely?
> >>>>>> 
> >>>>>> How about changing this to >= rather than dumping the assertion entirely?
> >>>>> 
> >>>>> How about changing this to >= rather than dumping the assertion entirely?
> >>>> 
> >>>> How about changing this to >= rather than dumping the assertion entirely?
> >>> 
> >>> How about changing this to >= rather than dumping the assertion entirely?
> >> 
> >> How about changing this to >= rather than dumping the assertion entirely?
> > 
> > How about changing this to >= rather than dumping the assertion entirely?
> 
> How about changing this to >= rather than dumping the assertion entirely?

You are really serious about the assertion, aren't you? :)

[Dirk Schulze]

Comment on attachment 226434 [details]
Updating patch

r=me with the change request from Darin.

[Bear Travis]

Created attachment 226604 [details]
Patch with compile assert

Adding the suggested compile assert that evenodd polygon string openings will be longer than those for nonzero polygon strings.

[WebKit Commit Bot]

Comment on attachment 226604 [details]
Patch with compile assert

Clearing flags on attachment: 226604

Committed r165638: <http://trac.webkit.org/changeset/165638>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.