mus: Server-side implementation of modal windows

mus: Server-side implementation of modal windows

This patch adds server-side implementation for window modal windows,
i.e. windows that are modal to their transient parent. System modal
windows will be added later.

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

[mohsen, 2016-03-02 07:19:54]

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

[mohsen, 2016-03-02 07:19:55]

Can you please take a look....

I'll add some tests soon

[sky, 2016-03-02 18:28:33]

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
components/mus/ws/event_dispatcher.cc:460: pointer_target.in_nonclient_area =
Aren't you going to use an enum to indicate modal?

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_impl.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:232: ServerWindow* window =
GetWindowByClientId(window_id);
Add to the access policy where this operation is allowed. We should route it to
the wm as necessary. Basically look at something like
WindowTreeImpl::SetWindowBounds. We want that same logic here.

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:234: Operation op(this,
connection_manager_, OperationType::SET_AS_MODAL);
As you don't send any sort of notification out you don't need to create an
operation.

[mohsen, 2016-03-03 22:54:59]

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
components/mus/ws/event_dispatcher.cc:460: pointer_target.in_nonclient_area =
On 2016/03/02 at 18:28:33, sky wrote:
> Aren't you going to use an enum to indicate modal?

Not at the moment. That enum would be useful if we send it to the client (e.g.
the window manager). On the server side we only need to know who to send the
event to for which a boolean would be enough (maybe we need to rename it to
something better). For sending this information to the client, I've added a
paragraph and a comment to the design doc. Please take a look:
https://docs.google.com/document/d/1nrDrFEvXbGhqLvH4a1uutg3uFRJsNESZAUcAyYtQ6...

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_impl.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:232: ServerWindow* window =
GetWindowByClientId(window_id);
On 2016/03/02 at 18:28:33, sky wrote:
> Add to the access policy where this operation is allowed. We should route it
to the wm as necessary. Basically look at something like
WindowTreeImpl::SetWindowBounds. We want that same logic here.

Added to the access policy. However, do we also need to route this to the window
manager like SetWindowBounds? It seems to me that SetAsModal is more like
AddTransientWidnow rather than SetWindowBounds.

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:234: Operation op(this,
connection_manager_, OperationType::SET_AS_MODAL);
On 2016/03/02 at 18:28:33, sky wrote:
> As you don't send any sort of notification out you don't need to create an
operation.

Right. Removed.

[sky, 2016-03-03 23:50:39]

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
components/mus/ws/event_dispatcher.cc:460: pointer_target.in_nonclient_area =
On 2016/03/03 22:54:59, mohsen wrote:
> On 2016/03/02 at 18:28:33, sky wrote:
> > Aren't you going to use an enum to indicate modal?
> 
> Not at the moment. That enum would be useful if we send it to the client (e.g.
> the window manager). On the server side we only need to know who to send the
> event to for which a boolean would be enough (maybe we need to rename it to
> something better). For sending this information to the client, I've added a
> paragraph and a comment to the design doc. Please take a look:
>
https://docs.google.com/document/d/1nrDrFEvXbGhqLvH4a1uutg3uFRJsNESZAUcAyYtQ6...

If you don't send a flag indicating this, I'm not sure how happy some of the
code that the windowmanager usese will be if it sees events outside the bounds
of window. Did you make sure we ignore events outside the bounds of the window
in the clients?

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_impl.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:232: ServerWindow* window =
GetWindowByClientId(window_id);
On 2016/03/03 22:54:59, mohsen wrote:
> On 2016/03/02 at 18:28:33, sky wrote:
> > Add to the access policy where this operation is allowed. We should route it
> to the wm as necessary. Basically look at something like
> WindowTreeImpl::SetWindowBounds. We want that same logic here.
> 
> Added to the access policy. However, do we also need to route this to the
window
> manager like SetWindowBounds? It seems to me that SetAsModal is more like
> AddTransientWidnow rather than SetWindowBounds.

Seems like we want clients making any random window system modal. Routing to the
windowmanager could protected against that.

