Share animations from compositor to main thread.

Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjSthizmz7g4BnOnBA/edit?usp=sharing

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6581bb3e791018e48d078a910ddfdce19a1c10bb
Cr-Commit-Position: refs/heads/master@{#378411}

[ymalik, 2016-02-26 00:23:16]

Description was changed from

==========
Share animations from compositor to main thread.

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
==========

to

==========
Share animations from compositor to main thread.

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[skobes, 2016-02-26 02:04:10]

FYI, I patched this in but it's giving me some compiler errors...

FAILED: ninja -t msvc -e environment.x86 -- C:\goma\goma-win64/gomacc
"C:\chrome\depot_tools\win_toolchain\vs_files\b349b3cc596d5f7e13d649532ddd7e8db39db0cb\VC\bin\amd64_x86\cl.exe"
/nologo /showIncludes /FC @obj\cc\input\cc.scroll_state.obj.rsp /c
..\..\cc\input\scroll_state.cc /Foobj\cc\input\cc.scroll_state.obj
/Fdobj\cc\cc.cc.pdb
c:\chrome\depot_tools\win_toolchain\vs_files\b349b3cc596d5f7e13d649532ddd7e8db39db0cb\vc\include\memory(1193):
error C2027: use of undefined type 'cc::AnimationCurve'
c:\chrome\src\cc\animation\animation.h(16): note: see declaration of
'cc::AnimationCurve'
c:\chrome\depot_tools\win_toolchain\vs_files\b349b3cc596d5f7e13d649532ddd7e8db39db0cb\vc\include\memory(1192):
note: while compiling class template member function 'void
std::default_delete<T>::operator ()(_Ty *) throw() const'
        with
        [
            T=cc::AnimationCurve,
            _Ty=cc::AnimationCurve
        ]
c:\chrome\src\base\memory\scoped_ptr.h(186): note: see reference to function
template instantiation 'void std::default_delete<T>::operator ()(_Ty *) throw()
const' being compiled
        with
        [
            T=cc::AnimationCurve,
            _Ty=cc::AnimationCurve
        ]
c:\chrome\src\base\memory\scoped_ptr.h(217): note: see reference to class
template instantiation 'std::default_delete<T>' being compiled
        with
        [
            T=cc::AnimationCurve
        ]
c:\chrome\src\base\memory\scoped_ptr.h(223): note: see reference to class
template instantiation
'base::internal::scoped_ptr_impl<cc::AnimationCurve,std::default_delete<T>>::Data'
being compiled
        with
        [
            T=cc::AnimationCurve
        ]
c:\chrome\src\base\memory\scoped_ptr.h(419): note: see reference to class
template instantiation
'base::internal::scoped_ptr_impl<cc::AnimationCurve,std::default_delete<T>>'
being compiled
        with
        [
            T=cc::AnimationCurve
        ]
c:\chrome\src\cc\animation\animation.h(174): note: see reference to class
template instantiation 'scoped_ptr<cc::AnimationCurve,std::default_delete<T>>'
being compiled
        with
        [
            T=cc::AnimationCurve
        ]

[ymalik, 2016-02-26 18:42:14]

Description was changed from

==========
Share animations from compositor to main thread.

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller
looks for impl-only animations in this state an passes a TAKEOVER animation
event to
the main thread with the animation_curve. The main thread animation then
completes
the animation.

Notes:
This is only for impl only animations, i.e, the jidder bug won't be fixed for an
animation
running on cc that was initiated by the main thread (keyboard scroll). Future CL
will add
the take over logic for MT-initiated animations.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-26 18:44:32]

Description was changed from

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller
looks for impl-only animations in this state an passes a TAKEOVER animation
event to
the main thread with the animation_curve. The main thread animation then
completes
the animation.

Notes:
This is only for impl only animations, i.e, the jidder bug won't be fixed for an
animation
running on cc that was initiated by the main thread (keyboard scroll). Future CL
will add
the take over logic for MT-initiated animations.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-26 19:03:27]

Description was changed from

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
-This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-26 19:03:53]

Description was changed from

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
-This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ymalik, 2016-02-26 19:05:24]

ymalik@chromium.org changed reviewers:
+ ajuma@chromium.org

[ymalik, 2016-02-26 19:05:25]

