Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809553002:
Fix corner case if another rotation request comes before the copy request finishes. (Closed)
DescriptionFix corner case if another rotation request comes before the copy request finishes.
We can return immediately if another rotation request comes before the
copy request finishes.
BUG=
Review-Url: https://codereview.chromium.org/2809553002
Cr-Commit-Position: refs/heads/master@{#463458}
Committed: https://chromium.googlesource.com/chromium/src/+/4a485532d97f667f4bdac8d81d992caf6e28872a
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add enum state for screen rotation animation. #
Total comments: 4
Patch Set 3 : Add TODO to handle cancel the copy request or ignore the copy result. #
Total comments: 2
Messages
Total messages: 23 (13 generated)
wutao@chromium.org changed reviewers: + oshima@chromium.org
Hi Oshima, Would you please have a look of this fix. Thank you. 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2809553002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:426: if (!old_layer_tree_owner_) what's the exact call sequence that reaches this condition? My suggestion is that instead of having simple "bool is_rotating", define state enum (e.g INIT, COPY_REQUESTED, ROTATING, DONE), and track the state and do the right thing based on the state.
Hi Oshima, Add the enum state, please review. Best, Tao https://codereview.chromium.org/2809553002/diff/1/ash/rotator/screen_rotation... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/1/ash/rotator/screen_rotation... ash/rotator/screen_rotation_animator.cc:426: if (!old_layer_tree_owner_) On 2017/04/10 18:38:46, oshima wrote: > what's the exact call sequence that reaches this condition? > > My suggestion is that instead of having simple "bool is_rotating", > define state enum (e.g INIT, COPY_REQUESTED, ROTATING, DONE), and track the > state and do the right thing based > on the state. Add the enum state, please review. Current sequence is: 1) rotation_request when idle -> rotate -> finish. 2) rotation_request when rotating -> finish current animation to target position and continue new rotation_request. When we adding the copy request, the sequence becomes: 1) rotation_request when idle -> send copy request -> wait the copy result -> rotate -> finish. 2) rotation_request when rotating -> finish current animation to target position and continue new rotation_request. 3) rotation_request when wait the copy result -> I think we should ignore the rotation request during this time. Otherwise, we need cancel the copy request and abort current rotation. This complicates the situation and gain little from it.
https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:385: switches::kAshEnableSmoothScreenRotation); was this for debugging? https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:402: break; We still need to handle this otherwise the device will be in inconsistent state. You may address this in a separate CL, but please add TODO.
Hi Oshima, I removed the debugging flag and added a TODO. I will start another cl to handle canceling the copy request or ignoring the copy result. Best, Tao https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:385: switches::kAshEnableSmoothScreenRotation); On 2017/04/10 22:56:59, oshima wrote: > was this for debugging? Thank you to find it out! Removed. https://codereview.chromium.org/2809553002/diff/20001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:402: break; On 2017/04/10 22:56:59, oshima wrote: > We still need to handle this otherwise the device will be in inconsistent state. > You may address this in a separate CL, but please add TODO. Thanks, I will address this in a separate CL.
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...
https://codereview.chromium.org/2809553002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:417: std::move(last_pending_request_); by the way, do you need to move this to stack?
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
https://codereview.chromium.org/2809553002/diff/40001/ash/rotator/screen_rota... File ash/rotator/screen_rotation_animator.cc (right): https://codereview.chromium.org/2809553002/diff/40001/ash/rotator/screen_rota... ash/rotator/screen_rotation_animator.cc:417: std::move(last_pending_request_); On 2017/04/10 23:20:39, oshima wrote: > by the way, do you need to move this to stack? Talked offline. I will change the unique_ptr to local variable in the other cl. We should be able reset |last_pending_request_| after calling |Rotate()|. It should never happen when we reach this code again and |last_pending_request_| has not been cleared. But simpler way is to just keep the rotation/source on the stack and reset before calling it.
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": 40001, "attempt_start_ts": 1491868556977510,
"parent_rev": "e7ecb635ed4e9aa37627a2d5278c5d24b82f41fb", "commit_rev":
"4a485532d97f667f4bdac8d81d992caf6e28872a"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491868556977510,
"parent_rev": "e7ecb635ed4e9aa37627a2d5278c5d24b82f41fb", "commit_rev":
"4a485532d97f667f4bdac8d81d992caf6e28872a"}
Message was sent while issue was closed.
Description was changed from ========== Fix corner case if another rotation request comes before the copy request finishes. We can return immediately if another rotation request comes before the copy request finishes. BUG= ========== to ========== Fix corner case if another rotation request comes before the copy request finishes. We can return immediately if another rotation request comes before the copy request finishes. BUG= Review-Url: https://codereview.chromium.org/2809553002 Cr-Commit-Position: refs/heads/master@{#463458} Committed: https://chromium.googlesource.com/chromium/src/+/4a485532d97f667f4bdac8d81d99... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/4a485532d97f667f4bdac8d81d99... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
