[CSS Shapes] Basic shapes' computed position should be a horizontal and vertical offset

[CSS Shapes] Basic shapes' computed position should be a horizontal and vertical offset

This patch updates the computed position values used for blending to be a horizontal
left offset and a vertical top offset. When positions include a center, bottom, or
right position offset, it is converted to the appropriate top/left coordinate as
a calc expression. Serialized values still use the original bottom/right directions
when present, and also omit the top/left keywords where possible.

A test is added to shape-outside.html since it had no test for interpolation of extended positions.

Patch by Bear Travis.

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

[Bear Travis, 2014-01-23 22:04:41]

Looks solid. Some small nits, the only major concern I have is the test failure.
I would rather see this patch checked in without failing test expectations, or
at least a pointer to a subsequent issue that will fix it.

https://codereview.chromium.org/144373002/diff/40001/LayoutTests/fast/shapes/...
File LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt (right):

https://codereview.chromium.org/144373002/diff/40001/LayoutTests/fast/shapes/...
LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:43: PASS
getComputedStyleValue("shape-inside", "circle(10px at left top 10px)") is
"circle(10px at 0% 10px)"
I don't mind this as an intermediate patch, but the serialization should
eventually replace the 0%, 50%, and 100% keywords where possible.

https://codereview.chromium.org/144373002/diff/40001/LayoutTests/virtual/lega...
File
LayoutTests/virtual/legacy-animations-engine/animations/interpolation/shape-outside-expected.txt
(right):

https://codereview.chromium.org/144373002/diff/40001/LayoutTests/virtual/lega...
LayoutTests/virtual/legacy-animations-engine/animations/interpolation/shape-outside-expected.txt:43:
FAIL: shape-outside from [circle(20% at right 10% bottom 20px)] to [circle(30%
at right 20% bottom 30px)] was [circle(20% at calc(100% - 10%) calc(100% -
20px))] at 0, expected [circle(20% at right 10% bottom 20px)]
I'm a little skeptical of these failures. Do you know why they are failing here?
It's possible that it's using the computed animation value rather than the final
one, but it should be documented, and that it will be fixed with the
serialization patches.

https://codereview.chromium.org/144373002/diff/40001/Source/core/css/BasicSha...
File Source/core/css/BasicShapeFunctions.cpp (right):

https://codereview.chromium.org/144373002/diff/40001/Source/core/css/BasicSha...
Source/core/css/BasicShapeFunctions.cpp:353: if (centerX.direction() ==
BasicShapeCenterCoordinate::TopLeft)
As a nit, this method feels like it's a little too long and repetitive for what
it's doing.
Perhaps using a ternary operator or splitting out the common code into an inline
static function?

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

https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/BasicShapes.cpp:392: 
Extra newline.

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

https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
Source/core/rendering/style/BasicShapes.h:90: const Length& x() const { return
m_x; }
Is this change necessary to this patch? It feels like a slightly separate
concern.

[rwlbuis, 2014-01-24 20:27:56]

On 2014/01/23 22:04:41, Bear Travis wrote:
> Looks solid. Some small nits, the only major concern I have is the test
failure.
> I would rather see this patch checked in without failing test expectations, or
> at least a pointer to a subsequent issue that will fix it.

The fails in the patch may be hard to avoid since the same test is used for
legacy and current animation engine.

>
https://codereview.chromium.org/144373002/diff/40001/LayoutTests/fast/shapes/...
> File LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt
(right):
> 
>
https://codereview.chromium.org/144373002/diff/40001/LayoutTests/fast/shapes/...
> LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:43: PASS
> getComputedStyleValue("shape-inside", "circle(10px at left top 10px)") is
> "circle(10px at 0% 10px)"
> I don't mind this as an intermediate patch, but the serialization should
> eventually replace the 0%, 50%, and 100% keywords where possible.

Sure, this can be fixed in a later patch, should be one of the next CSS shape
patches we do.

>
https://codereview.chromium.org/144373002/diff/40001/LayoutTests/virtual/lega...
> File
>
LayoutTests/virtual/legacy-animations-engine/animations/interpolation/shape-outside-expected.txt
> (right):
> 
>
https://codereview.chromium.org/144373002/diff/40001/LayoutTests/virtual/lega...
>
LayoutTests/virtual/legacy-animations-engine/animations/interpolation/shape-outside-expected.txt:43:
> FAIL: shape-outside from [circle(20% at right 10% bottom 20px)] to [circle(30%
> at right 20% bottom 30px)] was [circle(20% at calc(100% - 10%) calc(100% -
> 20px))] at 0, expected [circle(20% at right 10% bottom 20px)]
> I'm a little skeptical of these failures. Do you know why they are failing
here?
> It's possible that it's using the computed animation value rather than the
final
> one, but it should be documented, and that it will be fixed with the
> serialization patches.