Hey Ali, sorry for the large CL.

I think the crux here is in
LayerAnimationController/AnimationEvent/ScrollAnimator/LayerImpl, the rest is
just plumbing :)

Thanks!

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
I tried to make this pure virtual, but had the following problem:
Source/core/animation/Animation.h inherits this class. When I override the
function there, I get an error when I try to upload that we shouldn't reference
cc in Source/core. Thats strange because WebCompositorAnimationDelegate is in
public/platform and can reference cc, and Animation includes
WebCompositorAnimationDelegate, so it should also be allowed to reference cc?

[ajuma, 2016-02-26 23:56:55]

Nice work, this mostly looks great.

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/animation_...
File cc/animation/animation_delegate.h (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/animation_...
cc/animation/animation_delegate.h:28: double animation_start_time,
This should include a target property (even if we happen to only be using it for
scroll animations right now).

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:937:
events->events_.push_back(std::move(aborted_event));
AnimationEvent doesn't have a move constructor, so won't this just use the copy
constructor (meaning the call to std::move has no effect)?

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:1028:
start_time_(base::TimeTicks()) {}
Initialize takeover_ here.

https://codereview.chromium.org/1734063003/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:515: layer_animation_controller()->AbortAnimations(
Shouldn't this be only if the animations are impl-only? That said, the old
animation code path is close to being deleted (see crbug.com/575041), so this
might not end up mattering by the time this lands.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:143: return true;
Should this return false? (We're already at the target.)

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:344: // over an
animation, do nothing to avoid jidder.
"judder"? or maybe "jitter"?

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:90: // Returns true
if will animate to the given target position.
Please expand this to include the sorts of things that would cause us to animate
or not animate.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
On 2016/02/26 19:05:25, ymalik1 wrote:
> I tried to make this pure virtual, but had the following problem:
> Source/core/animation/Animation.h inherits this class. When I override the
> function there, I get an error when I try to upload that we shouldn't
reference
> cc in Source/core. Thats strange because WebCompositorAnimationDelegate is in
> public/platform and can reference cc, and Animation includes
> WebCompositorAnimationDelegate, so it should also be allowed to reference cc?

No, core isn't allowed to refer to cc types. The options seem to be to leave
this as non-pure virtual or to define a subclass (say in platform/graphics) that
provides an override and then subclass *that* in core instead of subclassing
WebCompositorAnimationDelegate. I'll leave this up to whoever does the public/
OWNERS review for this CL.

[ymalik, 2016-02-29 16:31:09]

Hey Ali, PTAL :)

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/animation_...
File cc/animation/animation_delegate.h (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/animation_...
cc/animation/animation_delegate.h:28: double animation_start_time,
On 2016/02/26 23:56:54, ajuma wrote:
> This should include a target property (even if we happen to only be using it
for
> scroll animations right now).

Totally. Done thanks!

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
cc/animation/layer_animation_controller.cc:937:
events->events_.push_back(std::move(aborted_event));
On 2016/02/26 23:56:54, ajuma wrote:
> AnimationEvent doesn't have a move constructor, so won't this just use the
copy
> constructor (meaning the call to std::move has no effect)?

Yes, that's correct. Got rid of the std::move call.

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
File cc/animation/layer_animation_controller_unittest.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/animation/layer_anim...
cc/animation/layer_animation_controller_unittest.cc:1028:
start_time_(base::TimeTicks()) {}
On 2016/02/26 23:56:54, ajuma wrote:
> Initialize takeover_ here.

Done.

https://codereview.chromium.org/1734063003/diff/80001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1734063003/diff/80001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:515: layer_animation_controller()->AbortAnimations(
On 2016/02/26 23:56:54, ajuma wrote:
> Shouldn't this be only if the animations are impl-only? That said, the old
> animation code path is close to being deleted (see crbug.com/575041), so this
> might not end up mattering by the time this lands.

Yes, you're right. I am removing it though, since I don't have the takeover
pluming for the old path anyway.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:143: return true;
On 2016/02/26 23:56:54, ajuma wrote:
> Should this return false? (We're already at the target.)

Ideally yes, but this was originally there for scroll latching. See the comment
L130-132.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:344: // over an
animation, do nothing to avoid jidder.
On 2016/02/26 23:56:54, ajuma wrote:
> "judder"? or maybe "jitter"?

Done.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.h:90: // Returns true
if will animate to the given target position.
On 2016/02/26 23:56:54, ajuma wrote:
> Please expand this to include the sorts of things that would cause us to
animate
> or not animate.

Done.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
On 2016/02/26 23:56:54, ajuma wrote:
> On 2016/02/26 19:05:25, ymalik1 wrote:
> > I tried to make this pure virtual, but had the following problem:
> > Source/core/animation/Animation.h inherits this class. When I override the
> > function there, I get an error when I try to upload that we shouldn't
> reference
> > cc in Source/core. Thats strange because WebCompositorAnimationDelegate is
in
> > public/platform and can reference cc, and Animation includes
> > WebCompositorAnimationDelegate, so it should also be allowed to reference
cc?
> 
> No, core isn't allowed to refer to cc types. The options seem to be to leave
> this as non-pure virtual or to define a subclass (say in platform/graphics)
that
> provides an override and then subclass *that* in core instead of subclassing
> WebCompositorAnimationDelegate. I'll leave this up to whoever does the public/
> OWNERS review for this CL.

Acknowledged.

[ajuma, 2016-02-29 17:57:36]

Thanks, lgtm.

[ymalik, 2016-02-29 18:02:54]

ymalik@chromium.org changed reviewers:
+ jbroman@chromium.org

[ymalik, 2016-02-29 18:04:48]

ymalik@chromium.org changed reviewers:
+ rbyers@chromium.org
- jbroman@chromium.org

[ymalik, 2016-02-29 18:04:49]

+Rick for Source/platform, public/platform

[Rick Byers, 2016-02-29 18:35:33]

public/platform LGTM (with nit)
Source/platform rubber-stamp LGTM (ajuma is an expert in this space)

https://codereview.chromium.org/1734063003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
Nit: reference the bug that tracks this.
Looks like there's a few subclasses, so I assume you'd rather do this in a
separate CL to keep this one from getting even bigger, right?

[ymalik, 2016-02-29 18:40:05]

@Rick, PTAL :)

https://codereview.chromium.org/1734063003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
On 2016/02/29 18:35:33, Rick Byers wrote:
> Nit: reference the bug that tracks this.
> Looks like there's a few subclasses, so I assume you'd rather do this in a
> separate CL to keep this one from getting even bigger, right?

Oops, please see comments from previous patchset:
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...

[Rick Byers, 2016-02-29 20:21:35]

>> Looks like there's a few subclasses, so I assume you'd rather do this in a
>> separate CL to keep this one from getting even bigger, right?
> Oops, please see comments from previous patchset:
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...

Ah, sorry I missed that.  I'll reply there.

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
(right):

https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
TODO(ymalik): Make pure virtual.
On 2016/02/29 16:31:09, ymalik1 wrote:
> On 2016/02/26 23:56:54, ajuma wrote:
> > On 2016/02/26 19:05:25, ymalik1 wrote:
> > > I tried to make this pure virtual, but had the following problem:
> > > Source/core/animation/Animation.h inherits this class. When I override the
> > > function there, I get an error when I try to upload that we shouldn't
> > reference
> > > cc in Source/core. Thats strange because WebCompositorAnimationDelegate is
> in
> > > public/platform and can reference cc, and Animation includes
> > > WebCompositorAnimationDelegate, so it should also be allowed to reference
> cc?
> > 
> > No, core isn't allowed to refer to cc types. The options seem to be to leave
> > this as non-pure virtual or to define a subclass (say in platform/graphics)
> that
> > provides an override and then subclass *that* in core instead of subclassing
> > WebCompositorAnimationDelegate. I'll leave this up to whoever does the
public/
> > OWNERS review for this CL.
> 
> Acknowledged.

I think a regular virtual is probably OK if you can describe (in a comment here)
why it's OK for only the classes with access to 'cc' to be able to implement
this method.  In particular, since blink's Animation class is handling
notifyAnimationStarted, won't it also really want to know about
animationTakeover cases?

[ymalik, 2016-02-29 21:48:28]

On 2016/02/29 20:21:35, Rick Byers wrote:
> >> Looks like there's a few subclasses, so I assume you'd rather do this in a
> >> separate CL to keep this one from getting even bigger, right?
> > Oops, please see comments from previous patchset:
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> 
> Ah, sorry I missed that.  I'll reply there.
> 
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
> (right):
> 
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
> TODO(ymalik): Make pure virtual.
> On 2016/02/29 16:31:09, ymalik1 wrote:
> > On 2016/02/26 23:56:54, ajuma wrote:
> > > On 2016/02/26 19:05:25, ymalik1 wrote:
> > > > I tried to make this pure virtual, but had the following problem:
> > > > Source/core/animation/Animation.h inherits this class. When I override
the
> > > > function there, I get an error when I try to upload that we shouldn't
> > > reference
> > > > cc in Source/core. Thats strange because WebCompositorAnimationDelegate
is
> > in
> > > > public/platform and can reference cc, and Animation includes
> > > > WebCompositorAnimationDelegate, so it should also be allowed to
reference
> > cc?
> > > 
> > > No, core isn't allowed to refer to cc types. The options seem to be to
leave
> > > this as non-pure virtual or to define a subclass (say in
platform/graphics)
> > that
> > > provides an override and then subclass *that* in core instead of
subclassing
> > > WebCompositorAnimationDelegate. I'll leave this up to whoever does the
> public/
> > > OWNERS review for this CL.
> > 
> > Acknowledged.
> 
> I think a regular virtual is probably OK if you can describe (in a comment
here)
> why it's OK for only the classes with access to 'cc' to be able to implement
> this method.  In particular, since blink's Animation class is handling
> notifyAnimationStarted, won't it also really want to know about
> animationTakeover cases?

Done.

[Rick Byers, 2016-02-29 22:30:00]

On 2016/02/29 21:48:28, ymalik1 wrote:
> On 2016/02/29 20:21:35, Rick Byers wrote:
> > >> Looks like there's a few subclasses, so I assume you'd rather do this in
a
> > >> separate CL to keep this one from getting even bigger, right?
> > > Oops, please see comments from previous patchset:
> > >
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > 
> > Ah, sorry I missed that.  I'll reply there.
> > 
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
> > TODO(ymalik): Make pure virtual.
> > On 2016/02/29 16:31:09, ymalik1 wrote:
> > > On 2016/02/26 23:56:54, ajuma wrote:
> > > > On 2016/02/26 19:05:25, ymalik1 wrote:
> > > > > I tried to make this pure virtual, but had the following problem:
> > > > > Source/core/animation/Animation.h inherits this class. When I override
> the
> > > > > function there, I get an error when I try to upload that we shouldn't
> > > > reference
> > > > > cc in Source/core. Thats strange because
WebCompositorAnimationDelegate
> is
> > > in
> > > > > public/platform and can reference cc, and Animation includes
> > > > > WebCompositorAnimationDelegate, so it should also be allowed to
> reference
> > > cc?
> > > > 
> > > > No, core isn't allowed to refer to cc types. The options seem to be to
> leave
> > > > this as non-pure virtual or to define a subclass (say in
> platform/graphics)
> > > that
> > > > provides an override and then subclass *that* in core instead of
> subclassing
> > > > WebCompositorAnimationDelegate. I'll leave this up to whoever does the
> > public/
> > > > OWNERS review for this CL.
> > > 
> > > Acknowledged.
> > 
> > I think a regular virtual is probably OK if you can describe (in a comment
> here)
> > why it's OK for only the classes with access to 'cc' to be able to implement
> > this method.  In particular, since blink's Animation class is handling
> > notifyAnimationStarted, won't it also really want to know about
> > animationTakeover cases?
> 
> Done.

Ok, public LGTM (assuming Ali is still happy with the overall API design here -
seems a little unfortunate to have to special-case scrolling animations like
this, but Ali is the expert in the animations code so I defer to him).

[ajuma, 2016-02-29 22:37:57]

On 2016/02/29 22:30:00, Rick Byers wrote:
> On 2016/02/29 21:48:28, ymalik1 wrote:
> > On 2016/02/29 20:21:35, Rick Byers wrote:
> > > >> Looks like there's a few subclasses, so I assume you'd rather do this
in
> a
> > > >> separate CL to keep this one from getting even bigger, right?
> > > > Oops, please see comments from previous patchset:
> > > >
> > >
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > > 
> > > Ah, sorry I missed that.  I'll reply there.
> > > 
> > >
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > > File third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1734063003/diff/80001/third_party/WebKit/publ...
> > > third_party/WebKit/public/platform/WebCompositorAnimationDelegate.h:20: //
> > > TODO(ymalik): Make pure virtual.
> > > On 2016/02/29 16:31:09, ymalik1 wrote:
> > > > On 2016/02/26 23:56:54, ajuma wrote:
> > > > > On 2016/02/26 19:05:25, ymalik1 wrote:
> > > > > > I tried to make this pure virtual, but had the following problem:
> > > > > > Source/core/animation/Animation.h inherits this class. When I
override
> > the
> > > > > > function there, I get an error when I try to upload that we
shouldn't
> > > > > reference
> > > > > > cc in Source/core. Thats strange because
> WebCompositorAnimationDelegate
> > is
> > > > in
> > > > > > public/platform and can reference cc, and Animation includes
> > > > > > WebCompositorAnimationDelegate, so it should also be allowed to
> > reference
> > > > cc?
> > > > > 
> > > > > No, core isn't allowed to refer to cc types. The options seem to be to
> > leave
> > > > > this as non-pure virtual or to define a subclass (say in
> > platform/graphics)
> > > > that
> > > > > provides an override and then subclass *that* in core instead of
> > subclassing
> > > > > WebCompositorAnimationDelegate. I'll leave this up to whoever does the
> > > public/
> > > > > OWNERS review for this CL.
> > > > 
> > > > Acknowledged.
> > > 
> > > I think a regular virtual is probably OK if you can describe (in a comment
> > here)
> > > why it's OK for only the classes with access to 'cc' to be able to
implement
> > > this method.  In particular, since blink's Animation class is handling
> > > notifyAnimationStarted, won't it also really want to know about
> > > animationTakeover cases?
> > 
> > Done.
> 
> Ok, public LGTM (assuming Ali is still happy with the overall API design here
-
> seems a little unfortunate to have to special-case scrolling animations like
> this, but Ali is the expert in the animations code so I defer to him).

Still lgtm. Agreed that having to special case scroll animations is a bit
unfortunate, but seems like a reasonable way to deal with the restriction on
using cc in core.

[ymalik, 2016-03-01 05:20:51]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-03-01 05:20:52]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1734063003/#ps140001
(title: "fix win trybot failure")

[commit-bot: I haz the power, 2016-03-01 05:21:08]

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

[ymalik, 2016-03-01 05:21:18]

The CQ bit was unchecked by ymalik@chromium.org

[ymalik, 2016-03-01 05:21:28]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-03-01 05:21:51]

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

