Send takeover msg from MT to CC using the animation path.

Send takeover msg from MT to CC using the animation path.

This is a follow-up CL to https://crrev.com/1950243005/

When a main thread scrolling reason gets added while there is a scroll offset
animation running on CC, the animation is aborted on CC and finished on the MT.

Before this CL, the plumbing for aborting the animation went through
LayerImpl::set_main_thread_scrolling_reasons, which is reworked with property
trees. This CL uses the communication scheme described in http://bit.ly/1Y6RA6y
to send a "takeover" update to CC.

BUG=607047
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c
Cr-Commit-Position: refs/heads/master@{#396061}

[ymalik, 2016-05-24 22:20:16]

Description was changed from

==========
Send takeover msg from MT to CC using the animation path.

This is a follow-up CL to https://crrev.com/1950243005/

When a main thread scrolling reason gets added while there is a scroll offset
animation running on CC, the animation is aborted on CC and finished on the MT.

Before this CL, the plumbing for aborting the animation went through
LayerImpl::set_main_thread_scrolling_reasons, which is reworked with property
trees. This CL uses the communication scheme described in http://bit.ly/1Y6RA6y
to send a "takeover" update to CC.

BUG=607047
==========

to

==========
Send takeover msg from MT to CC using the animation path.

This is a follow-up CL to https://crrev.com/1950243005/

When a main thread scrolling reason gets added while there is a scroll offset
animation running on CC, the animation is aborted on CC and finished on the MT.

Before this CL, the plumbing for aborting the animation went through
LayerImpl::set_main_thread_scrolling_reasons, which is reworked with property
trees. This CL uses the communication scheme described in http://bit.ly/1Y6RA6y
to send a "takeover" update to CC.

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

[ymalik, 2016-05-24 22:21:54]

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

[ajuma, 2016-05-24 23:25:09]

lgtm, the new approach seems a lot cleaner than the old one!

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.cc:12: : element_id_(element_id) {}
takeover_ needs to be initialized here

[skobes, 2016-05-24 23:50:50]

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.cc:33:
element_to_update_map_.insert(std::make_pair(element_id, update));
Hmm, I think unordered_map::insert will not replace an existing value.  Maybe
you want [] instead?

(same in AddTakeoverUpdate)

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.h:52: using ElementToUpdate =
I'd call this UpdateMap or maybe ElementToUpdateMap.

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:669:
scrollAnimator->takeoverCompositorAnimation();
Perhaps takeoverImplOnlyScrollOffsetAnimation can be combined with
takeoverCompositorAnimation?

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:147:
// abort the impl-only scroll offset animation and continue it on mt.
spell out "main thread" for clarity

[ymalik, 2016-05-25 16:09:05]

@skobes, ptal :)

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.cc (right):

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.cc:12: : element_id_(element_id) {}
On 2016/05/24 23:25:09, ajuma wrote:
> takeover_ needs to be initialized here

Thanks! Done.

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.cc:33:
element_to_update_map_.insert(std::make_pair(element_id, update));
On 2016/05/24 23:50:50, skobes wrote:
> Hmm, I think unordered_map::insert will not replace an existing value.  Maybe
> you want [] instead?
> 
> (same in AddTakeoverUpdate)

Good catch. Done!

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
File cc/animation/scroll_offset_animations.h (right):

https://codereview.chromium.org/2006103004/diff/1/cc/animation/scroll_offset_...
cc/animation/scroll_offset_animations.h:52: using ElementToUpdate =
On 2016/05/24 23:50:50, skobes wrote:
> I'd call this UpdateMap or maybe ElementToUpdateMap.

Done.

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:669:
scrollAnimator->takeoverCompositorAnimation();
On 2016/05/24 23:50:50, skobes wrote:
> Perhaps takeoverImplOnlyScrollOffsetAnimation can be combined with
> takeoverCompositorAnimation?

Yeah! That's actually more correct because we either have an animation running
on cc or on mt, so shouldn't be doing both.

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
(right):