I think the problem is that the same test is used for current and legacy
animation engine. You'll see that we already have fails in the expected results.

>
https://codereview.chromium.org/144373002/diff/40001/Source/core/css/BasicSha...
> File Source/core/css/BasicShapeFunctions.cpp (right):
> 
>
https://codereview.chromium.org/144373002/diff/40001/Source/core/css/BasicSha...
> Source/core/css/BasicShapeFunctions.cpp:353: if (centerX.direction() ==
> BasicShapeCenterCoordinate::TopLeft)
> As a nit, this method feels like it's a little too long and repetitive for
what
> it's doing.
> Perhaps using a ternary operator or splitting out the common code into an
inline
> static function?

You are right, it is a bit verbose, I tried to clean it up.

>
https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
> File Source/core/rendering/style/BasicShapes.cpp (right):
> 
>
https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
> Source/core/rendering/style/BasicShapes.cpp:392: 
> Extra newline.

Fixed.

>
https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
> File Source/core/rendering/style/BasicShapes.h (right):
> 
>
https://codereview.chromium.org/144373002/diff/40001/Source/core/rendering/st...
> Source/core/rendering/style/BasicShapes.h:90: const Length& x() const { return
> m_x; }
> Is this change necessary to this patch? It feels like a slightly separate
> concern.

I did it to reduce the diff to WebKit. But I removed it for now, however we'll
have to do some cleanup patches on both sides to make porting easier.

[rwlbuis, 2014-02-26 16:07:16]

On 2014/01/24 20:27:56, rwlbuis wrote:
> On 2014/01/23 22:04:41, Bear Travis wrote:
> > Looks solid. Some small nits, the only major concern I have is the test
> failure.
> > I would rather see this patch checked in without failing test expectations,
or
> > at least a pointer to a subsequent issue that will fix it.
> 
> The fails in the patch may be hard to avoid since the same test is used for
> legacy and current animation engine.

Note: the legacy animarion engine is removed so we do not longer have this
problem.

[rwlbuis, 2014-02-27 21:26:41]

This patch is about interpolation so I hope I caught the right reviewer :) This
patch is brought over from WebKit and now the legacy animation system is gone it
shows up all green.

[dstockwell, 2014-02-28 01:57:53]

lgtm, but see comment

https://codereview.chromium.org/144373002/diff/340001/LayoutTests/animations/...
File LayoutTests/animations/interpolation/shape-outside.html (right):

https://codereview.chromium.org/144373002/diff/340001/LayoutTests/animations/...
LayoutTests/animations/interpolation/shape-outside.html:97: // TODO: add
intermediate keyframe tests when CSS shapes position computed values are cleaned
up
I think it's better to add these now and check in the FAIL expectations. That
way the fix is forced to clean this up and we can catch any other regressions.

[rwlbuis, 2014-02-28 19:46:23]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-02-28 19:50:09]

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

[commit-bot: I haz the power, 2014-02-28 21:23:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-28 21:23:05]

Retried try job too often on blink_android_compile_dbg for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_andr...

[rwlbuis, 2014-02-28 21:28:28]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-02-28 21:28:35]

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

[commit-bot: I haz the power, 2014-02-28 22:31:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-28 22:31:19]

Retried try job too often on blink_android_compile_rel for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_andr...

[rwlbuis, 2014-02-28 22:42:17]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-02-28 22:43:02]

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

[commit-bot: I haz the power, 2014-02-28 23:49:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-28 23:49:21]

Retried try job too often on blink_android_compile_rel for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_andr...

[rwlbuis, 2014-03-01 00:01:32]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-01 00:01:57]

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

[commit-bot: I haz the power, 2014-03-01 01:22:18]

The CQ bit was unchecked by commit-bot@chromium.org

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

Retried try job too often on blink_android_compile_dbg for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_andr...

[rwlbuis, 2014-03-01 01:52:34]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2014-03-01 01:52:44]

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

[commit-bot: I haz the power, 2014-03-01 01:53:04]

Change committed as 168180