Implement DeferredAnimatableValue

Implement DeferredAnimatableValue

This sentinal AnimatableValue marks points in a KeyframeVector that need
to be replaced with a snapshot of the element's current computed
CSSValue once such values have been applied during
applyMatchedProperties().

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

[alancutter (OOO until 2018), 2013-07-23 08:09:03]

On 2013/07/16 12:13:52, alancutter wrote:

ping

[pdr., 2013-07-23 22:45:08]

LGTM with a naming question and a code question.

Is the name DeferredSnapshot spec'ed? I would prefer something like
"PendingSnapshotValue", or others. Deferred is confusing to me to use as a
placeholder. As with all bikeshedding arguments, this could just be my personal
preference; please ask around to see if others understand this :)

https://codereview.chromium.org/19367002/diff/4001/Source/core/animation/Anim...
File Source/core/animation/AnimatableValue.cpp (right):

https://codereview.chromium.org/19367002/diff/4001/Source/core/animation/Anim...
Source/core/animation/AnimatableValue.cpp:60: static AnimatableDeferredSnapshot*
deferredSnapshotSentinelValue = AnimatableDeferredSnapshot::create().leakRef();
Won't this create a new deferredSnapshotSentinelValue each time? I would have
expected something like:
if (!deferredSnapshotSentinelValue)
    deferredSnapshotSentinelValue = ...
return deferredSnapshotSentinelValue;

Additionally, why is this here and not in AnimatableDeferredSnapshot?

[alancutter (OOO until 2018), 2013-07-24 00:57:19]

On 2013/07/23 22:45:08, pdr wrote:
> LGTM with a naming question and a code question.
> 

Thanks pdr!

> Is the name DeferredSnapshot spec'ed? I would prefer something like
> "PendingSnapshotValue", or others. Deferred is confusing to me to use as a
> placeholder. As with all bikeshedding arguments, this could just be my
personal
> preference; please ask around to see if others understand this :)
> 

Discussing...

>
https://codereview.chromium.org/19367002/diff/4001/Source/core/animation/Anim...
> File Source/core/animation/AnimatableValue.cpp (right):
> 
>
https://codereview.chromium.org/19367002/diff/4001/Source/core/animation/Anim...
> Source/core/animation/AnimatableValue.cpp:60: static
AnimatableDeferredSnapshot*
> deferredSnapshotSentinelValue =
AnimatableDeferredSnapshot::create().leakRef();
> Won't this create a new deferredSnapshotSentinelValue each time? I would have
> expected something like:
> if (!deferredSnapshotSentinelValue)
>     deferredSnapshotSentinelValue = ...
> return deferredSnapshotSentinelValue;

static local variables get this behaviour for free. :)

> 
> Additionally, why is this here and not in AnimatableDeferredSnapshot?

AnimatableDeferredSnapshot's create method returns a PassRefPtr (by convention),
uses of it that only operate on the raw pointer would have unnecessary ref
churn.
Giving AnimatableDeferredSnapshot a getSentinel() method is a valid alternative
but would be inconsistent with the implementation of the other sentinel,
AnimatableNeutral.

[pdr., 2013-07-24 05:02:48]

>
https://codereview.chromium.org/19367002/diff/4001/Source/core/animation/Anim...
> > Source/core/animation/AnimatableValue.cpp:60: static
> AnimatableDeferredSnapshot*
> > deferredSnapshotSentinelValue =
> AnimatableDeferredSnapshot::create().leakRef();
> > Won't this create a new deferredSnapshotSentinelValue each time? I would
have
> > expected something like:
> > if (!deferredSnapshotSentinelValue)
> >     deferredSnapshotSentinelValue = ...
> > return deferredSnapshotSentinelValue;
> 
> static local variables get this behaviour for free. :)

/me is embarrassed!
I've always used the DEFINE_STATIC_LOCAL macro and assumed it worked as I
described. Thanks for setting me straight.
I've filed crbug.com/263695 to remove the DEFINE_STATIC_LOCAL macro and prevent
others from learning the same bad habits.

[commit-bot: I haz the power, 2013-07-24 06:16:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/19367002/12001

[commit-bot: I haz the power, 2013-07-24 08:15:36]

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

[commit-bot: I haz the power, 2013-07-24 10:04:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/19367002/12001

[commit-bot: I haz the power, 2013-07-24 12:09:49]

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

[commit-bot: I haz the power, 2013-07-24 23:44:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/19367002/12001

[commit-bot: I haz the power, 2013-07-25 00:34:57]

Change committed as 154872

[Steve Block, 2013-08-05 08:14:56]

Is there a patch in progress to actually apply these deferred values?

Also a couple of naming nits for consistency.

https://codereview.chromium.org/19367002/diff/12001/Source/core/animation/Ani...
File Source/core/animation/AnimatableValue.cpp (right):

https://codereview.chromium.org/19367002/diff/12001/Source/core/animation/Ani...
Source/core/animation/AnimatableValue.cpp:58: const AnimatableValue*
AnimatableValue::deferredSnapshotValue()
deferredValue() to match DeferredAnimatableValue?

https://codereview.chromium.org/19367002/diff/12001/Source/core/animation/Def...
File Source/core/animation/DeferredAnimatableValue.h (right):

https://codereview.chromium.org/19367002/diff/12001/Source/core/animation/Def...
Source/core/animation/DeferredAnimatableValue.h:39: class
DeferredAnimatableValue : public AnimatableValue {
For the neutral value you use the name 'AnimatableNeutral'. It would be good to
use a consistent pattern (I think I prefer the pattern here, so
AnimatableNeutral -> NeutralAnimatableValue)

[alancutter (OOO until 2018), 2013-08-05 10:00:28]

On 2013/08/05 08:14:56, Steve Block wrote:
> Is there a patch in progress to actually apply these deferred values?

timloh is about to submit a patch to remove it actually. Turns out it's not
necessary anymore as we take the snapshot outside of the style resolving stage.