Fix WindowTreeHostManager::GetPrimaryRootWindow.

Fix WindowTreeHostManager::GetPrimaryRootWindow

When the last display is removed, the |primary_display_id| is set to
invalid. In this case, we should return root window of the
|primary_tree_host_for_replace_|.

BUG=718232
Test=tested on local device

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

[wutao, 2017-05-10 19:17:16]

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

[wutao, 2017-05-10 19:17:16]

Hi Oshima,

This is a potential fix suggested by varkha@. Tested on local device. It works.
PTAL.

Thanks,
Tao

[wutao, 2017-05-10 19:17:25]

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

[commit-bot: I haz the power, 2017-05-10 19:18:50]

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-10 20:17:01]

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

[commit-bot: I haz the power, 2017-05-10 20:17:02]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-05-10 22:23:59]

lgtm with a nit and a tangential comment. Thanks for looking into this!

https://codereview.chromium.org/2870253006/diff/1/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2870253006/diff/1/ash/shell.cc#newcode272
ash/shell.cc:272: WmWindow* wm_window =
instance_->shell_port_->GetPrimaryRootWindow();
Not for this CL - WmWindow should probably go away here as we are using Aura as
a client lib in mus_ash. Maybe a TODO to make shell_port.h use aura::Window* in
place of WmWindow *.

https://codereview.chromium.org/2870253006/diff/1/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

https://codereview.chromium.org/2870253006/diff/1/ash/wm/window_util.cc#newco...
ash/wm/window_util.cc:66: aura::Window* GetActiveWindow() {
nit: Maybe a comment about when primary root window might be null.

[varkha, 2017-05-10 22:25:21]

I wonder if there is a chance to add a test that tries removing a primary root
window.

[wutao, 2017-05-10 22:41:58]

On 2017/05/10 22:25:21, varkha wrote:
> I wonder if there is a chance to add a test that tries removing a primary root
> window.

What to test in this case?
Can we force to set primary display id to invalid id. And test GetActiveWindow
should return null.

[oshima, 2017-05-10 22:51:05]

Can you hold a sec? I think there is a better way.

[oshima, 2017-05-11 21:28:45]

On 2017/05/10 22:51:05, oshima wrote:
> Can you hold a sec? I think there is a better way.

Can you instead try changing WidowTreeHostManager::GetPrimaryDisplay to return
the root window of primary_tree_host_for_replace_? You may want to send it to
try bots first
to see if there is any regression that it may cause.

[wutao, 2017-05-11 22:00:11]

Description was changed from

==========
Fix ash::wm::GetActiveWindow.

ash::Shell::GetPrimaryRootWindow could be nullptr when the only display
is removed. In this situation, ash::wm::GetActiveWindow should return
nullptr as well.

BUG=718232
Test=tested on local device
==========

to

==========
Fix WindowTreeHostManager::GetPrimaryRootWindow

When the last display is removed, the |primary_display_id| is set to
invalid. In this case, we should return root window of the
|primary_tree_host_for_replace_|.

BUG=718232
Test=tested on local device
==========

[wutao, 2017-05-11 22:14:42]

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

[commit-bot: I haz the power, 2017-05-11 22:16:15]

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-11 23:36:05]

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

[commit-bot: I haz the power, 2017-05-11 23:36:06]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-05-11 23:42:23]

Hi Oshima,

It passed all the tests. ptal. Thanks.

Tao

[oshima, 2017-05-12 00:19:21]

one more change, then lgtm

https://codereview.chromium.org/2870253006/diff/20001/ash/display/window_tree...
File ash/display/window_tree_host_manager.cc (right):

https://codereview.chromium.org/2870253006/diff/20001/ash/display/window_tree...
ash/display/window_tree_host_manager.cc:639: controller->Shutdown();
can you reset the primary_tree_host_for_replace_ if it's same as host_to_delete
here?

[wutao, 2017-05-12 00:43:57]

Hi Oshima,

ptal.

Thank you,
Tao

https://codereview.chromium.org/2870253006/diff/20001/ash/display/window_tree...
File ash/display/window_tree_host_manager.cc (right):

https://codereview.chromium.org/2870253006/diff/20001/ash/display/window_tree...
ash/display/window_tree_host_manager.cc:639: controller->Shutdown();
On 2017/05/12 00:19:21, oshima wrote:
> can you reset the primary_tree_host_for_replace_ if it's same as
host_to_delete
> here?

Done.

[wutao, 2017-05-12 00:44:36]

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

[commit-bot: I haz the power, 2017-05-12 00:45:39]

Dry run: CQ is trying da patch.

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

[oshima, 2017-05-12 00:48:45]

slgtm

[commit-bot: I haz the power, 2017-05-12 01:23:57]

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

[commit-bot: I haz the power, 2017-05-12 01:23:58]

Dry run: This issue passed the CQ dry run.

[wutao, 2017-05-12 01:41:21]

The CQ bit was checked by wutao@chromium.org

[wutao, 2017-05-12 01:41:21]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2870253006/#ps40001
(title: "Reset primary_tree_host_for_replace_.")

[commit-bot: I haz the power, 2017-05-12 01:42:05]

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-12 01:49:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494553281060280,
"parent_rev": "59da93bfe66eddf5da25f6cae760fb2813e4ae8d", "commit_rev":
"e8ce0ec0131153014f66ef73920527c25c0589d8"}

[commit-bot: I haz the power, 2017-05-12 01:49:24]

Description was changed from

==========
Fix WindowTreeHostManager::GetPrimaryRootWindow

When the last display is removed, the |primary_display_id| is set to
invalid. In this case, we should return root window of the
|primary_tree_host_for_replace_|.

BUG=718232
Test=tested on local device
==========

to

==========
Fix WindowTreeHostManager::GetPrimaryRootWindow

When the last display is removed, the |primary_display_id| is set to
invalid. In this case, we should return root window of the
|primary_tree_host_for_replace_|.

BUG=718232
Test=tested on local device

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

[commit-bot: I haz the power, 2017-05-12 01:49:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e8ce0ec0131153014f66ef739205...