cros: Do not cache |root_window| in ScreenRotationAnimator.

cros: Do not cache |root_window| in ScreenRotationAnimator.

When removing a display the |root_window| might change. The cache does
not work in this situation.

BUG=728029
TEST=manual and ScreenRotationAnimatorSmoothAnimationTest.*

Review-Url: https://codereview.chromium.org/2910413002
Cr-Commit-Position: refs/heads/master@{#476723}
Committed: https://chromium.googlesource.com/chromium/src/+/c18f66cce628e31fbde34fa521e526fafeab4839

[wutao, 2017-05-31 07:18:50]

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

[wutao, 2017-05-31 07:18:50]

Hi Oshima,

Please have a look. Also added a test.

Thanks.
Tao

[wutao, 2017-05-31 07:19:03]

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

[commit-bot: I haz the power, 2017-05-31 07:19:19]

Dry run: 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-05-31 08:47:03]

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

[commit-bot: I haz the power, 2017-05-31 08:47:03]

Dry run: This issue passed the CQ dry run.

[oshima, 2017-05-31 22:37:58]

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

https://codereview.chromium.org/2910413002/diff/1/ash/rotator/screen_rotation...
ash/rotator/screen_rotation_animator.cc:328: if
(!root_window->layer()->Contains(old_layer_tree_owner_->root())) {
Hmm, I probably made a bad recommendation. I'm very sorry about that.

It's probably better to create a rotator for root window (then you don't have to
worry about these condition, or invalid display in this class).

In GetScreenRotationAnimatorForDisplay, you can find the root window, and then
create the animator for the root window.

[wutao, 2017-06-01 07:59:25]

On 2017/05/31 22:37:58, oshima wrote:
>
https://codereview.chromium.org/2910413002/diff/1/ash/rotator/screen_rotation...
> File ash/rotator/screen_rotation_animator.cc (right):
> 
>
https://codereview.chromium.org/2910413002/diff/1/ash/rotator/screen_rotation...
> ash/rotator/screen_rotation_animator.cc:328: if
> (!root_window->layer()->Contains(old_layer_tree_owner_->root())) {
> Hmm, I probably made a bad recommendation. I'm very sorry about that.
> 
> It's probably better to create a rotator for root window (then you don't have
to
> worry about these condition, or invalid display in this class).
> 
> In GetScreenRotationAnimatorForDisplay, you can find the root window, and then
> create the animator for the root window.

Uploaded a new patch. At some cases, we still need to check invalid display and
consistent of the |root_window_| and the GetRootWindow(display_id).
Please see the code for details.

[wutao, 2017-06-01 07:59:55]

Hi Oshima,

Please have a look.

Thank you,
Tao

[wutao, 2017-06-01 08:00:05]

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

[commit-bot: I haz the power, 2017-06-01 08:00:21]

Dry run: 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-06-01 08:47:35]

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

[commit-bot: I haz the power, 2017-06-01 08:47:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[oshima, 2017-06-01 15:55:00]

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:278: if
(!IsDisplayIdValid(rotation_request->display_id) ||
you should be able t remove this because if it's invalid, then this root window
should have new display id.

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:318: if
(!IsDisplayIdValid(rotation_request->display_id) ||
ditto

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:446: void
ScreenRotationAnimator::Rotate(int64_t display_id,
Use the display id of the root window instead.

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:488:
IsDisplayIdValid(last_pending_request_->display_id) &&
ditto

[wutao, 2017-06-01 18:18:47]

On 2017/06/01 15:55:00, oshima wrote:
>
https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
> File ash/rotator/screen_rotation_animator.cc (right):
> 
>
https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
> ash/rotator/screen_rotation_animator.cc:278: if
> (!IsDisplayIdValid(rotation_request->display_id) ||
> you should be able t remove this because if it's invalid, then this root
window
> should have new display id.
> 
>
https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
> ash/rotator/screen_rotation_animator.cc:318: if
> (!IsDisplayIdValid(rotation_request->display_id) ||
> ditto
> 
>
https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
> ash/rotator/screen_rotation_animator.cc:446: void
> ScreenRotationAnimator::Rotate(int64_t display_id,
> Use the display id of the root window instead.
> 
>
https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
> ash/rotator/screen_rotation_animator.cc:488:
> IsDisplayIdValid(last_pending_request_->display_id) &&
> ditto

One exception case is that, when the external display is the primary display.
In this case, when rotating the external display and remove it during animation,
the |root_window| will be moved to internal display.
If we do not check or stop, it will continue rotating the internal display.
If this is want we want?
If yes, some corner cases need to handle, because the original/cached
rotation_request's old/new rotation may not be the same as the internal display.

[wutao, 2017-06-01 22:48:04]

Hi Oshima,

Please take another look.

Thank you,
Tao

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
File ash/rotator/screen_rotation_animator.cc (right):

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:278: if
(!IsDisplayIdValid(rotation_request->display_id) ||
On 2017/06/01 15:54:59, oshima wrote:
> you should be able t remove this because if it's invalid, then this root
window
> should have new display id.

Done.

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:318: if
(!IsDisplayIdValid(rotation_request->display_id) ||
On 2017/06/01 15:54:59, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:446: void
ScreenRotationAnimator::Rotate(int64_t display_id,
On 2017/06/01 15:54:59, oshima wrote:
> Use the display id of the root window instead.

Done.

https://codereview.chromium.org/2910413002/diff/40001/ash/rotator/screen_rota...
ash/rotator/screen_rotation_animator.cc:488:
IsDisplayIdValid(last_pending_request_->display_id) &&
On 2017/06/01 15:54:59, oshima wrote:
> ditto

Done.

[oshima, 2017-06-02 16:13:43]

lgtm

[wutao, 2017-06-02 16:19:10]

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

[commit-bot: I haz the power, 2017-06-02 16:19:22]

Dry run: 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-06-02 18:00:31]

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

[commit-bot: I haz the power, 2017-06-02 18:00:32]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-06-02 18:05:02]

The CQ bit was checked by wutao@chromium.org

[commit-bot: I haz the power, 2017-06-02 18:05:21]

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-06-02 18:11:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496426702625130,
"parent_rev": "0665658248db9bab31c319a1d11279b2bed3b6df", "commit_rev":
"c18f66cce628e31fbde34fa521e526fafeab4839"}

[commit-bot: I haz the power, 2017-06-02 18:11:12]

Description was changed from

==========
cros: Do not cache |root_window| in ScreenRotationAnimator.

When removing a display the |root_window| might change. The cache does
not work in this situation.

BUG=728029
TEST=manual and ScreenRotationAnimatorSmoothAnimationTest.*
==========

to

==========
cros: Do not cache |root_window| in ScreenRotationAnimator.

When removing a display the |root_window| might change. The cache does
not work in this situation.

BUG=728029
TEST=manual and ScreenRotationAnimatorSmoothAnimationTest.*

Review-Url: https://codereview.chromium.org/2910413002
Cr-Commit-Position: refs/heads/master@{#476723}
Committed:
https://chromium.googlesource.com/chromium/src/+/c18f66cce628e31fbde34fa521e5...
==========

[commit-bot: I haz the power, 2017-06-02 18:11:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c18f66cce628e31fbde34fa521e5...