CSS Exclusions polygon shape arguments should be comma separated

[Bear Travis]

The spec draft http://dev.w3.org/csswg/css3-exclusions/
says that the polygon shape's arguments should be comma separated.
Currently points are space-separated as x,y value pairs.

[Hans Muller]

Created attachment 134623 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 134623 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This change is per the draft exclusions spec, http://dev.w3.org/csswg/css3-exclusions.

This change is not mentioned any where in http://dev.w3.org/csswg/css3-exclusions/#supported-svg-shapes.
Could you clarify why we're making this change. Maybe a link to a public mailing list discussion?

> Source/WebCore/css/CSSParser.cpp:4285
> +    // <length>, <length>, ... <length>, <length> -> each pair has 4 elements except the last one
> +    if (!size || (size % 4) - 3)

Can it have an optional comma at the very end?

[Hans Muller]

(In reply to comment #2)

> > Source/WebCore/css/CSSParser.cpp:4285
> > +    // <length>, <length>, ... <length>, <length> -> each pair has 4 elements except the last one
> > +    if (!size || (size % 4) - 3)
> 
> Can it have an optional comma at the very end?

My interpretation of http://www.w3.org/TR/css3-values/#functional-notation is that a trailing comma is not allowed.

[Hans Muller]

(In reply to comment #2)
> (From update of attachment 134623 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134623&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        This change is per the draft exclusions spec, http://dev.w3.org/csswg/css3-exclusions.
> 
> This change is not mentioned any where in http://dev.w3.org/csswg/css3-exclusions/#supported-svg-shapes.

My understanding was that "<length>{2}#" implied that all arguments were to be comma separated (as  in http://dev.w3.org/csswg/css3-values/#component-multipliers and http://www.w3.org/TR/css3-values/#functional-notation) and Vincent Hardy, one of the spec editors, confirmed as much.   

> Could you clarify why we're making this change. Maybe a link to a public mailing list discussion?

Don't know if this point was discussed on a public forum, looking into that now...

[Ryosuke Niwa]

(In reply to comment #4)
> > Could you clarify why we're making this change. Maybe a link to a public mailing list discussion?
> 
> Don't know if this point was discussed on a public forum, looking into that now...

But this should certainly be mentioned in the spec, right? I'd rather wait until the spec is changed to explicitly state this.

[Hans Muller]

Created attachment 134913 [details]
Patch

[Hans Muller]

(In reply to comment #5)
> (In reply to comment #4)
> > > Could you clarify why we're making this change. Maybe a link to a public mailing list discussion?
> > 
> > Don't know if this point was discussed on a public forum, looking into that now...
> 
> But this should certainly be mentioned in the spec, right? I'd rather wait until the spec is changed to explicitly state this.

Thanks for pushing back on this point, my interpretation of the spec (http://dev.w3.org/csswg/css3-exclusions/#supported-svg-shapes) was incorrect. The spec says that the syntax for polygon is "polygon([<fill-rule>,]? <length>{2}#)", where the hash mark means that each space separated pair of lengths (except for the last one) is followed by a comma (http://dev.w3.org/csswg/css3-values/#component-multipliers).  So an example of the correct polygon syntax is:

   polygon(evenodd, 0px 0px, 10px 100px, 300px 50px);

I've updated the parser and tests to reflect this.

[Ryosuke Niwa]

Comment on attachment 134913 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:4305
> +        else if (commaOrNull->unit != CSSParserValue::Operator || commaOrNull->iValue != ',')

We should really extract an inline helper function for this.

[Hans Muller]

Created attachment 135462 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135462 [details]
Patch

Rejecting attachment 135462 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/webkit-commit-queue/Source/WebKit/chromium/tools/gyp --revision 1293 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
43>At revision 1293.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/12340379

[Hans Muller]

Created attachment 135927 [details]
Patch

[WebKit Review Bot]

Comment on attachment 135927 [details]
Patch

Clearing flags on attachment: 135927

Committed r113400: <http://trac.webkit.org/changeset/113400>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.