Make WindowTree::SetModal() take the type.

Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
Review-Url: https://codereview.chromium.org/2710023007
Cr-Commit-Position: refs/heads/master@{#455064}
Committed: https://chromium.googlesource.com/chromium/src/+/46d0c110e7c68b484bfc292cf030d599c9bb8251

[Hadi, 2017-02-24 18:51:51]

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

[commit-bot: I haz the power, 2017-02-24 18:52:16]

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

[Hadi, 2017-02-24 18:58:09]

Description was changed from

==========
Make WindowTree::SetModal() take the type.

BUG=660073
==========

to

==========
Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

[commit-bot: I haz the power, 2017-02-24 19:08:49]

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

[commit-bot: I haz the power, 2017-02-24 19:08:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hadi, 2017-02-24 19:16:28]

Description was changed from

==========
Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

to

==========
(NOT for Review) Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

[sky, 2017-02-24 20:18:53]

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

[sky, 2017-02-24 20:18:54]

I only poked at a couple of things. I think you're on a good path. The biggest
thing meaning is propagating the type to the window manager (ash). You need not
do it in this patch, but just want you to be aware that we need that too.

https://codereview.chromium.org/2710023007/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2710023007/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:180: // Sets |window_id| to be
modal. If the window has a transient parent, then
You'll want to update this comment.

https://codereview.chromium.org/2710023007/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2710023007/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:363: window->SetModal(false);
I think SetModal should change to take the type.

[Hadi, 2017-02-28 19:58:18]

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

[commit-bot: I haz the power, 2017-02-28 19:58:49]

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

[Hadi, 2017-02-28 20:01:20]

Description was changed from

==========
(NOT for Review) Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

to

==========
Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

[Hadi, 2017-02-28 20:27:23]

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

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

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

[Hadi, 2017-02-28 20:29:06]

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

[Hadi, 2017-02-28 20:29:07]

Addressed feedback. PTAL.

tsepez@: please review ui/base/mojo and window_tree.mojom.

Thanks.

https://codereview.chromium.org/2710023007/diff/20001/services/ui/public/inte...
File services/ui/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/2710023007/diff/20001/services/ui/public/inte...
services/ui/public/interfaces/window_tree.mojom:180: // Sets |window_id| to be
modal. If the window has a transient parent, then
On 2017/02/24 20:18:53, sky wrote:
> You'll want to update this comment.

Done.

https://codereview.chromium.org/2710023007/diff/20001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2710023007/diff/20001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:363: window->SetModal(false);
On 2017/02/24 20:18:53, sky wrote:
> I think SetModal should change to take the type.

Done.

[Tom Sepez, 2017-02-28 20:36:36]

Mojom itself LGTM, but do you want to call this concept "Modality" instead of
"ModalType"?

[commit-bot: I haz the power, 2017-02-28 20:40:57]

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

[commit-bot: I haz the power, 2017-02-28 20:40:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hadi, 2017-02-28 21:15:46]

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

[commit-bot: I haz the power, 2017-02-28 21:16:28]

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

[Hadi, 2017-02-28 21:18:47]

Thanks for review.

On 2017/02/28 20:36:36, Tom Sepez wrote:
> Mojom itself LGTM, but do you want to call this concept "Modality" instead of
> "ModalType"?

In ui/base/ui_base_types.h the corresponding enum is called ui::ModalType, so I
felt a bit uncomfortable changing this to "Modality" without changing that one.

[commit-bot: I haz the power, 2017-02-28 22:10:09]

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

[commit-bot: I haz the power, 2017-02-28 22:10:09]

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-01 00:13:43]

Do you plan on changing propagating to window manager separately?

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window.h:135: void SetModal(ModalType modal_type);
SetModalType

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:363: switch (modal_type) {
Early out if modal_type matches?

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:364: case MODAL_TYPE_NONE:
Move this to MODEL_TYPE_WINDOW as it's the same implementation.

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:368: if (user_id_ == InvalidUserId() ||
!display_root)
Why the InvalidUserId check?

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:370:
display_root->window_manager_state()->AddSystemModalWindow(window);
How come you don't SetModal(modal_type) in this case?

https://codereview.chromium.org/2710023007/diff/80001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2710023007/diff/80001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:483:
ScheduleInFlightChange(base::MakeUnique<InFlightSetModalChange>(window));
Don't you need to change InFlightSetModalChange to consider the type now? Please
add test coverage of this.

https://codereview.chromium.org/2710023007/diff/80001/ui/base/mojo/ui_base_ty...
File ui/base/mojo/ui_base_types.typemap (right):

https://codereview.chromium.org/2710023007/diff/80001/ui/base/mojo/ui_base_ty...
ui/base/mojo/ui_base_types.typemap:1: # Copyright 2016 The Chromium Authors. All
rights reserved.
2017

https://codereview.chromium.org/2710023007/diff/80001/ui/base/mojo/ui_base_ty...
File ui/base/mojo/ui_base_types_struct_traits.h (right):

https://codereview.chromium.org/2710023007/diff/80001/ui/base/mojo/ui_base_ty...
ui/base/mojo/ui_base_types_struct_traits.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
2017

[Hadi, 2017-03-03 16:23:16]

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

[commit-bot: I haz the power, 2017-03-03 16:23:41]

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-03 16:59:53]

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

[commit-bot: I haz the power, 2017-03-03 16:59:54]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hadi, 2017-03-03 18:30:55]

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

[commit-bot: I haz the power, 2017-03-03 18:31:06]

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-03 18:51:04]

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

