Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1971063004:
cc: Fix cc_unittests when main frame before activation is enabled. (Closed)
Descriptioncc: Fix cc_unittests when main frame before activation is enabled.
The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test
would time out because impl side would keep animating indefinitely. If
main frame before activation (MFBA) is enabled the test would keep
running because it waits for main frames to stop before ending.
A few scheduler state machine tests use the commit to active tree mode
which is incompatible with MFBA. One other test had to be modified to
remove unnecessary actions that would cause different behavior with
MFBA.
BUG=606210
R=enne@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/e85cf6b9adbf2f1ccfd6a9ea87c47cb186d549d6
Cr-Commit-Position: refs/heads/master@{#393962}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : rebase #
Depends on Patchset: Messages
Total messages: 22 (11 generated)
Description was changed from ========== cc: Fix animation test to not block activation indefinitely. When the main_frame_before_activation_enabled flag is enabled this test would time out because impl side would keep animating indefinitely. If main frames are sent before activation the test would keep running because it waits for main frames to stop before ending. BUG=606210 R=ajuma@chromium.org,enne@chromium.org ========== to ========== cc: Fix animation test to not block activation indefinitely. When the main_frame_before_activation_enabled flag is enabled this test would time out because impl side would keep animating indefinitely. If main frames are sent before activation the test would keep running because it waits for main frames to stop before ending. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
PTAL enne@ I removed a DCHECK from ProxyImpl because it doesn't hold when main_frame_before_activation_enabled is true. I can replace it with a more generic DCHECK but I think this sort of check belongs in the scheduler.
Actually nvm, turns out there was a bug in the scheduler that caused this so I'm closing this.
Description was changed from ========== cc: Fix animation test to not block activation indefinitely. When the main_frame_before_activation_enabled flag is enabled this test would time out because impl side would keep animating indefinitely. If main frames are sent before activation the test would keep running because it waits for main frames to stop before ending. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix animation test to not block activation indefinitely. When the main_frame_before_activation_enabled flag is enabled this test would time out because impl side would keep animating indefinitely. If main frames are sent before activation the test would keep running because it waits for main frames to stop before ending. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Actually nvm my previous comment, I was careless and didn't realize I wasn't testing with the right flag :/
https://codereview.chromium.org/1971063004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/1971063004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_animation.cc:920: if (scroll_delta.x() > 0.f || scroll_delta.y() > 0.f) This is the only real change in the patch, right? It's a little hard to see with the function moving. I'm not sure I understand what's happening with and without the command line flag wrt to this scroll delta. https://codereview.chromium.org/1971063004/diff/20001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (left): https://codereview.chromium.org/1971063004/diff/20001/cc/trees/proxy_impl.cc#... cc/trees/proxy_impl.cc:187: DCHECK(!layer_tree_host_impl_->pending_tree()); This seems reasonable to me. If we can send a main frame with the pending tree existing, I think it's fine to remove this, or move this dcheck to the scheduler.
Description was changed from ========== cc: Fix animation test to not block activation indefinitely. When the main_frame_before_activation_enabled flag is enabled this test would time out because impl side would keep animating indefinitely. If main frames are sent before activation the test would keep running because it waits for main frames to stop before ending. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
PTAL I made this CL fix all cc_unittests that break with main frame before activation. This is now dependent on https://codereview.chromium.org/1984453003/ which fixes a more serious issue with MFBA mode and aborted commits. https://codereview.chromium.org/1971063004/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest_animation.cc (right): https://codereview.chromium.org/1971063004/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest_animation.cc:920: if (scroll_delta.x() > 0.f || scroll_delta.y() > 0.f) On 2016/05/13 01:26:02, enne wrote: > This is the only real change in the patch, right? It's a little hard to see > with the function moving. I'm not sure I understand what's happening with and > without the command line flag wrt to this scroll delta. This isn't the substantive change of this CL. The big change is that BeginCommitOnImplThread blocks activation based on the same conditions as WillBeginImplFrameOnThread. So we don't keep blocking activation on every commit. https://codereview.chromium.org/1971063004/diff/20001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (left): https://codereview.chromium.org/1971063004/diff/20001/cc/trees/proxy_impl.cc#... cc/trees/proxy_impl.cc:187: DCHECK(!layer_tree_host_impl_->pending_tree()); On 2016/05/13 01:26:02, enne wrote: > This seems reasonable to me. If we can send a main frame with the pending tree > existing, I think it's fine to remove this, or move this dcheck to the > scheduler. I'm moving this change to https://codereview.chromium.org/1984453003/
Description was changed from ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
lgtm
Description was changed from ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=ajuma@chromium.org,enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by sunnyps@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org Link to the patchset: https://codereview.chromium.org/1971063004/#ps60001 (title: "rebase")
Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1984453003 Patch 20001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1971063004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1971063004/60001
Message was sent while issue was closed.
Description was changed from ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== cc: Fix cc_unittests when main frame before activation is enabled. The LayerTreeHostAnimationTestScrollOffsetAnimationRemoval test would time out because impl side would keep animating indefinitely. If main frame before activation (MFBA) is enabled the test would keep running because it waits for main frames to stop before ending. A few scheduler state machine tests use the commit to active tree mode which is incompatible with MFBA. One other test had to be modified to remove unnecessary actions that would cause different behavior with MFBA. BUG=606210 R=enne@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/e85cf6b9adbf2f1ccfd6a9ea87c47cb186d549d6 Cr-Commit-Position: refs/heads/master@{#393962} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e85cf6b9adbf2f1ccfd6a9ea87c47cb186d549d6 Cr-Commit-Position: refs/heads/master@{#393962} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