[mohsen, 2016-03-04 16:24:18]

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
File components/mus/ws/event_dispatcher.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/event_dis...
components/mus/ws/event_dispatcher.cc:460: pointer_target.in_nonclient_area =
On 2016/03/03 at 23:50:39, sky wrote:
> On 2016/03/03 22:54:59, mohsen wrote:
> > On 2016/03/02 at 18:28:33, sky wrote:
> > > Aren't you going to use an enum to indicate modal?
> > 
> > Not at the moment. That enum would be useful if we send it to the client
(e.g.
> > the window manager). On the server side we only need to know who to send the
> > event to for which a boolean would be enough (maybe we need to rename it to
> > something better). For sending this information to the client, I've added a
> > paragraph and a comment to the design doc. Please take a look:
> >
https://docs.google.com/document/d/1nrDrFEvXbGhqLvH4a1uutg3uFRJsNESZAUcAyYtQ6...
> 
> If you don't send a flag indicating this, I'm not sure how happy some of the
code that the windowmanager usese will be if it sees events outside the bounds
of window. Did you make sure we ignore events outside the bounds of the window
in the clients?

I've tested this code (using window_type_launcher) and it works properly (it
actually activates the modal window which is the desired behavior). The events
on the client side end up in Aura event dispatching code which I think handles
this case properly.

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
File components/mus/ws/window_tree_impl.cc (right):

https://codereview.chromium.org/1759523002/diff/1/components/mus/ws/window_tr...
components/mus/ws/window_tree_impl.cc:232: ServerWindow* window =
GetWindowByClientId(window_id);
On 2016/03/03 at 23:50:39, sky wrote:
> On 2016/03/03 22:54:59, mohsen wrote:
> > On 2016/03/02 at 18:28:33, sky wrote:
> > > Add to the access policy where this operation is allowed. We should route
it
> > to the wm as necessary. Basically look at something like
> > WindowTreeImpl::SetWindowBounds. We want that same logic here.
> > 
> > Added to the access policy. However, do we also need to route this to the
window
> > manager like SetWindowBounds? It seems to me that SetAsModal is more like
> > AddTransientWidnow rather than SetWindowBounds.
> 
> Seems like we want clients making any random window system modal. Routing to
the windowmanager could protected against that.

I see. I plan to implement modal systems in a follow-up patch (See the TODO I've
added in window_tree.mojom)

[sky, 2016-03-04 17:00:42]

https://codereview.chromium.org/1759523002/diff/20001/components/mus/ws/conne...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1759523002/diff/20001/components/mus/ws/conne...
components/mus/ws/connection_manager.cc:678: void
ConnectionManager::OnWindowVisibilityChanged(ServerWindow* window) {
What if the window is made modal when it's already visible?

Also, you need to potentially prevent capture if a client calls setcapture and
there is a modal window.

[mohsen, 2016-03-05 01:21:53]

https://codereview.chromium.org/1759523002/diff/20001/components/mus/ws/conne...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1759523002/diff/20001/components/mus/ws/conne...
components/mus/ws/connection_manager.cc:678: void
ConnectionManager::OnWindowVisibilityChanged(ServerWindow* window) {
On 2016/03/04 17:00:42, sky wrote:
> What if the window is made modal when it's already visible?
> 
> Also, you need to potentially prevent capture if a client calls setcapture and
> there is a modal window.

Both cases added.

[sky, 2016-03-07 16:08:19]

Is there a reason you aren't including tests in this patch?

[mohsen, 2016-03-07 16:53:58]

On 2016/03/07 at 16:08:19, sky wrote:
> Is there a reason you aren't including tests in this patch?

I'm preparing tests and will ad them today...

[sky, 2016-03-07 18:45:39]

Excellent!

On Mon, Mar 7, 2016 at 8:53 AM,  <mohsen@chromium.org> wrote:
> On 2016/03/07 at 16:08:19, sky wrote:
>> Is there a reason you aren't including tests in this patch?
>
> I'm preparing tests and will ad them today...
>
> https://codereview.chromium.org/1759523002/

-- 
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-03-08 08:50:59]

Added tests and rebased the code.

Also, changed GetModalTargetForWindow() in components/mus/ws/event_dispatcher.cc
to take into account drawn state of modal windows.

Please take another look...

[sky, 2016-03-08 17:07:53]

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/conn...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/conn...
components/mus/ws/connection_manager.cc:317: }
Do you need to call ReleaseCaptureForModalWindow here too?

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/disp...
File components/mus/ws/display.h (right):

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/disp...
components/mus/ws/display.h:124: void ReleaseCaptureForModalWindow(ServerWindow*
window);
It isn't particularly clear what this does from the name. Please add a
description.

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

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:55: ServerWindow*
GetClosestModalTargetForWindow(ServerWindow* target) {
Add description as closest isn't clear in this context. I think ancestor is
better, but I'm ok with closest and a description.

[mohsen, 2016-03-09 08:26:48]

As mentioned in inline comments, I moved some code around to be able to release
capture after WindowHierarchyChanged. Also, rebased the code after the patch
that moved EventDispatcher out of Display. Please take a look...

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/conn...
File components/mus/ws/connection_manager.cc (right):

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/conn...
components/mus/ws/connection_manager.cc:317: }
On 2016/03/08 at 17:07:53, sky wrote:
> Do you need to call ReleaseCaptureForModalWindow here too?

