Move static methods out of CSSLengthInterpolationType to LengthInterpolationFunctions

Move static methods out of CSSLengthInterpolationType to LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.

Committed: https://crrev.com/c31809296fc253caeb3a34dc3a05c0c96d3f9209
Cr-Commit-Position: refs/heads/master@{#416877}

[alancutter (OOO until 2018), 2016-08-26 12:42:31]

Description was changed from

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.
==========

to

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.

As an extra clean up createCSSValue() is removed in favour of using an
existing CSSPrimitiveValue::create() method.
==========

[alancutter (OOO until 2018), 2016-08-26 12:42:31]

alancutter@chromium.org changed reviewers:
+ suzyh@chromium.org

[alancutter (OOO until 2018), 2016-08-26 13:36:19]

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

[commit-bot: I haz the power, 2016-08-26 13:36:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 14:56:38]

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

[commit-bot: I haz the power, 2016-08-26 14:56:39]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-08-29 07:12:31]

On the whole, I'm fine with this patch, with two caveats and one apology:

I haven't reviewed the createCSSValue / apply change. Please pull that out to a
separate patch.

I haven't compared the moved functions line-by-line to ensure they're identical.
I assume you copy-pasted with essentially no changes.

I found you some yaks to shave. Sorry about that.

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp
(right):

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:110:
void CSSLengthInterpolationType::apply(const InterpolableValue&
interpolableValue, const NonInterpolableValue* nonInterpolableValue,
InterpolationEnvironment& environment) const
Can you make the change to this function (and associated removal of
createCSSValue) in a separate patch so it's clear they are independent changes?

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:117: //
Assert that setting the length on ComputedStyle directly is identical to the
StyleBuilder code path.
And in that separate patch, since you're already touching this DCHECK branch,
can you either

(a) update crbug.com/630487 with any changed info resulting from you touching
it, and optionally add a TODO referencing the bug, or

(b) fix crbug.com/630487,

whichever makes most sense?

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp
(right):

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp:109:
return true;
Looks like some further refactoring is needed as this no longer makes sense.

IIUC, "nonInterpolableValuesAreCompatible" is defined/used for ShadowNonInt'Val
and CSSLengthNonInt'Val and then the ListInterpolationFunctions. Either this
needs to be a function defined on NonInterpolableValue, inherited and overridden
only when needed; or these three usages need to be pulled out to yet another
separate library.

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp:143:
bool hasPercentage =
CSSLengthNonInterpolableValue::hasPercentage(nonInterpolableValue);
Why is hasPercentage a static function instead
nonInterpolableValue.hasPercentage()?

[alancutter (OOO until 2018), 2016-09-05 07:40:22]

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp
(right):

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:110:
void CSSLengthInterpolationType::apply(const InterpolableValue&
interpolableValue, const NonInterpolableValue* nonInterpolableValue,
InterpolationEnvironment& environment) const
On 2016/08/29 at 07:12:31, suzyh wrote:
> Can you make the change to this function (and associated removal of
createCSSValue) in a separate patch so it's clear they are independent changes?

Done. https://codereview.chromium.org/2314453002

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/CSSLengthInterpolationType.cpp:117: //
Assert that setting the length on ComputedStyle directly is identical to the
StyleBuilder code path.
On 2016/08/29 at 07:12:31, suzyh wrote:
> And in that separate patch, since you're already touching this DCHECK branch,
can you either
> 
> (a) update crbug.com/630487 with any changed info resulting from you touching
it, and optionally add a TODO referencing the bug, or
> 
> (b) fix crbug.com/630487,
> 
> whichever makes most sense?

Did B in https://codereview.chromium.org/2314453002.

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp
(right):

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp:109:
return true;
On 2016/08/29 at 07:12:31, suzyh wrote:
> Looks like some further refactoring is needed as this no longer makes sense.
> 
> IIUC, "nonInterpolableValuesAreCompatible" is defined/used for
ShadowNonInt'Val and CSSLengthNonInt'Val and then the
ListInterpolationFunctions. Either this needs to be a function defined on
NonInterpolableValue, inherited and overridden only when needed; or these three
usages need to be pulled out to yet another separate library.

Added type checks as discussed in https://codereview.chromium.org/2314443002.

https://codereview.chromium.org/2283783002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/animation/LengthInterpolationFunctions.cpp:143:
bool hasPercentage =
CSSLengthNonInterpolableValue::hasPercentage(nonInterpolableValue);
On 2016/08/29 at 07:12:31, suzyh wrote:
> Why is hasPercentage a static function instead
nonInterpolableValue.hasPercentage()?

It is false if nonInterpolableValue is nullptr. Calling methods on null pointers
is undefined behaviour (it'd probably work fine but wouldn't be healthy code).

[suzyh_UTC10 (ex-contributor), 2016-09-06 00:24:51]

lgtm

[alancutter (OOO until 2018), 2016-09-06 00:27:57]

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

[commit-bot: I haz the power, 2016-09-06 00:28:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 02:14:03]

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

[commit-bot: I haz the power, 2016-09-06 02:14:04]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-09-06 03:11:09]

On 2016/09/06 at 00:24:51, suzyh wrote:
> lgtm

Oh, update the CL description since you moved some of it to another patch.

[alancutter (OOO until 2018), 2016-09-06 03:18:52]

Description was changed from

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.

As an extra clean up createCSSValue() is removed in favour of using an
existing CSSPrimitiveValue::create() method.
==========

to

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.
==========

[alancutter (OOO until 2018), 2016-09-06 03:19:01]

On 2016/09/06 at 03:11:09, suzyh wrote:
> On 2016/09/06 at 00:24:51, suzyh wrote:
> > lgtm
> 
> Oh, update the CL description since you moved some of it to another patch.

Good catch, done.

[alancutter (OOO until 2018), 2016-09-07 05:32:08]

The CQ bit was checked by alancutter@chromium.org

[commit-bot: I haz the power, 2016-09-07 05:32:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 05:34:09]

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

[commit-bot: I haz the power, 2016-09-07 05:34:10]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[alancutter (OOO until 2018), 2016-09-07 05:49:41]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2016-09-07 05:49:42]

The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org
Link to the patchset: https://codereview.chromium.org/2283783002/#ps40001
(title: "Fix compile")

[commit-bot: I haz the power, 2016-09-07 05:49:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 07:59:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-07 08:00:45]

Description was changed from

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.
==========

to

==========
Move static methods out of CSSLengthInterpolationType to
LengthInterpolationFunctions

This change refactors the InterpolationValue logic out of
CSSLengthInterpolationType into its own file LengthInterpolationFunctions.
This allows other CSSInterpolationTypes to use Length functions without
importing unnecessary InterpolationType methods.

Committed: https://crrev.com/c31809296fc253caeb3a34dc3a05c0c96d3f9209
Cr-Commit-Position: refs/heads/master@{#416877}
==========

[commit-bot: I haz the power, 2016-09-07 08:00:46]

Patchset 3 (id:??) landed as
https://crrev.com/c31809296fc253caeb3a34dc3a05c0c96d3f9209
Cr-Commit-Position: refs/heads/master@{#416877}