Migrate the remaining CSS properties interpolable as Lengths to LengthInterpolationType

Migrate the remaining CSS properties interpolable as Lengths to LengthInterpolationType

This patch is part of a refactor away from using StyleInterpolation subclasses
to using per property InterpolationType singletons.
This migrates uses of LengthStyleInterpolation over to LengthInterpolationType for
all length properties except line-height and font-size.

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

[alancutter (OOO until 2018), 2015-07-22 05:07:56]

alancutter@chromium.org changed reviewers:
+ dstockwell@chromium.org, shans@chromium.org

[alancutter (OOO until 2018), 2015-07-22 05:07:57]

This patch requires extensive additional testing for neutral, initial, inherit
and unset code paths. I intend to land a separate patch with these tests
covering all interpolable properties before landing this change.

[alancutter (OOO until 2018), 2015-07-22 05:08:09]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-22 05:08:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243313002/1

[commit-bot: I haz the power, 2015-07-22 06:53:58]

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

[commit-bot: I haz the power, 2015-07-22 06:53:58]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2015-07-27 01:23:47]

This patch is ready for code review, tests are the only thing still to be done.
PTAL.

[dstockwell, 2015-08-31 05:33:40]

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthPropertyFunctions.cpp (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:55: bool
LengthPropertyFunctions::getPixelsForKeyword(CSSPropertyID property, CSSValueID
valueID, double& result)
Do we usually say 'pixels' or 'px'?

Why do we only support pixels for keywords? Is it just that all the existing
keywords are in pixels?

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:59: if (valueID ==
CSSValueBaseline) {
This looks painful to maintain, how can we keep it in sync?

Maybe we should write a guide on animation considerations for changes in css?

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:109: case
CSSPropertyBorderBottomWidth:
Why do we need to treat these differently?

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:279: // 0 represents the
"none" keyword.
We should add a style.hasPerspective and assert in perspective() that !0
(separately).

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:285:
ASSERT(!hasZoomedLength(CSSPropertyStrokeWidth));
Why do we assert this?

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:303: bool
LengthPropertyFunctions::setLength(CSSPropertyID property, ComputedStyle& style,
const Length& value)
Why did we move away from the setter based approach?

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthPropertyFunctions.h (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.h:21: static bool
hasZoomedLength(CSSPropertyID);
has -> is? I imagine there's only one length per property.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthStyleInterpolation.cpp (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthStyleInterpolation.cpp:165: #if ENABLE(ASSERT)
Seeing this the second time, I think this assert should have a comment.

[alancutter (OOO until 2018), 2015-08-31 07:15:30]

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthPropertyFunctions.cpp (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:55: bool
LengthPropertyFunctions::getPixelsForKeyword(CSSPropertyID property, CSSValueID
valueID, double& result)
On 2015/08/31 at 05:33:39, dstockwell wrote:
> Do we usually say 'pixels' or 'px'?
> 
> Why do we only support pixels for keywords? Is it just that all the existing
keywords are in pixels?

All the existing keywords are in pixels.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:59: if (valueID ==
CSSValueBaseline) {
On 2015/08/31 at 05:33:39, dstockwell wrote:
> This looks painful to maintain, how can we keep it in sync?
> 
> Maybe we should write a guide on animation considerations for changes in css?

We could expose this function to StyleBuilderConverter to avoid duplication of
logic, the only issue is where the code lives might be awkward. I suspect the
Compartmentalisation of CSS Properties will provide adequate housing for logic
like this.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:109: case
CSSPropertyBorderBottomWidth:
On 2015/08/31 at 05:33:39, dstockwell wrote:
> Why do we need to treat these differently?

See above comment.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:279: // 0 represents the
"none" keyword.
On 2015/08/31 at 05:33:39, dstockwell wrote:
> We should add a style.hasPerspective and assert in perspective() that !0
(separately).

Turns out hasPerspective() already exists. Adding the assert in perspective
would be a separate patch.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:285:
ASSERT(!hasZoomedLength(CSSPropertyStrokeWidth));
On 2015/08/31 at 05:33:39, dstockwell wrote:
> Why do we assert this?

style.strokeWidth() returns an UnzoomedLength from which we pull out a Length.
The assert is to justify dropping the Unzoomed part since we account for the
non-zoomed nature of the value in other parts of the API.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:303: bool
LengthPropertyFunctions::setLength(CSSPropertyID property, ComputedStyle& style,
const Length& value)
On 2015/08/31 at 05:33:39, dstockwell wrote:
> Why did we move away from the setter based approach?

Windows was getting crazy pants runtime crashes that seemed to be a result of
using function pointers across compilation units. I didn't fully investigate it
because I can't use function pointers for line-height anyway (later patch).

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthPropertyFunctions.h (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.h:21: static bool
hasZoomedLength(CSSPropertyID);
On 2015/08/31 at 05:33:40, dstockwell wrote:
> has -> is? I imagine there's only one length per property.

Done.

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthStyleInterpolation.cpp (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthStyleInterpolation.cpp:165: #if ENABLE(ASSERT)
On 2015/08/31 at 05:33:40, dstockwell wrote:
> Seeing this the second time, I think this assert should have a comment.

Done.

[alancutter (OOO until 2018), 2015-08-31 07:20:51]

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
File Source/core/animation/LengthPropertyFunctions.cpp (right):

https://codereview.chromium.org/1243313002/diff/60001/Source/core/animation/L...
Source/core/animation/LengthPropertyFunctions.cpp:109: case
CSSPropertyBorderBottomWidth:
On 2015/08/31 at 07:15:30, alancutter wrote:
> On 2015/08/31 at 05:33:39, dstockwell wrote:
> > Why do we need to treat these differently?
> 
> See above comment.

Updated comment to not have grammatical error.

[dstockwell, 2015-09-14 00:37:43]

lgtm

[alancutter (OOO until 2018), 2015-09-14 06:02:08]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-14 06:02:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243313002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243313002/140001

[commit-bot: I haz the power, 2015-09-14 07:18:48]

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

[commit-bot: I haz the power, 2015-09-14 07:18:49]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2015-09-17 01:35:16]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2015-09-17 01:35:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1243313002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1243313002/140001

[commit-bot: I haz the power, 2015-09-17 02:55:40]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202418