Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870023002:
Allow compositor locks to extend timeout. (Closed)
DescriptionAllow compositor locks to extend timeout.
Currently compositor lock will set a timeout with the timeout of the
first lock request. Incoming future locks will timeout with the same
time. Adding a flag to allow compositor locks to extend timeout based on
the most future timeout of all the locks when the flag is true.
BUG=678763
TEST=CompositorTestWithMockedTime.* && tested in screen rotation animation with wallpaper resize
Review-Url: https://codereview.chromium.org/2870023002
Cr-Commit-Position: refs/heads/master@{#470658}
Committed: https://chromium.googlesource.com/chromium/src/+/c5cabd63ff5c51e67270774173cc0f66059cf341
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add tests. #
Total comments: 18
Patch Set 3 : Add more scopes for EXPECT_CALL. #Patch Set 4 : Rebase. #
Messages
Total messages: 36 (27 generated)
wutao@google.com changed reviewers: + danakj@chromium.org, oshima@chromium.org, wutao@google.com
Hi Dana and Oshima, Please take a look. The usage will be: compositor->set_allow_locks_to_extend_timeout(true); SetDisplayRotation(); compositor->set_allow_locks_to_extend_timeout(false); Thanks. Tao
LG but can you add some tests? https://codereview.chromium.org/2870023002/diff/1/ui/compositor/compositor.h File ui/compositor/compositor.h (right): https://codereview.chromium.org/2870023002/diff/1/ui/compositor/compositor.h#... ui/compositor/compositor.h:425: // The estimated time that the locks will be timedout. will time out.
Description was changed from ========== Allow compositor locks to extend timeout. Currently compositor lock will set a timeout with the timeout of the first lock request. Incoming future locks will timeout with the same time. Adding a flag to allow compositor locks to extend timeout based on the most future timeout of all the locks when the flag is true. BUG=678763 TEST=manually ========== to ========== Allow compositor locks to extend timeout. Currently compositor lock will set a timeout with the timeout of the first lock request. Incoming future locks will timeout with the same time. Adding a flag to allow compositor locks to extend timeout based on the most future timeout of all the locks when the flag is true. BUG=678763 TEST=CompositorTestWithMockedTime.* && tested in screen rotation animation with wallpaper resize ==========
wutao@chromium.org changed reviewers: - wutao@google.com
Hi Dana, ptal. Thanks, Tao
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Thanks! The tests look good, my feedback about the mock usage is to scope the EXPECT_CALLs down more so you know which part of the function is actually calling them. Right now they could be called from many places. I've left my thoughts on how to do that. LGTM once that is resolved. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:265: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); Times(0) here https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:271: task_runner()->FastForwardBy(timeout2 - timeout1); EXPECT_CALL Times(1) here https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:300: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); Times(0) for both clients here right? https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:305: EXPECT_CALL(lock_client2, CompositorLockTimedOut()).Times(1); and Times(1) here for both? https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:330: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); 0 https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:336: task_runner()->FastForwardBy(timeout1 - timeout2); Times(1) down here for both https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:352: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); Also EXPECT_CALL(client2).Times(0) https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:359: // |lock1| is timeout. The second lock can timeout on its own. is timed out already. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:363: EXPECT_CALL(lock_client2, CompositorLockTimedOut()).Times(1); Also EXPECT_CALL(client1).Times(0)
Updated the tests. Hi Oshima, what do you think. Thanks, Tao https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... File ui/compositor/compositor_unittest.cc (right): https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:265: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > Times(0) here Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:271: task_runner()->FastForwardBy(timeout2 - timeout1); On 2017/05/09 22:23:19, danakj wrote: > EXPECT_CALL Times(1) here Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:300: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > Times(0) for both clients here right? Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:305: EXPECT_CALL(lock_client2, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > and Times(1) here for both? Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:330: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > 0 Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:336: task_runner()->FastForwardBy(timeout1 - timeout2); On 2017/05/09 22:23:19, danakj wrote: > Times(1) down here for both Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:352: EXPECT_CALL(lock_client1, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > Also EXPECT_CALL(client2).Times(0) Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:359: // |lock1| is timeout. The second lock can timeout on its own. On 2017/05/09 22:23:19, danakj wrote: > is timed out already. Done. https://codereview.chromium.org/2870023002/diff/20001/ui/compositor/composito... ui/compositor/compositor_unittest.cc:363: EXPECT_CALL(lock_client2, CompositorLockTimedOut()).Times(1); On 2017/05/09 22:23:19, danakj wrote: > Also EXPECT_CALL(client1).Times(0) Done.
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM
The CQ bit was checked by wutao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wutao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2870023002/#ps60001 (title: "Rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494441123952670,
"parent_rev": "50b2abf8195a44355d258a964613da0a1b8b5356", "commit_rev":
"c5cabd63ff5c51e67270774173cc0f66059cf341"}
Message was sent while issue was closed.
Description was changed from ========== Allow compositor locks to extend timeout. Currently compositor lock will set a timeout with the timeout of the first lock request. Incoming future locks will timeout with the same time. Adding a flag to allow compositor locks to extend timeout based on the most future timeout of all the locks when the flag is true. BUG=678763 TEST=CompositorTestWithMockedTime.* && tested in screen rotation animation with wallpaper resize ========== to ========== Allow compositor locks to extend timeout. Currently compositor lock will set a timeout with the timeout of the first lock request. Incoming future locks will timeout with the same time. Adding a flag to allow compositor locks to extend timeout based on the most future timeout of all the locks when the flag is true. BUG=678763 TEST=CompositorTestWithMockedTime.* && tested in screen rotation animation with wallpaper resize Review-Url: https://codereview.chromium.org/2870023002 Cr-Commit-Position: refs/heads/master@{#470658} Committed: https://chromium.googlesource.com/chromium/src/+/c5cabd63ff5c51e67270774173cc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c5cabd63ff5c51e67270774173cc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
