Takeover MT initiated animations from the compositor

Takeover MT initiated animations from the compositor

This is a followup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

BUG=581875
Committed: https://crrev.com/d0eaee288867a6e00e4a9c3fa65d28e4de9e922a
Cr-Commit-Position: refs/heads/master@{#379868}

[ymalik, 2016-03-07 15:53:50]

Description was changed from

==========
[WIP] First attempt to cancel compositor running animation initiated by MT

BUG=
==========

to

==========
Takeover MT initiated animations from the compositor

This is a follup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.
==========

[ymalik, 2016-03-07 15:53:50]

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

[ymalik, 2016-03-07 15:54:49]

Description was changed from

==========
Takeover MT initiated animations from the compositor

This is a follup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.
==========

to

==========
Takeover MT initiated animations from the compositor

This is a follup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

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

[ajuma, 2016-03-07 16:42:11]

lgtm

Please update the description to say a bit more about why we'd want to takeover
animations from the compositor.

https://codereview.chromium.org/1770443002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/1770443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:108:
// on the main thread. This could happen if a main thread scrolling
'main thread scrolling reason'

[ymalik, 2016-03-07 17:56:13]

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

[ymalik, 2016-03-07 17:57:21]

+Jeremy for Source/core, Source/platform

[jbroman, 2016-03-07 18:12:35]

Description was changed from

==========
Takeover MT initiated animations from the compositor

This is a follup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

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

to

==========
Takeover MT initiated animations from the compositor

This is a followup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

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

[jbroman, 2016-03-07 18:38:41]

rs lgtm with nits

I'm relying on ajuma's review for the correctness of the scrolling logic.

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

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:218: if
(!m_scrollableArea->shouldScrollOnMainThread()) {
super-nit: this might be a little easier to read with early return:

if (m_scrollableArea->shouldScrollOnMainThread())
    return false;

OwnPtr<CompositorAnimation> animation = ...;
// ...
bool sentToCompositor = addAnimation(animation.release();
if (sentToCompositor) {
     // ...
}
return sentToCompositor;

It reduces the level of indentation, and I know immediately that
if the scrollable area is on the main thread that nothing happens,
without having to read past the if statement.

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:99: explicit
TestScrollAnimator(ScrollableArea* scrollableArea, WTF::TimeFunction
timingFunction = WTF::monotonicallyIncreasingTime)
This is never constructed with the default argument; why have a default?

Also, two-argument constructors need not be explicit.

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:119: void
abortAnimation() override {};
nit: superfluous semicolon

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:320:
EXPECT_CALL(*scrollableArea, setScrollOffset(_, _)).Times(3);
It's not entirely obvious to me why we should expect three setScrollOffsets and
four registerForAnimations. Is this a meaningful expectation, or does this just
happen to be the number of calls?

[ymalik, 2016-03-07 21:26:07]

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

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:218: if
(!m_scrollableArea->shouldScrollOnMainThread()) {
On 2016/03/07 18:38:40, jbroman wrote:
> super-nit: this might be a little easier to read with early return:
> 
> if (m_scrollableArea->shouldScrollOnMainThread())
>     return false;
> 
> OwnPtr<CompositorAnimation> animation = ...;
> // ...
> bool sentToCompositor = addAnimation(animation.release();
> if (sentToCompositor) {
>      // ...
> }
> return sentToCompositor;
> 
> It reduces the level of indentation, and I know immediately that
> if the scrollable area is on the main thread that nothing happens,
> without having to read past the if statement.

Agreed. Done!

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:99: explicit
TestScrollAnimator(ScrollableArea* scrollableArea, WTF::TimeFunction
timingFunction = WTF::monotonicallyIncreasingTime)
On 2016/03/07 18:38:41, jbroman wrote:
> This is never constructed with the default argument; why have a default?
> 
> Also, two-argument constructors need not be explicit.

Done.

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:320:
EXPECT_CALL(*scrollableArea, setScrollOffset(_, _)).Times(3);
On 2016/03/07 18:38:41, jbroman wrote:
> It's not entirely obvious to me why we should expect three setScrollOffsets
and
> four registerForAnimations. Is this a meaningful expectation, or does this
just
> happen to be the number of calls?

Actually setScrollOffset is called 2 times, and registerForAnimation is called 3
times (I missed the failure earlier as apparently the test still passes if the
EXPECT_CALL expectation is unsatisfied). 

The reason for the extra registerForAnimation is that when we are running an
animation on the compositor, we deregister for service because the animation is
serviced by the compositor. Thus, we need to registerForAnimation again if the
animation is cancelled or we need to takeover.

[jbroman, 2016-03-07 21:33:17]

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right):

https://codereview.chromium.org/1770443002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:320:
EXPECT_CALL(*scrollableArea, setScrollOffset(_, _)).Times(3);
On 2016/03/07 at 21:26:07, ymalik1 wrote:
> On 2016/03/07 18:38:41, jbroman wrote:
> > It's not entirely obvious to me why we should expect three setScrollOffsets
and
> > four registerForAnimations. Is this a meaningful expectation, or does this
just
> > happen to be the number of calls?
> 
> Actually setScrollOffset is called 2 times, and registerForAnimation is called
3 times (I missed the failure earlier as apparently the test still passes if the
EXPECT_CALL expectation is unsatisfied). 

Yeah, you'll have to make sure your mock object is collected (if, as here, it's
garbage-collected). I think tdresser ran into a similar issue recently.

> The reason for the extra registerForAnimation is that when we are running an
animation on the compositor, we deregister for service because the animation is
serviced by the compositor. Thus, we need to registerForAnimation again if the
animation is cancelled or we need to takeover.

A brief comment explaining where the magic numbers come from (unless it's
obvious to everyone working on ScrollAnimator) would be nice. :)

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

https://codereview.chromium.org/1770443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:236:
sentToCompositor = addAnimation(animation.release());
nit: move the variable declaration here (delete the declaration above, and
insert "bool" here)

[ymalik, 2016-03-08 15:27:25]

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

https://codereview.chromium.org/1770443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:236:
sentToCompositor = addAnimation(animation.release());
On 2016/03/07 21:33:17, jbroman wrote:
> nit: move the variable declaration here (delete the declaration above, and
> insert "bool" here)

Done.

[ymalik, 2016-03-08 15:27:31]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-03-08 15:27:32]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1770443002/#ps100001
(title: "nitsnits")

[commit-bot: I haz the power, 2016-03-08 15:27:46]

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

[commit-bot: I haz the power, 2016-03-08 15:39:00]

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

[commit-bot: I haz the power, 2016-03-08 15:39:01]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ymalik, 2016-03-08 17:48:05]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-03-08 17:48:32]

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

[commit-bot: I haz the power, 2016-03-08 18:25:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-08 18:27:44]

Description was changed from

==========
Takeover MT initiated animations from the compositor

This is a followup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

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

to

==========
Takeover MT initiated animations from the compositor

This is a followup CL to https://crrev.com/1734063003/. It finishes a
MT initiated animation running on the compositor, on the MT.

BUG=581875

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

[commit-bot: I haz the power, 2016-03-08 18:27:45]

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