Move interpolated Length unit types out of InterpolableValue into NonInterpolableValue

Move interpolated Length unit types out of InterpolableValue into NonInterpolableValue

We currently store a list of 0s or 1s for each Length unit type in a Length
InterpolableValue. This change moves this data into the NonInterpolableValue
component of a Length interpolation. It also simplifies it down to only
whether a percentage is present or not since percentage is the only unit
type that doesn't get resolved to pixels in computed values.

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

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

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

[alancutter (OOO until 2018), 2015-07-27 01:24:02]

shans dstockwell: Poke.

[dstockwell, 2015-08-31 06:44:26]

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
File Source/core/animation/InterpolationType.h (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
Source/core/animation/InterpolationType.h:63: virtual
PassRefPtrWillBeRawPtr<NonInterpolableValue> mergeNonInterpolableValues(const
NonInterpolableValue* a, const NonInterpolableValue* b) const
comment, what does it mean to merge these?

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
Source/core/animation/InvalidatableStyleInterpolation.cpp:134:
underlyingValue->setNonInterpolableValue(m_cachedValue->type().mergeNonInterpolableValues(underlyingValue->nonInterpolableValue(),
m_cachedValue->nonInterpolableValue()));
It seems odd to modify the underlying value. Should these use appliedValue? I
realize they are the same instance.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
File Source/core/animation/LengthInterpolationType.cpp (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:16: class HasPercentage :
public NonInterpolableValue {
I was going to suggest LengthHasPercentage but I guess since it's restricted to
this unit the name is OK. But if we're going to have a general naming scheme for
NIV (eg. footypeNIV) we should follow that here too.

Since the design is slightly unusual (the NIV doesn't contain any data - but is
the data) I think it would be valuable to explain that here in a comment.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:180: static double
clampToRange(double x, ValueRange range)
Move this alongside ValueRange.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:245: bool hasPercentage =
toHasPercentage(nonInterpolableValue);
On further thoughts, lets rename HasPercentage to LengthNonInterpolableValue (or
whatever) and add static functions to contain this logic, so this becomes bool
hasPercentage = LengthNonInterpolableValue::hasPercentage(nonInterpolableValue);

[alancutter (OOO until 2018), 2015-09-01 00:26:00]

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
File Source/core/animation/InterpolationType.h (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
Source/core/animation/InterpolationType.h:63: virtual
PassRefPtrWillBeRawPtr<NonInterpolableValue> mergeNonInterpolableValues(const
NonInterpolableValue* a, const NonInterpolableValue* b) const
On 2015/08/31 at 06:44:26, dstockwell wrote:
> comment, what does it mean to merge these?

Added comment. Changed default implementation to assert and return nullptr as I
can't actually think of a situation where we'd simply take the RHS.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
File Source/core/animation/InvalidatableStyleInterpolation.cpp (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/I...
Source/core/animation/InvalidatableStyleInterpolation.cpp:134:
underlyingValue->setNonInterpolableValue(m_cachedValue->type().mergeNonInterpolableValues(underlyingValue->nonInterpolableValue(),
m_cachedValue->nonInterpolableValue()));
On 2015/08/31 at 06:44:26, dstockwell wrote:
> It seems odd to modify the underlying value. Should these use appliedValue? I
realize they are the same instance.

Done.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
File Source/core/animation/LengthInterpolationType.cpp (right):

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:16: class HasPercentage :
public NonInterpolableValue {
On 2015/08/31 at 06:44:26, dstockwell wrote:
> I was going to suggest LengthHasPercentage but I guess since it's restricted
to this unit the name is OK. But if we're going to have a general naming scheme
for NIV (eg. footypeNIV) we should follow that here too.
> 
> Since the design is slightly unusual (the NIV doesn't contain any data - but
is the data) I think it would be valuable to explain that here in a comment.

Added comment.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:180: static double
clampToRange(double x, ValueRange range)
On 2015/08/31 at 06:44:26, dstockwell wrote:
> Move this alongside ValueRange.

It's probably better if this is done in as separate patch and have all other
uses of ValueRange updated to use the same clamping function. Added TODO.

https://codereview.chromium.org/1248093002/diff/20001/Source/core/animation/L...
Source/core/animation/LengthInterpolationType.cpp:245: bool hasPercentage =
toHasPercentage(nonInterpolableValue);
On 2015/08/31 at 06:44:26, dstockwell wrote:
> On further thoughts, lets rename HasPercentage to LengthNonInterpolableValue
(or whatever) and add static functions to contain this logic, so this becomes
bool hasPercentage =
LengthNonInterpolableValue::hasPercentage(nonInterpolableValue);

Done, changed get() to create(bool hasPercentage) to push all the singleton
boolean logic inside LengthNonInterpolableValue.

[dstockwell, 2015-09-09 10:07:16]

lgtm

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

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:23]

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

[commit-bot: I haz the power, 2015-09-14 06:21:27]

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

[commit-bot: I haz the power, 2015-09-14 06:21:28]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[alancutter (OOO until 2018), 2015-09-17 03:00:36]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2015-09-17 03:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1248093002/#ps140001
(title: "Initialise")

[commit-bot: I haz the power, 2015-09-17 03:01:22]

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

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

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

[commit-bot: I haz the power, 2015-09-17 03:03:56]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[alancutter (OOO until 2018), 2015-09-17 07:07:51]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2015-09-17 07:07:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1248093002/#ps160001
(title: "Rebase again")

[commit-bot: I haz the power, 2015-09-17 07:08:01]

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

[commit-bot: I haz the power, 2015-09-17 10:07:50]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202439