Layout support for new circle shape syntax

Layout support for new circle shape syntax

Implement support for doing layout with the new circle shape syntax,
including basic animation support.

Add a few tests for the new circle syntax. The old tests in
LayoutTests/csswg will be removed when the old syntax is removed.

Also update existing shape-inside, animation, and clip-path tests to
test the new syntax.

Patch by Bem Jones-Bey.

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

[Bem Jones-Bey (adobe), 2013-12-19 23:08:47]

lgtm, with a couple of nits:

https://codereview.chromium.org/115253003/diff/40001/Source/core/rendering/st...
File Source/core/rendering/style/BasicShapes.h (right):

https://codereview.chromium.org/115253003/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/BasicShapes.h:146: // FIXME determine how to
interpolate between keywords. See bug 125108.
You could merge in the interpolation patch to this one, or if not, change to a
Blink bug number.

https://codereview.chromium.org/115253003/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/BasicShapes.h:181: // FIXME determine how to
interpolate between keywords. See bug 125108.
Ditto.

[rwlbuis, 2013-12-20 22:37:12]

First trybot shows up green, so submit for review :)

[Bem Jones-Bey (adobe), 2013-12-20 23:55:10]

On 2013/12/20 22:37:12, rwlbuis wrote:
> First trybot shows up green, so submit for review :)

Still lgtm, but just noticed that your BUG= line is empty, you should probably
link it to the shapes syntax tracking bug: 322165.

[rwlbuis, 2013-12-21 00:34:17]

On 2013/12/20 23:55:10, Bem Jones-Bey (adobe) wrote:
> On 2013/12/20 22:37:12, rwlbuis wrote:
> > First trybot shows up green, so submit for review :)
> 
> Still lgtm, but just noticed that your BUG= line is empty, you should probably
> link it to the shapes syntax tracking bug: 322165.

Done!

[leviw_travelin_and_unemployed, 2014-01-07 19:41:33]

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/shapes/Shape.cpp (right):

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/shapes/Shape.cpp:159: float centerX =
floatValueForCenterCoordinate(circle->centerX(), boxWidth);
I don't love having these coordinates split up so much. Instead of the lines
above that say:

float boxWidth = horizontalWritingMode ? logicalBoxSize.width() :
logicalBoxSize.height();
float boxHeight = horizontalWritingMode ? logicalBoxSize.height() :
logicalBoxSize.width();

I'd have used flipForWritingMode to grab a FloatSize. (I realize this new code
is consistent with what's around it)

Do you ever need floatValueForCenterCoordinate for just one direction, or will
it always be for X and Y?

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/BasicShapes.cpp (right):

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/BasicShapes.cpp:147: float
BasicShapeCircle::floatValueForRadiusInBox(float boxWidth, float boxHeight)
const
Why not FloatSize?

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/BasicShapes.h (right):

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/BasicShapes.h:144: bool canBlend(const
BasicShapeCenterCoordinate& other) const
I do love how much this makes me think of willitblend.com

[eseidel, 2014-01-07 19:47:16]

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/BasicShapes.cpp (right):

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/BasicShapes.cpp:54: const BasicShapeCircle*
otherCircle = static_cast<const BasicShapeCircle*>(other);
How do we know that "other" is OK to static_cast?  Should we have a
toBasicShapeCircle(other) helper here (which would ASSERT somethign for us?)

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/BasicShapes.cpp:150: return
floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth + boxHeight *
boxHeight) / 2));
I wonder if we have a hypotenuse() function somewhere?

[leviw_travelin_and_unemployed, 2014-01-07 19:51:55]

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
File Source/core/rendering/style/BasicShapes.cpp (right):

https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
Source/core/rendering/style/BasicShapes.cpp:150: return
floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth + boxHeight *
boxHeight) / 2));
On 2014/01/07 19:47:16, eseidel wrote:
> I wonder if we have a hypotenuse() function somewhere?

cmath has hypot and hypotf

[rwlbuis, 2014-01-07 20:51:14]

On 2014/01/07 19:47:16, eseidel wrote:
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> File Source/core/rendering/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> Source/core/rendering/style/BasicShapes.cpp:54: const BasicShapeCircle*
> otherCircle = static_cast<const BasicShapeCircle*>(other);
> How do we know that "other" is OK to static_cast?  Should we have a
> toBasicShapeCircle(other) helper here (which would ASSERT somethign for us?)

The type() check just above should be enough.

>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> Source/core/rendering/style/BasicShapes.cpp:150: return
> floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth + boxHeight *
> boxHeight) / 2));
> I wonder if we have a hypotenuse() function somewhere?

Good idea, but I think the division by two messes the use up. My replacement
would be this:

hypot(sqrtf(boxWidth * (boxWidth / 2)), sqrtf(boxHeight * (boxHeight / 2)))

I am not sure that is better....
I do wonder if making use of pow would be an improvement in the original.

[rwlbuis, 2014-01-07 21:21:08]