Yeah, we should release capture if necessary in this case too. However, we can't
use ReleaseCaptureForModalWindow as it needs the modal window suspected to block
the capture. I've moved around codes a bit to be able to do this.

Also, added this code to OnWindowHierarchyChanged instead of here, because
windows are passed as const here which makes things a bit complicated.

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/disp...
File components/mus/ws/display.h (right):

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/disp...
components/mus/ws/display.h:124: void ReleaseCaptureForModalWindow(ServerWindow*
window);
On 2016/03/08 at 17:07:53, sky wrote:
> It isn't particularly clear what this does from the name. Please add a
description.

Done. Also, renamed the function as I needed to add another function with
similar functionality.

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

https://codereview.chromium.org/1759523002/diff/100001/components/mus/ws/even...
components/mus/ws/event_dispatcher.cc:55: ServerWindow*
GetClosestModalTargetForWindow(ServerWindow* target) {
On 2016/03/08 at 17:07:53, sky wrote:
> Add description as closest isn't clear in this context. I think ancestor is
better, but I'm ok with closest and a description.

Renamed to GetModalChildForWindowAncestor() (also changed to return nullptr when
nothing is found to match the new name better).

I moved these functions to server_window.cc to make the functionality accessible
to other codes.

[sky, 2016-03-09 16:33:56]

LGTM with the following

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

https://codereview.chromium.org/1759523002/diff/180001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:187: SetAsModal(uint32
change_id, uint32 window_id);
SetModal here too.

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
File components/mus/ws/server_window.cc (right):

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.cc:41: }
nit: // namespace

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.h:112: void SetAsModal();
SetModal.

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.h:115: const ServerWindow* GetModalTarget()
const;
Add description of GetModalTarget.

[mohsen, 2016-03-09 18:32:52]

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

https://codereview.chromium.org/1759523002/diff/180001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:187: SetAsModal(uint32
change_id, uint32 window_id);
On 2016/03/09 at 16:33:56, sky wrote:
> SetModal here too.

Done.

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
File components/mus/ws/server_window.cc (right):

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.cc:41: }
On 2016/03/09 at 16:33:56, sky wrote:
> nit: // namespace

Done.

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
File components/mus/ws/server_window.h (right):

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.h:112: void SetAsModal();
On 2016/03/09 at 16:33:56, sky wrote:
> SetModal.

Done. Also changed AccessPolicy::CanSetAsModal to CanSetModal.

https://codereview.chromium.org/1759523002/diff/180001/components/mus/ws/serv...
components/mus/ws/server_window.h:115: const ServerWindow* GetModalTarget()
const;
On 2016/03/09 at 16:33:56, sky wrote:
> Add description of GetModalTarget.

Done.

[mohsen, 2016-03-09 19:29:19]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-03-09 19:29:19]

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/1759523002/#ps200001
(title: "SetAsModal -> SetModal + Other review comments addressed")

[commit-bot: I haz the power, 2016-03-09 19:29:48]

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

[commit-bot: I haz the power, 2016-03-09 19:36:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-03-09 19:41:15]

Description was changed from

==========
mus: Server-side implementation of modal windows

This patch adds server-side implementation for window modal windows,
i.e. windows that are modal to their transient parent. System modal
windows will be added later.

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

to

==========
mus: Server-side implementation of modal windows

This patch adds server-side implementation for window modal windows,
i.e. windows that are modal to their transient parent. System modal
windows will be added later.

BUG=548402

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

[commit-bot: I haz the power, 2016-03-09 19:41:16]

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