Inform window manager about modal windows in mus+ash.

Inform window manager about modal windows in mus+ash.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996
Review-Url: https://codereview.chromium.org/2745143004
Cr-Commit-Position: refs/heads/master@{#458065}
Committed: https://chromium.googlesource.com/chromium/src/+/edab7eb543505b23c3618767d40e2aa4f4ddeb19

[Hadi, 2017-03-14 16:07:22]

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

[commit-bot: I haz the power, 2017-03-14 16:07:52]

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-14 16:57:30]

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

[commit-bot: I haz the power, 2017-03-14 16:57:31]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-03-14 19:36:34]

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

[commit-bot: I haz the power, 2017-03-14 19:37:13]

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-14 19:38:46]

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

[Hadi, 2017-03-14 19:38:46]

PTAL.

How do we write tests for changes like this? I couldn't find tests for similar
situations.

[Hadi, 2017-03-14 19:41:14]

Description was changed from

==========
Inform window manager about modal windows.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

BUG=645996
==========

to

==========
Inform window manager about modal windows.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating the window when clicked outside of it), but for the rest we need more
wiring (e.g. no grey background yet) which will be done in subsequent CLs.

BUG=645996
==========

[Hadi, 2017-03-14 19:41:46]

Description was changed from

==========
Inform window manager about modal windows.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating the window when clicked outside of it), but for the rest we need more
wiring (e.g. no grey background yet) which will be done in subsequent CLs.

BUG=645996
==========

to

==========
Inform window manager about modal windows.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996
==========

[Hadi, 2017-03-14 19:44:33]

Description was changed from

==========
Inform window manager about modal windows.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996
==========

to

==========
Inform window manager about modal windows in mus+ash.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996
==========

[commit-bot: I haz the power, 2017-03-14 20:33:50]

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

[commit-bot: I haz the power, 2017-03-14 20:33:54]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-14 21:15:26]

On 2017/03/14 19:38:46, Hadi wrote:
> PTAL.
> 
> How do we write tests for changes like this? I couldn't find tests for similar
> situations.

Tests that verify the windowmanager is called appropriately are in
window_tree_client_unittest. You should be able to extend that for SetModal.
AuraTestBase is a WindowManagerDelegate. So you can add test coverage that the
aura calls the WindowManagerDelegate appropriately as well by extending
WindowTreeClient to have a function that calls WindowTreeClientPrivate.

[sky, 2017-03-14 21:16:23]

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

https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1298: WindowTree* wm_tree =
GetWindowManagerDisplayRoot(window)
As ash always allows the change I suggest you forward the change to ash, but
don't require a return value.

https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1302:
window_server_->GenerateWindowManagerChangeId(this, change_id),
If you generate a change id like this the expectation is that the window manager
would ack the change id. You don't have that here. Follow the pattern used in
SetCanFocus.

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

https://codereview.chromium.org/2745143004/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1405: DCHECK(window_manager_delegate_);
I think you should directly set the modal type here. I can't think of a reason
to call the delegate. IF we need to add it at some point, we can do that.

[Hadi, 2017-03-15 15:36:10]

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

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

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-15 16:50:44]

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

[commit-bot: I haz the power, 2017-03-15 16:50:45]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-03-17 15:46:31]

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

[commit-bot: I haz the power, 2017-03-17 15:47:02]

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 16:16:32]

Addressed feedback and added a test to window_tree_unittest.cc to check that it
forwards the change to window manager.

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

https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1298: WindowTree* wm_tree =
GetWindowManagerDisplayRoot(window)
On 2017/03/14 21:16:22, sky wrote:
> As ash always allows the change I suggest you forward the change to ash, but
> don't require a return value.

Done. I also moved the forwarding part to the other overload of SetModalType()
so I can test forwarding in window_tree_unittest.

https://codereview.chromium.org/2745143004/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1302:
window_server_->GenerateWindowManagerChangeId(this, change_id),
On 2017/03/14 21:16:23, sky wrote:
> If you generate a change id like this the expectation is that the window
manager
> would ack the change id. You don't have that here. Follow the pattern used in
> SetCanFocus.

