Handle new screen rotation request while waiting for the copy request callback.

Handle new screen rotation request while waiting for the copy request callback.

If there is a new screen rotation request while it is waiting for the
layer copy result, we should process the new rotation request and
ignore the previous layer copy result.

BUG=678763
TEST=Manual

Review-Url: https://codereview.chromium.org/2816683002
Cr-Commit-Position: refs/heads/master@{#464508}
Committed: https://chromium.googlesource.com/chromium/src/+/88733dff81ecec04786aaee3cf48f4bcd776cd80

[wutao, 2017-04-12 06:32:26]

wutao@chromium.org changed reviewers:
+ oshima@chromium.org

[wutao, 2017-04-12 06:32:27]

Hi Oshima,

Would you please have a look this approach to handle the copy request.

Thank you,
Tao

[wutao, 2017-04-13 16:45:59]

Hi Oshima,

Friendly ping.

Tao

[oshima, 2017-04-13 17:24:41]

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:247: if (rotation_request->id !=
rotation_request_id_) {
it's probably better to have DCHECK

rotation_request->id <= rotation_reuqest_id_

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:248: rotation_request.reset();
it'll be reset upon return (sorry I missed others in previous review)

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:406: default:
remove default (so that compiler can catch an error when you add new state but
forgot to update here).

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:425: ScreenRotationRequest
rotation_request = *last_pending_request_.get();
can you just keep the new_rotation and source on stack?

[wutao, 2017-04-13 17:51:12]

Hi Oshima,

Fixed as suggested. Thank you,

Tao

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:247: if (rotation_request->id !=
rotation_request_id_) {
On 2017/04/13 17:24:41, oshima wrote:
> it's probably better to have DCHECK
> 
> rotation_request->id <= rotation_reuqest_id_

Done.

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:248: rotation_request.reset();
On 2017/04/13 17:24:41, oshima wrote:
> it'll be reset upon return (sorry I missed others in previous review)

I removed this reset at 3 places. Please check it is correct.

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:406: default:
On 2017/04/13 17:24:41, oshima wrote:
> remove default (so that compiler can catch an error when you add new state but
> forgot to update here).

Done.

https://codereview.chromium.org/2816683002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:425: ScreenRotationRequest
rotation_request = *last_pending_request_.get();
On 2017/04/13 17:24:41, oshima wrote:
> can you just keep the new_rotation and source on stack?

Done.

[oshima, 2017-04-13 18:18:24]

lgtm

https://codereview.chromium.org/2816683002/diff/20001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator.h (right):

https://codereview.chromium.org/2816683002/diff/20001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.h:130: int64_t rotation_request_id_;
= 0

[wutao, 2017-04-13 18:53:58]

The CQ bit was checked by wutao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 18:54:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wutao, 2017-04-13 18:54:30]

https://codereview.chromium.org/2816683002/diff/20001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator.h (right):

https://codereview.chromium.org/2816683002/diff/20001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.h:130: int64_t rotation_request_id_;
On 2017/04/13 18:18:24, oshima wrote:
> = 0

Keep as it. I init it in the init list.

[commit-bot: I haz the power, 2017-04-13 19:28:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 19:28:32]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-04-13 19:30:51]

The CQ bit was checked by wutao@chromium.org

[commit-bot: I haz the power, 2017-04-13 19:31:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 19:39:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492111851096070,
"parent_rev": "4a7b0272fad569856d18cbf2265fdfb2ec29b4e1", "commit_rev":
"88733dff81ecec04786aaee3cf48f4bcd776cd80"}

[commit-bot: I haz the power, 2017-04-13 19:40:04]

Description was changed from

==========
Handle new screen rotation request while waiting for the copy request callback.

If there is a new screen rotation request while it is waiting for the
layer copy result, we should process the new rotation request and
ignore the previous layer copy result.

BUG=678763
TEST=Manual
==========

to

==========
Handle new screen rotation request while waiting for the copy request callback.

If there is a new screen rotation request while it is waiting for the
layer copy result, we should process the new rotation request and
ignore the previous layer copy result.

BUG=678763
TEST=Manual

Review-Url: https://codereview.chromium.org/2816683002
Cr-Commit-Position: refs/heads/master@{#464508}
Committed:
https://chromium.googlesource.com/chromium/src/+/88733dff81ecec04786aaee3cf48...
==========

[commit-bot: I haz the power, 2017-04-13 19:40:05]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/88733dff81ecec04786aaee3cf48...