https://codereview.chromium.org/2006103004/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:147:
// abort the impl-only scroll offset animation and continue it on mt.
On 2016/05/24 23:50:50, skobes wrote:
> spell out "main thread" for clarity

Done.

[skobes, 2016-05-25 16:48:53]

lgtm

[ymalik, 2016-05-25 16:58:28]

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

[ymalik, 2016-05-25 16:58:28]

+jbroman for Source/platform

[ymalik, 2016-05-25 18:03:50]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 18:07:06]

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

[commit-bot: I haz the power, 2016-05-25 18:25:37]

Dry run: None

[jbroman, 2016-05-25 19:00:38]

Assuming the scrolling logic was already reviewed by ajuma and skobes, platform
lgtm with a few comments.

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:36: void
takeoverImplOnlyScrollOffsetAnimation(cc::ElementId);
nit: "takeOver", since as a verb "take over" is two words

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:36: void
takeoverImplOnlyScrollOffsetAnimation(cc::ElementId);
nit: "takeOver", since as a verb "take over" is two words ("takeover" is a noun)

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

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:127:
takeoverImplOnlyScrollOffsetAnimation();
Please add "break;" here. The fallthrough is confusing.

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

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:147:
private:
nit: blank line before "private:"

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:149:
void takeoverImplOnlyScrollOffsetAnimation();
ditto

[ymalik, 2016-05-25 19:51:10]

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
(right):

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h:36: void
takeoverImplOnlyScrollOffsetAnimation(cc::ElementId);
On 2016/05/25 19:00:37, jbroman wrote:
> nit: "takeOver", since as a verb "take over" is two words ("takeover" is a
noun)

Done.

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

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp:127:
takeoverImplOnlyScrollOffsetAnimation();
On 2016/05/25 19:00:37, jbroman wrote:
> Please add "break;" here. The fallthrough is confusing.

Done.

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

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:147:
private:
On 2016/05/25 19:00:37, jbroman wrote:
> nit: blank line before "private:"

Done.

https://codereview.chromium.org/2006103004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h:149:
void takeoverImplOnlyScrollOffsetAnimation();
On 2016/05/25 19:00:37, jbroman wrote:
> ditto

Done.

[ymalik, 2016-05-25 19:51:20]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-05-25 19:51:20]

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

[commit-bot: I haz the power, 2016-05-25 19:51:48]

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

[commit-bot: I haz the power, 2016-05-25 20:31:02]

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

[commit-bot: I haz the power, 2016-05-25 20:31:03]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[ymalik, 2016-05-25 22:39:37]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-05-25 22:40:16]

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

[commit-bot: I haz the power, 2016-05-26 00:58:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-26 00:59:48]

Description was changed from

==========
Send takeover msg from MT to CC using the animation path.

This is a follow-up CL to https://crrev.com/1950243005/

When a main thread scrolling reason gets added while there is a scroll offset
animation running on CC, the animation is aborted on CC and finished on the MT.

Before this CL, the plumbing for aborting the animation went through
LayerImpl::set_main_thread_scrolling_reasons, which is reworked with property
trees. This CL uses the communication scheme described in http://bit.ly/1Y6RA6y
to send a "takeover" update to CC.

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

to

==========
Send takeover msg from MT to CC using the animation path.

This is a follow-up CL to https://crrev.com/1950243005/

When a main thread scrolling reason gets added while there is a scroll offset
animation running on CC, the animation is aborted on CC and finished on the MT.

Before this CL, the plumbing for aborting the animation went through
LayerImpl::set_main_thread_scrolling_reasons, which is reworked with property
trees. This CL uses the communication scheme described in http://bit.ly/1Y6RA6y
to send a "takeover" update to CC.

BUG=607047
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c
Cr-Commit-Position: refs/heads/master@{#396061}
==========

[commit-bot: I haz the power, 2016-05-26 00:59:49]

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