New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 607047 link

Starred by 1 user
Status: Fixed
Owner:
█ ymalik@chromium.org
(currently inactive on Chromium)
Closed: May 2016
Cc:
loyso@chromium.org
ajuma@chromium.org
skobes@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocking:
issue 594456



Sign in to add a comment

Plumb scroll offset animation takeover logic through the animation pipeline

Project Member Reported by ymalik@chromium.org, Apr 27 2016 
Currently, 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.

The plumbing for aborting the animation goes through LayerImpl::set_main_thread_scrolling_reasons, which will go away soon.

The right thing to do is probably to plumb something through the animation pipeline.

Note that there are also no integration tests that check whether an aborted animation is completed on the main thread, so if LayerImpl goes away, we will loose the capability of sharing animations from cc->mt without any warning. The first step here is to probably write a test in animation_timeline_unittest.cc.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 28 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/04787a7fb5dcf62e0e9766e4c459b630ebaddf73

commit 04787a7fb5dcf62e0e9766e4c459b630ebaddf73
Author: ymalik <ymalik@chromium.org>
Date: Thu Apr 28 15:49:31 2016

Add an integration test for the animation takeover path from cc to mt

Currently, when a main thread scrolling reason gets added,
LayerImpl::set_main_thread_scrolling_reasons tells the animation controller to
abort the animation running on CC and continue it on MT. This function will be
gone eventually (since these reasons are already stored in the scroll tree) and
we won't notice the takeover logic breaking as there is no test for it.

This CL adds a test for it. Note that it contains no behavioral change.

BUG= 607047 
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review-Url: https://codereview.chromium.org/1927833002
Cr-Commit-Position: refs/heads/master@{#390395}

[modify] https://crrev.com/04787a7fb5dcf62e0e9766e4c459b630ebaddf73/cc/trees/layer_tree_host_unittest_animation.cc
Project Member

Comment 2 by bugdroid1@chromium.org, May 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c34ac5562280bc5efd47a23f6974620ea7b757c

commit 4c34ac5562280bc5efd47a23f6974620ea7b757c
Author: ymalik <ymalik@chromium.org>
Date: Thu May 26 00:58:29 2016

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

Review-Url: https://codereview.chromium.org/2006103004
Cr-Commit-Position: refs/heads/master@{#396061}

[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/animation/animation_host.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/animation/animation_host.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/animation/scroll_offset_animations.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/animation/scroll_offset_animations.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/layers/layer_impl.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/layers/layer_impl.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/trees/layer_tree_host_unittest_animation.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/trees/layer_tree_impl.cc
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/cc/trees/layer_tree_impl.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/core/layout/ScrollAnchor.cpp
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/animation/CompositorAnimationHost.cpp
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/animation/CompositorAnimationHost.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimator.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.cpp
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimatorCompositorCoordinator.h
[modify] https://crrev.com/4c34ac5562280bc5efd47a23f6974620ea7b757c/third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp

Comment 3 by loyso@chromium.org, May 26 2016

Cc: loyso@chromium.org
Components: -Blink>Animation
Labels: -Type-Bug OS-All Type-Feature
Removing Blink>Animation label, the Blink>Scroll initiated these changes.

Comment 4 by ymalik@chromium.org, May 26 2016

Status: Fixed (was: Available)
►

Sign in to add a comment