[commit-bot: I haz the power, 2016-03-01 07:32:18]

Description was changed from

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-01 07:32:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-01 07:33:43]

Description was changed from

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread 
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Share animations from compositor to main thread.

This CL introduces a new state that a compositor animation can be in, namely,
ABORT_BUT_NEEDS_COMPLETION. When a main thread scrolling reason is added,
the current scroll offset animation is put into this state. The layer animation
controller looks for impl-only animations in this state and passes a TAKEOVER
animation event to the main thread with the animation_curve. The main thread
then completes the animation.

Notes:
- This CL is for impl only animations, i.e, the jidder bug won't be fixed for an
animation running on cc that was initiated by the main thread (keyboard scroll).
 Future CL will add the takeover logic for MT-initiated animations.
- This CL doesn't support the takeover notification from the old animation path.
The old path is where we don't use CompositorAnimationPlayer and its better
to not add the plumbing for something that will be removed soon anyway.

Design-doc
https://docs.google.com/a/google.com/document/d/1JUYvp5gilux7bca8vfmE4Rr5BUjS...

Test page:
http://philipwalton.github.io/polyfill/demos/position-sticky/

BUG=581875
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6581bb3e791018e48d078a910ddfdce19a1c10bb
Cr-Commit-Position: refs/heads/master@{#378411}
==========

[commit-bot: I haz the power, 2016-03-01 07:33:45]

Patchset 8 (id:??) landed as
https://crrev.com/6581bb3e791018e48d078a910ddfdce19a1c10bb
Cr-Commit-Position: refs/heads/master@{#378411}