Bug 129840 - [CSS Shapes] polygon default fill-rule should be omitted from the serialization
Summary: [CSS Shapes] polygon default fill-rule should be omitted from the serialization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL:
Keywords:
Depends on:
Blocks: 98664
  Show dependency treegraph
 
Reported: 2014-03-06 15:43 PST by Rebecca Hauck
Modified: 2014-03-14 13:02 PDT (History)
11 users (show)

See Also:


Attachments
Test case for bug (910 bytes, text/html)
2014-03-06 15:43 PST, Rebecca Hauck
no flags Details
Initial Patch (27.35 KB, patch)
2014-03-10 16:23 PDT, Bear Travis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (485.98 KB, application/zip)
2014-03-10 17:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (572.46 KB, application/zip)
2014-03-10 17:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (492.30 KB, application/zip)
2014-03-10 18:20 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (602.19 KB, application/zip)
2014-03-10 18:26 PDT, Build Bot
no flags Details
Updating patch (40.29 KB, patch)
2014-03-11 15:21 PDT, Bear Travis
krit: review+
krit: commit-queue-
Details | Formatted Diff | Diff
Patch with compile assert (37.59 KB, patch)
2014-03-13 11:41 PDT, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rebecca Hauck 2014-03-06 15:43:06 PST
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/
Comment 1 Rebecca Hauck 2014-03-06 15:43:23 PST
Created attachment 226054 [details]
Test case for bug
Comment 2 Rebecca Hauck 2014-03-06 15:44:54 PST
Correction: Test is assumed to be run from test is run from LayoutTests/fast/shapes/parsing
Comment 3 Rob Buis 2014-03-07 11:15:25 PST
I have a Blink fix in the works :

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

Feel free to grab it, or maybe after it lands...
Comment 4 Bear Travis 2014-03-10 16:23:00 PDT
Created attachment 226348 [details]
Initial Patch
Comment 5 Bear Travis 2014-03-10 16:23:36 PDT
Porting Rob's patch mentioned above.
Comment 6 Build Bot 2014-03-10 17:26:01 PDT
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
Comment 7 Build Bot 2014-03-10 17:26:04 PDT
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
Comment 8 Build Bot 2014-03-10 17:36:43 PDT
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
Comment 9 Build Bot 2014-03-10 17:36:46 PDT
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
Comment 10 Build Bot 2014-03-10 18:20:38 PDT
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
Comment 11 Build Bot 2014-03-10 18:20:41 PDT
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
Comment 12 Build Bot 2014-03-10 18:25:57 PDT
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
Comment 13 Build Bot 2014-03-10 18:26:00 PDT
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
Comment 14 Bear Travis 2014-03-11 15:21:12 PDT
Created attachment 226434 [details]
Updating patch

Forgot to update test results after the last merge. This should fix the failing tests.
Comment 15 Darin Adler 2014-03-11 15:41:07 PDT
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?
Comment 16 Darin Adler 2014-03-11 15:41:40 PDT
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?
Comment 17 Darin Adler 2014-03-11 15:41:54 PDT
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?
Comment 18 Darin Adler 2014-03-11 15:41:56 PDT
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?
Comment 19 Darin Adler 2014-03-11 15:41:57 PDT
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?
Comment 20 Darin Adler 2014-03-11 15:42:00 PDT
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?
Comment 21 Darin Adler 2014-03-11 15:42:02 PDT
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?
Comment 22 Darin Adler 2014-03-11 16:27:27 PDT
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?
Comment 23 Dirk Schulze 2014-03-12 02:47:39 PDT
(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? :)
Comment 24 Dirk Schulze 2014-03-12 13:12:32 PDT
Comment on attachment 226434 [details]
Updating patch

r=me with the change request from Darin.
Comment 25 Bear Travis 2014-03-13 11:41:23 PDT
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.
Comment 26 WebKit Commit Bot 2014-03-14 13:01:57 PDT
Comment on attachment 226604 [details]
Patch with compile assert

Clearing flags on attachment: 226604

Committed r165638: <http://trac.webkit.org/changeset/165638>
Comment 27 WebKit Commit Bot 2014-03-14 13:02:04 PDT
All reviewed patches have been landed.  Closing bug.