[commit-bot: I haz the power, 2017-03-03 18:51:17]

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-03 19:49:58]

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

[commit-bot: I haz the power, 2017-03-03 19:49:59]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-03-06 20:07:05]

On 2017/03/01 00:13:43, sky wrote:
> Do you plan on changing propagating to window manager separately?

Yes, that was the plan. I think we also may need to fix some event targeting
too, which I'll send as separate CLs.

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/server_w...
File services/ui/ws/server_window.h (right):

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/server_w...
services/ui/ws/server_window.h:135: void SetModal(ModalType modal_type);
On 2017/03/01 00:13:34, sky wrote:
> SetModalType

Done. Also changed WindowTree::SetModal() to WindowTree::SetModalType().

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:363: switch (modal_type) {
On 2017/03/01 00:13:35, sky wrote:
> Early out if modal_type matches?

Done. Should we return true or false in this case? (I assumed true).

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:364: case MODAL_TYPE_NONE:
On 2017/03/01 00:13:35, sky wrote:
> Move this to MODEL_TYPE_WINDOW as it's the same implementation.

I thought we shouldn't release capture when MODAL_TYPE_NONE. I agree that my
implementation was confusing and I should probably have checked this in the if()
for release capture. Changed this.

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:368: if (user_id_ == InvalidUserId() ||
!display_root)
On 2017/03/01 00:13:35, sky wrote:
> Why the InvalidUserId check?

The previous implementation checks for InvalidUserId for system modals. Also
window_tree.mojom says SetModal() can fail if client doesn't have a valid user
id.

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:370:
display_root->window_manager_state()->AddSystemModalWindow(window);
On 2017/03/01 00:13:35, sky wrote:
> How come you don't SetModal(modal_type) in this case?

AddSystemModalWindow() does this. I agree my implementation was a bit confusing,
so I also add SetModalType() to this case.

https://codereview.chromium.org/2710023007/diff/80001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2710023007/diff/80001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:483:
ScheduleInFlightChange(base::MakeUnique<InFlightSetModalChange>(window));
On 2017/03/01 00:13:43, sky wrote:
> Don't you need to change InFlightSetModalChange to consider the type now?
Please
> add test coverage of this.

Done. There was a test for TYPE_MODAL_NONE to TYPE_MODAL_WINDOW change fail. I
also (1) added a test for TYPE_MODAL_WINDOW to TYPE_MODAL_NONE fail, (2) changed
the ModalTypeSuccess test to check for setting successfully to TYPE_MODAL_SYSTEM
and TYPE_MODAL_NONE.

https://codereview.chromium.org/2710023007/diff/180001/services/ui/ws/server_...
File services/ui/ws/server_window.cc (left):

https://codereview.chromium.org/2710023007/diff/180001/services/ui/ws/server_...
services/ui/ws/server_window.cc:232: if (child->is_modal() &&
!child->transient_parent())
I am not sure if we should have retained this check or equivalent of this (i.e.
check if modal_type() == MODAL_TYPE_SYSTEM). Since we don't decide on system
modality based on not having transient parent anymore, I remove it.

[Hadi, 2017-03-06 20:08:06]

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

[commit-bot: I haz the power, 2017-03-06 20:08:59]

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-06 21:17:34]

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

[commit-bot: I haz the power, 2017-03-06 21:17:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sky, 2017-03-06 22:26:45]

LGTM

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2710023007/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:363: switch (modal_type) {
On 2017/03/06 20:07:05, Hadi wrote:
> On 2017/03/01 00:13:35, sky wrote:
> > Early out if modal_type matches?
> 
> Done. Should we return true or false in this case? (I assumed true).

Return true.

https://codereview.chromium.org/2710023007/diff/180001/ui/aura/mus/in_flight_...
File ui/aura/mus/in_flight_change.h (right):

https://codereview.chromium.org/2710023007/diff/180001/ui/aura/mus/in_flight_...
ui/aura/mus/in_flight_change.h:319: explicit
InFlightSetModalTypeChange(WindowMus* window,
no explicit

[Hadi, 2017-03-07 02:25:53]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-03-07 02:25:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2710023007/#ps200001
(title: "address feedback.")

[commit-bot: I haz the power, 2017-03-07 02:26:56]

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-07 04:19:33]

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

[commit-bot: I haz the power, 2017-03-07 04:19:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Hadi, 2017-03-07 04:21:39]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-03-07 04:22:23]

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-07 07:42:07]

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

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

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Hadi, 2017-03-07 12:06:04]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-03-07 12:06:16]

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-07 13:55:47]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488888364046070,
"parent_rev": "93f6cf7e294c59ec6f53b89c078073fc6da31433", "commit_rev":
"46d0c110e7c68b484bfc292cf030d599c9bb8251"}

[commit-bot: I haz the power, 2017-03-07 13:56:34]

Description was changed from

==========
Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073
==========

to

==========
Make WindowTree::SetModal() take the type.

Previously system modal was implicitly inferred as a modal window who
doesn't have a transient parent. But this is not true always, for example
a JS alert window can be system modal but also have a transient
parent.

This CL adds modality type to WindowTree::SetModal() so window server is
explicitly told the type of modality a window should take. 

BUG=660073

Review-Url: https://codereview.chromium.org/2710023007
Cr-Commit-Position: refs/heads/master@{#455064}
Committed:
https://chromium.googlesource.com/chromium/src/+/46d0c110e7c68b484bfc292cf030...
==========

[commit-bot: I haz the power, 2017-03-07 13:56:37]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/46d0c110e7c68b484bfc292cf030...