Reland: mus: Enable system modal windows

Reland: mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402
Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}

Committed: https://crrev.com/3fe9bbb646a9408e6156012d6004a16903423363
Cr-Commit-Position: refs/heads/master@{#392007}

[mohsen, 2016-03-22 05:17:38]

Description was changed from

==========
mus: Enable system modal widnows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402
==========

to

==========
mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402
==========

[mohsen, 2016-04-20 19:37:47]

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

[mohsen, 2016-04-20 19:37:48]

Please take a look...

(I'll most probably add a few more tests).

[sky, 2016-04-20 20:14:12]

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:177: return false;
nit: spacing is off (run git cl format).

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:179: system_modal_window_ = window;
I think we should cancel capture here rather than relying on the client, it's
safer that way and makes sure EventDispatcher isn't left in a weird state.

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:477: if (system_modal_window_ == window) {
You have this in the destructor as well. Maybe ResetSystemModalWindow?

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
components/mus/ws/window_tree.cc:249: wms->SetSystemModalWindow(window);
This is problematic because you're assuming there is only ever one system modal
window. Lets say a client creates two system modal windows, A and B, and then
shows A. I believe that won't work well with how you have the code.

[mohsen, 2016-04-21 17:58:43]

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:177: return false;
On 2016/04/20 at 20:14:11, sky wrote:
> nit: spacing is off (run git cl format).

Done.

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:179: system_modal_window_ = window;
On 2016/04/20 at 20:14:11, sky wrote:
> I think we should cancel capture here rather than relying on the client, it's
safer that way and makes sure EventDispatcher isn't left in a weird state.

Done.

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
components/mus/ws/event_dispatcher.cc:477: if (system_modal_window_ == window) {
On 2016/04/20 at 20:14:11, sky wrote:
> You have this in the destructor as well. Maybe ResetSystemModalWindow?

Done.

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
components/mus/ws/window_tree.cc:249: wms->SetSystemModalWindow(window);
On 2016/04/20 at 20:14:12, sky wrote:
> This is problematic because you're assuming there is only ever one system
modal window. Lets say a client creates two system modal windows, A and B, and
then shows A. I believe that won't work well with how you have the code.

What would exactly not work in your example? The client creates A as system
modal, then creates B and tries to set it as system modal, but it fails and its
modality gets reverted. A remains system modal, B remains a regular window.

[sky, 2016-04-21 20:12:36]

On Thu, Apr 21, 2016 at 10:58 AM,  <mohsen@chromium.org> wrote:
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
> File components/mus/ws/event_dispatcher.cc (right):
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
> components/mus/ws/event_dispatcher.cc:177: return false;
> On 2016/04/20 at 20:14:11, sky wrote:
>> nit: spacing is off (run git cl format).
>
> Done.
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
> components/mus/ws/event_dispatcher.cc:179: system_modal_window_ =
> window;
> On 2016/04/20 at 20:14:11, sky wrote:
>> I think we should cancel capture here rather than relying on the
> client, it's safer that way and makes sure EventDispatcher isn't left in
> a weird state.
>
> Done.
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/event...
> components/mus/ws/event_dispatcher.cc:477: if (system_modal_window_ ==
> window) {
> On 2016/04/20 at 20:14:11, sky wrote:
>> You have this in the destructor as well. Maybe ResetSystemModalWindow?
>
> Done.
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
> File components/mus/ws/window_tree.cc (right):
>
>
https://codereview.chromium.org/1818333002/diff/60001/components/mus/ws/windo...
> components/mus/ws/window_tree.cc:249: wms->SetSystemModalWindow(window);
> On 2016/04/20 at 20:14:12, sky wrote:
>> This is problematic because you're assuming there is only ever one
> system modal window. Lets say a client creates two system modal windows,
> A and B, and then shows A. I believe that won't work well with how you
> have the code.
>
> What would exactly not work in your example? The client creates A as
> system modal, then creates B and tries to set it as system modal, but it
> fails and its modality gets reverted. A remains system modal, B remains
> a regular window.

Sorry, my example wasn't that good.
1. Create A
2. Set A as system modal (A is not shown yet).
3. Create B
4. Set B as system modal (B is not shown yet).
5. Show B

Other permutations are showing A, hiding A, then showing B.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2016-04-25 22:01:29]

Please take a look...

I added a stack of system modal windows, instead of just one. Do you think that
we need to move all system modal window code to a ModalWindowController (to
which we can probably add non-system modal code, too)?

[sky, 2016-04-25 23:36:08]

https://codereview.chromium.org/1818333002/diff/120001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1818333002/diff/120001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:177: ObserveWindow(window);
It's confusing and error prone to have this class track windows for two
purposes. By that I mean observe changes to drawn status for both windows
actively sending events to as well as possible modal windows.

I think the modal stack should be moved to a separate class that can handle
tracking and call back to this class as necessary. Is that what you meant by
ModalWindowController ?

[mohsen, 2016-04-26 20:24:05]

https://codereview.chromium.org/1818333002/diff/120001/components/mus/ws/even...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1818333002/diff/120001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:177: ObserveWindow(window);
On 2016/04/25 at 23:36:08, sky wrote:
> It's confusing and error prone to have this class track windows for two
purposes. By that I mean observe changes to drawn status for both windows
actively sending events to as well as possible modal windows.
> 
> I think the modal stack should be moved to a separate class that can handle
tracking and call back to this class as necessary. Is that what you meant by
ModalWindowController ?

I agree. Even before adding modal windows, it was observing for both active
pointer targets and capture window and it seemed confusing to me. That's why I
added ObserveWindow()/UnobserveWindow() to simply things a bit.

And, yes, that's exactly what I meant by ModalWindowController. Done; and moved
some other modal window functionality to it. However, I still think
ObserveWindow()/UnobserveWindow() are useful, so I kept them.

[sky, 2016-04-26 23:40:08]

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.cc (right):

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:47:
DCHECK(std::find(system_modal_windows_.begin(), system_modal_windows_.end(),
!ContainsValue (in stl_util).

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:101: if (!window->IsDrawn())
You need to use a ServerWindowDrawnTracker as OnWindowVisibilityChanged is only
called when the visibility of the window changes, which is not necessarily the
same as the drawn state.

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:107:
system_modal_windows_.splice(system_modal_windows_.begin(),
Don't you need to tell EventDispatcher when the modality changes? For example,
if a modal window is now drawn might capture need to change?

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.h (right):

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.h:19: class EventDispatcherTestApi;
Create a ModalWindowControllerTestApi class and friend it.

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.h:70: // List of system modal windows
in order they are added/shown.
Actually, this is reverse order, which I think it a bit confusing. How about
making it so last added is at the top?

[mohsen, 2016-04-27 20:18:25]

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.cc (right):

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:47:
DCHECK(std::find(system_modal_windows_.begin(), system_modal_windows_.end(),
On 2016/04/26 at 23:40:08, sky wrote:
> !ContainsValue (in stl_util).

Done.

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:101: if (!window->IsDrawn())
On 2016/04/26 at 23:40:08, sky wrote:
> You need to use a ServerWindowDrawnTracker as OnWindowVisibilityChanged is
only called when the visibility of the window changes, which is not necessarily
the same as the drawn state.

Done. I've used similar approach in WindowServer to release capture blocked by a
newly shown modal window. I think that one has a similar problem, but I'm not
sure if that would be efficient to use ServerWindowDrawnTracker there.

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:107:
system_modal_windows_.splice(system_modal_windows_.begin(),
On 2016/04/26 at 23:40:08, sky wrote:
> Don't you need to tell EventDispatcher when the modality changes? For example,
if a modal window is now drawn might capture need to change?

This is already handled in WindowServer, for all (system- and window-) modal
windows (using OnWindowVisibilityChanged() mentioned in my previous comment).

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.h (right):

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.h:19: class EventDispatcherTestApi;
On 2016/04/26 at 23:40:08, sky wrote:
> Create a ModalWindowControllerTestApi class and friend it.

Done.

https://codereview.chromium.org/1818333002/diff/160001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.h:70: // List of system modal windows
in order they are added/shown.
On 2016/04/26 at 23:40:08, sky wrote:
> Actually, this is reverse order, which I think it a bit confusing. How about
making it so last added is at the top?

They only reason I used reverse order is to be able to use range-based for loop
in GetActiveSystemModalWindow() which probably is not worth the confusion.
Fixed.

[sky, 2016-04-27 21:16:16]

LGTM

https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.cc (right):

https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:115:
system_modal_windows_.splice(system_modal_windows_.end(),
erase?

[mohsen, 2016-04-27 21:19:28]

https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
File components/mus/ws/modal_window_controller.cc (right):

https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
components/mus/ws/modal_window_controller.cc:115:
system_modal_windows_.splice(system_modal_windows_.end(),
On 2016/04/27 at 21:16:16, sky wrote:
> erase?

This is to move the most recently shown item to the end of the list. Makes
sense?

[sky, 2016-04-27 23:43:29]

Subtle. Make sense.

On Wed, Apr 27, 2016 at 2:19 PM,  <mohsen@chromium.org> wrote:
>
>
https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
> File components/mus/ws/modal_window_controller.cc (right):
>
>
https://codereview.chromium.org/1818333002/diff/180001/components/mus/ws/moda...
> components/mus/ws/modal_window_controller.cc:115:
> system_modal_windows_.splice(system_modal_windows_.end(),
> On 2016/04/27 at 21:16:16, sky wrote:
>> erase?
>
> This is to move the most recently shown item to the end of the list.
> Makes sense?
>
> https://codereview.chromium.org/1818333002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2016-04-28 16:57:18]

I added two more tests and cleaned up comments. Can you take another look,
please...

[sky, 2016-04-29 00:23:18]

LGTM

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:174: // This also places
|transient_window_id| always on top of |window_id|.
nit: remove 'always'.

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:175: // |transient_window_id|
should not be a system modal window.
Fails if |transient|window_id| is a system modal window.

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:187: // Sets |window_id| to
be modal. It would be modal to its transient parent if
If the window has a transient parent, then the window is modal to the transient
parent. Otherwise the window is modal to the system. This releases capture if
necessary.

[mohsen, 2016-04-29 03:33:38]

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
File components/mus/public/interfaces/window_tree.mojom (right):

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:174: // This also places
|transient_window_id| always on top of |window_id|.
On 2016/04/29 at 00:23:18, sky wrote:
> nit: remove 'always'.

Done.

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:175: // |transient_window_id|
should not be a system modal window.
On 2016/04/29 at 00:23:18, sky wrote:
> Fails if |transient|window_id| is a system modal window.

I think "Fails if" is used in comments whenever the action fails gracefully such
that the client is notified of the failure to revert the client-side value.
That's not the case here as we have a DCHECK. Right?

https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:187: // Sets |window_id| to
be modal. It would be modal to its transient parent if
On 2016/04/29 at 00:23:18, sky wrote:
> If the window has a transient parent, then the window is modal to the
transient parent. Otherwise the window is modal to the system. This releases
capture if necessary.

Done.

[sky, 2016-04-29 17:11:02]

On Thu, Apr 28, 2016 at 8:33 PM,  <mohsen@chromium.org> wrote:
>
>
https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
> File components/mus/public/interfaces/window_tree.mojom (right):
>
>
https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
> components/mus/public/interfaces/window_tree.mojom:174: // This also
> places |transient_window_id| always on top of |window_id|.
> On 2016/04/29 at 00:23:18, sky wrote:
>> nit: remove 'always'.
>
> Done.
>
>
https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
> components/mus/public/interfaces/window_tree.mojom:175: //
> |transient_window_id| should not be a system modal window.
> On 2016/04/29 at 00:23:18, sky wrote:
>> Fails if |transient|window_id| is a system modal window.
>
> I think "Fails if" is used in comments whenever the action fails
> gracefully such that the client is notified of the failure to revert the
> client-side value. That's not the case here as we have a DCHECK. Right?

Don't DCHECK (that could kill the server). Instead the request should fail.

In general the server needs to deal with a client doing bad/wrong
things and not DCHECK. At some point we may kill connections to mus if
the client does too much bad stuff.

  -Scott

>
>
https://codereview.chromium.org/1818333002/diff/200001/components/mus/public/...
> components/mus/public/interfaces/window_tree.mojom:187: // Sets
> |window_id| to be modal. It would be modal to its transient parent if
> On 2016/04/29 at 00:23:18, sky wrote:
>> If the window has a transient parent, then the window is modal to the
> transient parent. Otherwise the window is modal to the system. This
> releases capture if necessary.
>
> Done.
>
> https://codereview.chromium.org/1818333002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mohsen, 2016-04-29 20:40:25]

Fixed AddTransientWindow to fail on system modals.

[sky, 2016-05-02 15:15:35]

LGTM

[mohsen, 2016-05-02 15:32:16]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2016-05-02 15:32:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1818333002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1818333002/240001

[commit-bot: I haz the power, 2016-05-02 16:22:26]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-02 16:23:40]

Description was changed from

==========
mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402
==========

to

==========
mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

[commit-bot: I haz the power, 2016-05-02 16:23:41]

Patchset 13 (id:??) landed as
https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}

[Ben Goodger (Google), 2016-05-03 17:22:25]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1949603002/ by ben@chromium.org.

The reason for reverting is: Cases crashes when running

mojo_runner mojo:mash_init then selecting a user.

ED::UnobserveWindow @ the top of the call stack..

[mohsen, 2016-05-06 02:39:22]

Description was changed from

==========
mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

to

==========
Reland of mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

[mohsen, 2016-05-06 02:40:34]

Description was changed from

==========
Reland of mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

to

==========
Reland: mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

[mohsen, 2016-05-06 05:28:46]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-05-06 05:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1818333002/#ps260001
(title: "Rebased after fixing the crash")

[commit-bot: I haz the power, 2016-05-06 05:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1818333002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1818333002/260001

[commit-bot: I haz the power, 2016-05-06 05:32:28]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-05-06 05:34:12]

Description was changed from

==========
Reland: mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}
==========

to

==========
Reland: mus: Enable system modal windows

When a modal window does not have a transient parent, it will be modal
to the whole system.

BUG=548402

Committed: https://crrev.com/aad86d4a28e819ff7b1d6cef106c7d88da2d48aa
Cr-Commit-Position: refs/heads/master@{#390963}

Committed: https://crrev.com/3fe9bbb646a9408e6156012d6004a16903423363
Cr-Commit-Position: refs/heads/master@{#392007}
==========

[commit-bot: I haz the power, 2016-05-06 05:34:14]

Patchset 14 (id:??) landed as
https://crrev.com/3fe9bbb646a9408e6156012d6004a16903423363
Cr-Commit-Position: refs/heads/master@{#392007}