>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> > Source/core/rendering/style/BasicShapes.cpp:150: return
> > floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth + boxHeight
*
> > boxHeight) / 2));
> > I wonder if we have a hypotenuse() function somewhere?
> 
> Good idea, but I think the division by two messes the use up. My replacement
> would be this:
> 
> hypot(sqrtf(boxWidth * (boxWidth / 2)), sqrtf(boxHeight * (boxHeight / 2)))
> 
> I am not sure that is better....
> I do wonder if making use of pow would be an improvement in the original.

Oops, my maths is rusty! Bear pointed out that hypot(boxWidth, boxHeight) /
sqrtf(2) should work, and I verified it does.

[Bem Jones-Bey (adobe), 2014-01-07 21:46:40]

On 2014/01/07 21:21:08, rwlbuis wrote:
> >
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> > > Source/core/rendering/style/BasicShapes.cpp:150: return
> > > floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth +
boxHeight
> *
> > > boxHeight) / 2));
> > > I wonder if we have a hypotenuse() function somewhere?
> > 
> > Good idea, but I think the division by two messes the use up. My replacement
> > would be this:
> > 
> > hypot(sqrtf(boxWidth * (boxWidth / 2)), sqrtf(boxHeight * (boxHeight / 2)))
> > 
> > I am not sure that is better....
> > I do wonder if making use of pow would be an improvement in the original.
> 
> Oops, my maths is rusty! Bear pointed out that hypot(boxWidth, boxHeight) /
> sqrtf(2) should work, and I verified it does.

Probably hypotf instead of hypot, tho?

[rwlbuis, 2014-01-07 22:05:05]

On 2014/01/07 19:41:33, Levi wrote:
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> File Source/core/rendering/shapes/Shape.cpp (right):
> 
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> Source/core/rendering/shapes/Shape.cpp:159: float centerX =
> floatValueForCenterCoordinate(circle->centerX(), boxWidth);
> I don't love having these coordinates split up so much. Instead of the lines
> above that say:
> 
> float boxWidth = horizontalWritingMode ? logicalBoxSize.width() :
> logicalBoxSize.height();
> float boxHeight = horizontalWritingMode ? logicalBoxSize.height() :
> logicalBoxSize.width();
> 
> I'd have used flipForWritingMode to grab a FloatSize. (I realize this new code
> is consistent with what's around it)

Ok, right I just went with what was already there.

> Do you ever need floatValueForCenterCoordinate for just one direction, or will
> it always be for X and Y?

They are always used together, first X calculation and then Y. I think the
method is the way it is right now because this way there is no need to specify
whether the direction is X or Y.

>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> File Source/core/rendering/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> Source/core/rendering/style/BasicShapes.cpp:147: float
> BasicShapeCircle::floatValueForRadiusInBox(float boxWidth, float boxHeight)
> const
> Why not FloatSize?

No particular reason, I just thought it may be a bit slower to use FloatSize.

[rwlbuis, 2014-01-07 22:05:19]

On 2014/01/07 21:46:40, Bem Jones-Bey (adobe) wrote:
> On 2014/01/07 21:21:08, rwlbuis wrote:
> > >
> >
>
https://codereview.chromium.org/115253003/diff/120001/Source/core/rendering/s...
> > > > Source/core/rendering/style/BasicShapes.cpp:150: return
> > > > floatValueForLength(m_radius.value(), sqrtf((boxWidth * boxWidth +
> boxHeight
> > *
> > > > boxHeight) / 2));
> > > > I wonder if we have a hypotenuse() function somewhere?
> > > 
> > > Good idea, but I think the division by two messes the use up. My
replacement
> > > would be this:
> > > 
> > > hypot(sqrtf(boxWidth * (boxWidth / 2)), sqrtf(boxHeight * (boxHeight /
2)))
> > > 
> > > I am not sure that is better....
> > > I do wonder if making use of pow would be an improvement in the original.
> > 
> > Oops, my maths is rusty! Bear pointed out that hypot(boxWidth, boxHeight) /
> > sqrtf(2) should work, and I verified it does.
> 
> Probably hypotf instead of hypot, tho?

Probably :)

[leviw_travelin_and_unemployed, 2014-01-08 23:07:42]

lgtm

https://codereview.chromium.org/115253003/diff/430001/Source/core/css/BasicSh...
File Source/core/css/BasicShapeFunctions.cpp (right):

https://codereview.chromium.org/115253003/diff/430001/Source/core/css/BasicSh...
Source/core/css/BasicShapeFunctions.cpp:364: switch (centerX.keyword()) {
I guess what I was imagining was this single accessor that took a FloatSize like
you have here since the function is always called with a pair of coordinates,
and it could call out to a static function that does the logic you're
duplicating here.

Anyways this is okay.

[commit-bot: I haz the power, 2014-01-09 00:15:18]

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

[commit-bot: I haz the power, 2014-01-09 03:03:19]

Change committed as 164731