Implement parsing of the new ellipse shape syntax.

Implement parsing of the new ellipse shape syntax.

    This closely follows the patch for the new circle syntax,  
    with some refactoring of functionality shared by both.

    Updated existing parsing tests to cover this.

BUG=322165
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164465

[bemjb, 2013-12-07 00:05:32]

Thanks for porting this; you need to update the description and title; the
description says just ellipses, it should mention that this is for circles as
well. Also, would be nice to rewrite the description to be one set of prose,
it's a bit hard to read as two independent change descriptions.

https://codereview.chromium.org/103413006/diff/1/LayoutTests/fast/masking/par...
File LayoutTests/fast/masking/parsing-clip-path-shape.html (right):

https://codereview.chromium.org/103413006/diff/1/LayoutTests/fast/masking/par...
LayoutTests/fast/masking/parsing-clip-path-shape.html:100:
testInner("-webkit-clip-path", "ellipse(100%, 100%, 100%, 100%)", "ellipse(100%,
100%, 100%, 100%)");
You should have some tests for clip-path with the new ellipse syntax, just like
there are tests for the new circle syntax.

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.cpp (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.cpp:117: result.reserveCapacity((sizeof(opening)
- 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) +
centerX.length() + centerY.length());
It looks like you didn't pull the latest patch: I'd expect this reserveCapacity
call to be removed, and the "append" for the literals below to be appendLiteral.

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
File Source/core/css/CSSBasicShapes.h (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
Source/core/css/CSSBasicShapes.h:50: CSSBasicShapeCircleType = 7
Should you remove the explicit numbers in this patch, or did you want to follow
up with that later?

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSParser-in...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSParser-in...
Source/core/css/CSSParser-in.cpp:5175: if (value->id == CSSValueClosestSide &&
value->id == CSSValueFarthestSide)
This should be a ||, not &&.

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
File Source/core/rendering/shapes/Shape.cpp (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/Shape.cpp:149: // FIXME implement layout. bug
124619
This is the webkit bug number, Either remove the number or file a Blink bug for
this.

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
Source/core/rendering/shapes/Shape.cpp:168: // FIXME implement layout. bug
124619
Ditto.

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/BasicShapes.cpp (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/BasicShapes.cpp:115: // FIXME Complete
implementation of path. Bug 124619.
Again, replace webkit bug with Blink bug #.

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/BasicShapes.cpp:185: // FIXME Complete
implementation of path. Bug 124619.
Ditto.

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
File Source/core/rendering/style/BasicShapes.h (right):

https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
Source/core/rendering/style/BasicShapes.h:55: BasicShapeCircleType = 7
Do you want to remove these explicit numbers in this patch?

[rwlbuis, 2013-12-09 16:46:28]

On 2013/12/07 00:05:32, bemjb wrote:
> Thanks for porting this; you need to update the description and title; the
> description says just ellipses, it should mention that this is for circles as
> well. Also, would be nice to rewrite the description to be one set of prose,
> it's a bit hard to read as two independent change descriptions.

This patch was more of a backup upload so I don't lose this work. It is actually
circle + ellipse part of the work squashed.
The circle part (where I credit you) is here:

https://codereview.chromium.org/98723006/

>
https://codereview.chromium.org/103413006/diff/1/LayoutTests/fast/masking/par...
> File LayoutTests/fast/masking/parsing-clip-path-shape.html (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/LayoutTests/fast/masking/par...
> LayoutTests/fast/masking/parsing-clip-path-shape.html:100:
> testInner("-webkit-clip-path", "ellipse(100%, 100%, 100%, 100%)",
"ellipse(100%,
> 100%, 100%, 100%)");
> You should have some tests for clip-path with the new ellipse syntax, just
like
> there are tests for the new circle syntax.

Ok.

>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
> File Source/core/css/CSSBasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
> Source/core/css/CSSBasicShapes.cpp:117:
result.reserveCapacity((sizeof(opening)
> - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) +
> centerX.length() + centerY.length());
> It looks like you didn't pull the latest patch: I'd expect this
reserveCapacity
> call to be removed, and the "append" for the literals below to be
appendLiteral.

Good point, will fix.

>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
> File Source/core/css/CSSBasicShapes.h (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSBasicShap...
> Source/core/css/CSSBasicShapes.h:50: CSSBasicShapeCircleType = 7
> Should you remove the explicit numbers in this patch, or did you want to
follow
> up with that later?

Good suggestion, will do.

>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSParser-in...
> File Source/core/css/CSSParser-in.cpp (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/css/CSSParser-in...
> Source/core/css/CSSParser-in.cpp:5175: if (value->id == CSSValueClosestSide &&
> value->id == CSSValueFarthestSide)
> This should be a ||, not &&.

Good catch.

>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
> File Source/core/rendering/shapes/Shape.cpp (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
> Source/core/rendering/shapes/Shape.cpp:149: // FIXME implement layout. bug
> 124619
> This is the webkit bug number, Either remove the number or file a Blink bug
for
> this.

I think we can remove it, another patch we'll port will fix this one too.

>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/shapes...
> Source/core/rendering/shapes/Shape.cpp:168: // FIXME implement layout. bug
> 124619
> Ditto.
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
> File Source/core/rendering/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/BasicShapes.cpp:115: // FIXME Complete
> implementation of path. Bug 124619.
> Again, replace webkit bug with Blink bug #.
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/BasicShapes.cpp:185: // FIXME Complete
> implementation of path. Bug 124619.
> Ditto.
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
> File Source/core/rendering/style/BasicShapes.h (right):
> 
>
https://codereview.chromium.org/103413006/diff/1/Source/core/rendering/style/...
> Source/core/rendering/style/BasicShapes.h:55: BasicShapeCircleType = 7
> Do you want to remove these explicit numbers in this patch?

Yes, will do.

[Bem Jones-Bey (adobe), 2013-12-19 21:42:49]

lgtm

[rwlbuis, 2013-12-19 21:54:41]

eseidel@chromium.org: this should be straightforward as it is like
https://codereview.chromium.org/98723006/, but for ellipses. 

Original WebKit patch here: https://bugs.webkit.org/show_bug.cgi?id=124620

[rwlbuis, 2013-12-20 23:05:44]

On 2013/12/19 21:54:41, rwlbuis wrote:
> eseidel@chromium.org: this should be straightforward as it is like
> https://codereview.chromium.org/98723006/, but for ellipses. 
> 
> Original WebKit patch here: https://bugs.webkit.org/show_bug.cgi?id=124620

I don't think the win_blink redness is caused by this patch, see for instance:

http://build.chromium.org/p/tryserver.chromium/builders/win_blink/builds/1078

[rwlbuis, 2014-01-02 21:20:21]

Rebased after the CSSParser rename. Bots look green (I don't expect win_blink to
be, but it seems it has a problem anyway and everybody ignores it).

[krit, 2014-01-03 20:15:20]

On 2014/01/02 21:20:21, rwlbuis wrote:
> Rebased after the CSSParser rename. Bots look green (I don't expect win_blink
to
> be, but it seems it has a problem anyway and everybody ignores it).

lgtm too

[commit-bot: I haz the power, 2014-01-03 20:17:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rob.buis@samsung.com/103413006/100001

[commit-bot: I haz the power, 2014-01-03 21:48:27]

Change committed as 164465