Web Animations: Add InterpolableValue hierarchy.

Web Animations: Add InterpolableValue hierarchy.

This patch introduces the InterpolableValue hierarchy, which stores
the interpolable portion of generic values.

This is part of an effort to refactor the animations pipeline in order
to make it generic on animation value type (see linked master bug).

This header is not yet included anywhere. It will be used in future
patches.

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

[dstockwell, 2014-02-25 02:14:51]

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
File Source/core/animation/InterpolableValue.h (right):

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
Use the new/shorter license header.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:63: virtual
PassOwnPtr<InterpolableValue> interpolate(InterpolableValue &other, double
percentage) OVERRIDE FINAL {
percentage -> progress

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:63: virtual
PassOwnPtr<InterpolableValue> interpolate(InterpolableValue &other, double
percentage) OVERRIDE FINAL {
these non-trivial functions should move to cpp

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:98: return (other.clone());
remove parens here

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:128: InterpolableList *otherList =
toInterpolableList(&other);
assert same size

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:146: void
set(PassOwnPtr<InterpolableValue> value, size_t position) {
m_values.get()[position] = value; }
ASSERT position < m_size? Or is the size mutable?

Seems odd that position is not the first argument.

[shans, 2014-02-25 03:26:42]

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
File Source/core/animation/InterpolableValue.h (right):

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
On 2014/02/25 02:14:51, dstockwell wrote:
> Use the new/shorter license header.

Done.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:63: virtual
PassOwnPtr<InterpolableValue> interpolate(InterpolableValue &other, double
percentage) OVERRIDE FINAL {
On 2014/02/25 02:14:51, dstockwell wrote:
> percentage -> progress

Done.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:63: virtual
PassOwnPtr<InterpolableValue> interpolate(InterpolableValue &other, double
percentage) OVERRIDE FINAL {
On 2014/02/25 02:14:51, dstockwell wrote:
> percentage -> progress

Done.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:98: return (other.clone());
On 2014/02/25 02:14:51, dstockwell wrote:
> remove parens here

Done.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:128: InterpolableList *otherList =
toInterpolableList(&other);
On 2014/02/25 02:14:51, dstockwell wrote:
> assert same size

Done.

https://codereview.chromium.org/177183007/diff/1/Source/core/animation/Interp...
Source/core/animation/InterpolableValue.h:146: void
set(PassOwnPtr<InterpolableValue> value, size_t position) {
m_values.get()[position] = value; }
On 2014/02/25 02:14:51, dstockwell wrote:
> ASSERT position < m_size? Or is the size mutable?
> 
> Seems odd that position is not the first argument.

Done.

[shans, 2014-02-25 21:35:47]

Now with unit tests. ptal.

[alancutter (OOO until 2018), 2014-02-26 00:42:29]

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
File Source/core/animation/InterpolableValue.h (right):

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValue.h:32: virtual
PassOwnPtr<InterpolableValue> interpolate(const InterpolableValue &other, double
progress) const OVERRIDE FINAL;
The suite of blend() functions have the "other" being the value to blend from.
The first parameter for interpolate() should be named "to" to avoid confusion.

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValue.h:86: ASSERT(position < m_size);
I'm not so sure unchecked array access in release is okay, especially if the
array size must be agreed upon across different code segments (builder and
applicator).
You might want to confirm with someone more senior in the project than I.
Example:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
File Source/core/animation/InterpolableValueTest.cpp (right):

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValueTest.cpp:62: EXPECT_FLOAT_EQ(104.35,
toInterpolableNumber(outList->get(2))->value());
Should these list tests be testing more than one interpolation fraction value?

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValueTest.cpp:85:
EXPECT_TRUE(toInterpolableBool(outList->get(2))->value());
Overloading [] might make this cleaner here and in future uses of
InterpolableLists, thoughts?

[shans, 2014-02-26 21:24:20]

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
File Source/core/animation/InterpolableValue.h (right):

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValue.h:32: virtual
PassOwnPtr<InterpolableValue> interpolate(const InterpolableValue &other, double
progress) const OVERRIDE FINAL;
On 2014/02/26 00:42:29, alancutter wrote:
> The suite of blend() functions have the "other" being the value to blend from.
> The first parameter for interpolate() should be named "to" to avoid confusion.

Done.

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValue.h:86: ASSERT(position < m_size);
On 2014/02/26 00:42:29, alancutter wrote:
> I'm not so sure unchecked array access in release is okay, especially if the
> array size must be agreed upon across different code segments (builder and
> applicator).
> You might want to confirm with someone more senior in the project than I.
> Example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

InterpolableValues should only be constructed in a pairwise fashion, and are
intended to be solely used by the Interpolation class (which will be provided in
a follow-up patch). The 'shape' (i.e. type and list length) of matching
InterpolableValue trees should be checked when an Interpolation object is
constructed, rather than here.

To reflect this I've marked the interpolate function as private and friended the
unit testing class for now. Once Interpolation is available, then
InterpolableValue will friend that instead.

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
File Source/core/animation/InterpolableValueTest.cpp (right):

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValueTest.cpp:62: EXPECT_FLOAT_EQ(104.35,
toInterpolableNumber(outList->get(2))->value());
On 2014/02/26 00:42:29, alancutter wrote:
> Should these list tests be testing more than one interpolation fraction value?

As discussed, this is sufficient.

https://codereview.chromium.org/177183007/diff/40001/Source/core/animation/In...
Source/core/animation/InterpolableValueTest.cpp:85:
EXPECT_TRUE(toInterpolableBool(outList->get(2))->value());
On 2014/02/26 00:42:29, alancutter wrote:
> Overloading [] might make this cleaner here and in future uses of
> InterpolableLists, thoughts?

I don't think it is worth it, as InterpolableValues will only be used from
Interpolation.

[alancutter (OOO until 2018), 2014-03-03 00:02:01]

lgtm

[dstockwell, 2014-03-03 00:11:40]

lgtm

[commit-bot: I haz the power, 2014-03-03 00:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/177183007/60001

[commit-bot: I haz the power, 2014-03-03 01:31:24]

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

[commit-bot: I haz the power, 2014-03-03 01:31:24]

Retried try job too often on win_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[shans, 2014-03-03 02:38:00]

The CQ bit was checked by shans@chromium.org

[commit-bot: I haz the power, 2014-03-03 02:38:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shans@chromium.org/177183007/80001

[commit-bot: I haz the power, 2014-03-03 04:56:32]

Change committed as 168262