Done.

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

https://codereview.chromium.org/2745143004/diff/40001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1405: DCHECK(window_manager_delegate_);
On 2017/03/14 21:16:23, sky wrote:
> I think you should directly set the modal type here. I can't think of a reason
> to call the delegate. IF we need to add it at some point, we can do that.

Done.

[Hadi, 2017-03-17 16:18:42]

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

[Hadi, 2017-03-17 16:18:43]

tsepez@ can you please review window_manager.mojom?

Thanks.

[commit-bot: I haz the power, 2017-03-17 16:19:03]

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

[commit-bot: I haz the power, 2017-03-17 16:19:07]

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

[Tom Sepez, 2017-03-17 17:03:12]

lgtm

[Hadi, 2017-03-17 17:29:19]

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

[commit-bot: I haz the power, 2017-03-17 17:29:53]

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-17 18:24:29]

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

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

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-17 19:07:06]

LGTM

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

https://codereview.chromium.org/2745143004/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree_unittest.cc:1400: TestWindowTreeBinding*
child_binding;
= nullptr

[Hadi, 2017-03-17 19:10:03]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-03-17 19:10:03]

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/2745143004/#ps100001
(title: "Addressed feedback.")

[commit-bot: I haz the power, 2017-03-17 19:10:52]

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-17 20:31:41]

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

[commit-bot: I haz the power, 2017-03-17 20:31:42]

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

[Hadi, 2017-03-18 05:14:08]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-03-18 05:14: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-03-18 07:30:03]

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

[commit-bot: I haz the power, 2017-03-18 07:30:04]

Failed to apply patch for ui/aura/mus/window_tree_client.h:
While running git apply --index -p1;
  error: patch failed: ui/aura/mus/window_tree_client.h:29
  error: ui/aura/mus/window_tree_client.h: patch does not apply

Patch:       ui/aura/mus/window_tree_client.h
Index: ui/aura/mus/window_tree_client.h
diff --git a/ui/aura/mus/window_tree_client.h b/ui/aura/mus/window_tree_client.h
index
63073714c818f062032f27e0558111c8cbff5323..633fa9cd6a25b56fb98cfda8d4b37d114edbebb5
100644
--- a/ui/aura/mus/window_tree_client.h
+++ b/ui/aura/mus/window_tree_client.h
@@ -29,6 +29,7 @@
 #include "ui/aura/mus/mus_types.h"
 #include "ui/aura/mus/window_manager_delegate.h"
 #include "ui/aura/mus/window_tree_host_mus_delegate.h"
+#include "ui/base/ui_base_types.h"
 
 namespace display {
 class Display;
@@ -391,6 +392,7 @@ class AURA_EXPORT WindowTreeClient
       Id window_id,
       const std::string& name,
       const base::Optional<std::vector<uint8_t>>& transit_data) override;
+  void WmSetModalType(Id window_id, ui::ModalType type) override;
   void WmSetCanFocus(Id window_id, bool can_focus) override;
   void WmCreateTopLevelWindow(
       uint32_t change_id,

[Hadi, 2017-03-20 13:17:17]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2017-03-20 13:17:17]

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/2745143004/#ps120001
(title: "rebased.")

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

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 15:15:38]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490015837421440,
"parent_rev": "ab1f101d05be92332f8c4a8532f6807e580c0568", "commit_rev":
"edab7eb543505b23c3618767d40e2aa4f4ddeb19"}

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

Description was changed from

==========
Inform window manager about modal windows in mus+ash.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996
==========

to

==========
Inform window manager about modal windows in mus+ash.

We need to inform window manager about modal windows. This is necessary because
for example SystemModalContainerLayoutManager needs to know about modal types
so it can create the fullscreen grey background for system modals.

After this change we have some of the WM effects for modal windows (e.g.
animating system modal windows when clicked outside of it), but for
the rest we need more wiring (e.g. no grey background yet) which will
be done in subsequent CLs.

BUG=645996

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

[commit-bot: I haz the power, 2017-03-20 15:16:25]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/edab7eb543505b23c3618767d40e...