Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927833002:
Add an integration test for the animation takeover path from cc to mt (Closed)
DescriptionAdd 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
Committed: https://crrev.com/04787a7fb5dcf62e0e9766e4c459b630ebaddf73
Cr-Commit-Position: refs/heads/master@{#390395}
Patch Set 1 #
Total comments: 2
Patch Set 2 : review feedback #Patch Set 3 : rebase master #Messages
Total messages: 18 (9 generated)
Description was changed from ========== 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. LayerImpl will be gone eventually 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 ========== to ========== 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. LayerImpl will be gone eventually 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 ==========
ymalik@chromium.org changed reviewers: + ajuma@chromium.org
Generally looks good. For the description, note that LayerImpl will still be sticking around, but the pushing of main_thread_scrolling_reasons from Layer to LayerImpl will be going away (since these reasons are already stored in the scroll tree). Also, one nit about the test: https://codereview.chromium.org/1927833002/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/1927833002/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest_animation.cc:793: if (!have_animations) { Check host_impl->sync_tree()->source_frame_number() here, and only add the animation when this is 1.
Description was changed from ========== 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. LayerImpl will be gone eventually 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 ========== to ========== 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 ==========
PTAL :) Updated description and addressed comment. https://codereview.chromium.org/1927833002/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/1927833002/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest_animation.cc:793: if (!have_animations) { On 2016/04/28 13:43:54, ajuma wrote: > Check host_impl->sync_tree()->source_frame_number() here, and only add the > animation when this is 1. Confirmed with you that this should be 0. Done.
Thanks! lgtm
The CQ bit was checked by ymalik@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927833002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927833002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org Link to the patchset: https://codereview.chromium.org/1927833002/#ps40001 (title: "rebase master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927833002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927833002/40001
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/04787a7fb5dcf62e0e9766e4c459b630ebaddf73 Cr-Commit-Position: refs/heads/master@{#390395}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/04787a7fb5dcf62e0e9766e4c459b630ebaddf73 Cr-Commit-Position: refs/heads/master@{#390395} ========== |
