Add system modals to the proper container in mus+ash.

Add system modals to the proper container in mus+ash.

System modal windows should be added to kShellWindowId_SystemModalContainer so
we get effects like gray background for them.

BUG=702747
Review-Url: https://codereview.chromium.org/2759643003
Cr-Commit-Position: refs/heads/master@{#458533}
Committed: https://chromium.googlesource.com/chromium/src/+/92b3b48f68b255d6acc8efee1ac5682684676bba

[Hadi, 2017-03-17 20:06:00]

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

[commit-bot: I haz the power, 2017-03-17 20:06:25]

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

[Hadi, 2017-03-17 20:07:08]

moshayedi@chromium.org changed reviewers:
+ sky@chromium.org

[Hadi, 2017-03-17 20:07:08]

PTAL.

[commit-bot: I haz the power, 2017-03-17 20:28:21]

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

[commit-bot: I haz the power, 2017-03-17 20:28:22]

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_...)

[sky, 2017-03-17 21:37:47]

https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:325: void
WindowManager::OnWmSetModalType(aura::Window* window, ui::ModalType type) {
I'm a little surprised we need this, but I guess that's because classic ash only
allows the modality to be set before parenting.

https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:336: int32_t new_container_id = type ==
ui::MODAL_TYPE_SYSTEM
Can you use GetDefaultParent? In container_finder?

[Hadi, 2017-03-20 13:53:48]

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

[commit-bot: I haz the power, 2017-03-20 13:54:04]

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

[Hadi, 2017-03-20 15:03:29]

Couple of questions:

1. |context| is unused in ash::wm::GetDefaultParent(), should we remove it?

2. If not, what is a sensible value for it? (Comments say it is the new root,
which I suspect should be same as old root since display doesn't seem to change
here)

https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager....
ash/mus/window_manager.cc:336: int32_t new_container_id = type ==
ui::MODAL_TYPE_SYSTEM
On 2017/03/17 21:37:46, sky wrote:
> Can you use GetDefaultParent? In container_finder?

Done.

[Hadi, 2017-03-20 15:23:33]

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

[commit-bot: I haz the power, 2017-03-20 15:23:46]

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-03-20 16:28:29]

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

[commit-bot: I haz the power, 2017-03-20 16:28:30]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-20 16:37:14]

On 2017/03/20 15:03:29, Hadi wrote:
> Couple of questions:
> 
> 1. |context| is unused in ash::wm::GetDefaultParent(), should we remove it?

Indeed, it looks like it can be removed.

> 2. If not, what is a sensible value for it? (Comments say it is the new root,
> which I suspect should be same as old root since display doesn't seem to
change
> here)
> 
>
https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager.cc
> File ash/mus/window_manager.cc (right):
> 
>
https://codereview.chromium.org/2759643003/diff/40001/ash/mus/window_manager....
> ash/mus/window_manager.cc:336: int32_t new_container_id = type ==
> ui::MODAL_TYPE_SYSTEM
> On 2017/03/17 21:37:46, sky wrote:
> > Can you use GetDefaultParent? In container_finder?
> 
> Done.

[Hadi, 2017-03-20 18:41:33]

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

[commit-bot: I haz the power, 2017-03-20 18:42:08]

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

[Hadi, 2017-03-20 19:04:32]

On 2017/03/20 16:37:14, sky wrote:
> On 2017/03/20 15:03:29, Hadi wrote:
> > Couple of questions:
> > 
> > 1. |context| is unused in ash::wm::GetDefaultParent(), should we remove it?
> 
> Indeed, it looks like it can be removed.

Updated with |context| removed. PTAL.

[sky, 2017-03-20 20:14:30]

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager...
ash/mus/window_manager.cc:332: window->SetProperty(aura::client::kModalKey,
type);
If the property changes from system shouldn't you adjust the parent?

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager...
ash/mus/window_manager.cc:338: DCHECK(window->parent());
Why should the window already have a parent?

[Hadi, 2017-03-20 20:28:58]

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager...
ash/mus/window_manager.cc:332: window->SetProperty(aura::client::kModalKey,
type);
On 2017/03/20 20:14:30, sky wrote:
> If the property changes from system shouldn't you adjust the parent?

Right. The following if won't return if old_type is system, which takes care of
this.

https://codereview.chromium.org/2759643003/diff/140001/ash/mus/window_manager...
ash/mus/window_manager.cc:338: DCHECK(window->parent());
On 2017/03/20 20:14:30, sky wrote:
> Why should the window already have a parent?

Done.

[sky, 2017-03-20 21:53:24]

LGTM

[Hadi, 2017-03-21 18:49:14]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-03-21 18:50:01]

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-03-21 20:27:50]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490122154499030,
"parent_rev": "a362072374c53f432bc4cffa26489789f551d069", "commit_rev":
"92b3b48f68b255d6acc8efee1ac5682684676bba"}

[commit-bot: I haz the power, 2017-03-21 20:29:23]

Description was changed from

==========
Add system modals to the proper container in mus+ash.

System modal windows should be added to kShellWindowId_SystemModalContainer so
we get effects like gray background for them.

BUG=702747
==========

to

==========
Add system modals to the proper container in mus+ash.

System modal windows should be added to kShellWindowId_SystemModalContainer so
we get effects like gray background for them.

BUG=702747

Review-Url: https://codereview.chromium.org/2759643003
Cr-Commit-Position: refs/heads/master@{#458533}
Committed:
https://chromium.googlesource.com/chromium/src/+/92b3b48f68b255d6acc8efee1ac5...
==========

[commit-bot: I haz the power, 2017-03-21 20:29:24]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/92b3b48f68b255d6acc8efee1ac5...