Tab dragging as implemented as a mus API.

mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900
Committed: https://crrev.com/6d0134e5306fcdc9b27865b4171e35ee929692ce
Cr-Commit-Position: refs/heads/master@{#404021}

[Elliot Glaysher, 2016-06-22 21:21:49]

Description was changed from

==========
[xxx]: Draft of tab dragging as implemented as a mus API.

BUG=
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking
window move. The client asks the window server to perform
a window move, which forwards this request to the window
manager. We want to do this instead of having the client
perform all moves itself because the window manager performs
special actions, like docking a window when dragged to the
side.

BUG=599549
==========

[Elliot Glaysher, 2016-06-22 21:21:49]

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

[Elliot Glaysher, 2016-06-22 22:13:05]

Description was changed from

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking
window move. The client asks the window server to perform
a window move, which forwards this request to the window
manager. We want to do this instead of having the client
perform all moves itself because the window manager performs
special actions, like docking a window when dragged to the
side.

BUG=599549
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking
window move. The client asks the window server to perform
a window move, which forwards this request to the window
manager. We want to do this instead of having the client
perform all moves itself because the window manager performs
special actions, like docking a window when dragged to the
side.

This then uses that API to implement tab dragging in mus+ash.

BUG=599549
==========

[Elliot Glaysher, 2016-06-22 22:14:12]

Description was changed from

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking
window move. The client asks the window server to perform
a window move, which forwards this request to the window
manager. We want to do this instead of having the client
perform all moves itself because the window manager performs
special actions, like docking a window when dragged to the
side.

This then uses that API to implement tab dragging in mus+ash.

BUG=599549
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

This then uses that API to implement tab dragging in mus+ash. Tabs can
now be dragged inside tab strips. They can be dragged out of tabstrips
into new windows. They can be dragged back into preexisting
windows. Finally, dragging to the side activates the correct window
manager actions.

BUG=599549
==========

[sky, 2016-06-22 23:47:57]

It would be nice if you could break this up. Perhaps the client lib and server
changes (with tests) in one patch, ash side in another and chrome in the last
(I'm ok with combining the last two into a single patch as well).

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
File components/mus/public/cpp/lib/in_flight_change.cc (right):

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/cpp/lib/in_flight_change.cc:55:
WindowPrivate(window()).LocalSetBounds(window()->bounds(), revert_bounds_);
This is problematic because it may conflict with any InFlightBoundsChanges. For
example, consider the following scenario:
. start move loop
. Window::SetBounds (in client)
. move loop canceled

In this case you don't want to change the bounds at all.

I think you need to use an InFlightBoundsChange so revert values are properly
merged.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
File components/mus/public/cpp/lib/in_flight_change.h (right):

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/cpp/lib/in_flight_change.h:33: MOVE_LOOP,
nit: keep sorted.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:632:
base::MessageLoop::ScopedNestableTaskAllower allow_nested(loop);
mus::Window shouldn't run a nested message loop. Clients that want a nested
message loop (NativeWidgetMus) can run one if they want to.

Also, this code should only allow one move loop at a time.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
File components/mus/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_manager.mojom:89:
WmPerformMoveLoop(uint32 change_id, uint32 window_id,
nit: window_id on its own line.
Also, document what cursor_location is as it isn't obvious from reading the
description.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_manager.mojom:146:
OnWmMoveLoopCompleted(uint32 change_id, uint32 window_id, bool completed);
Why do you need to supply the window_id here as well? mus should know the window
based on the change_id.

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

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:235: // when a client sends
this message and when the window manager starts acting
There is a delay, but isn't this sent before the client has ack'd the current
event so that mus should know the cursor position it sent to the client and can
pass that to the windowmanager?

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:372:
OnMoveLoopCompleted(uint32 change_id, bool success);
Can't you use OnChangeCompleted() for this?

https://codereview.chromium.org/2060513002/diff/130001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/2060513002/diff/130001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1417: // TODO(erg): Should we also ensure that
no second client can try to perform
Yes, as mash won't support it.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1422: if (!success) {
You should also fail if ShouldRouteToWindowManager returns false.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1431: .window_manager_state;
Sorry, I changed this around. You'll want:

    WindowManagerDisplayRoot* display_root =
        GetWindowManagerDisplayRoot(window);
    WindowTree* wm_tree = display_root->window_manager_state();

https://codereview.chromium.org/2060513002/diff/130001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1438: wms->SetCapture(window, true);
true should be the id() of the WindowTree from WindowManagerState.

[Elliot Glaysher, 2016-06-23 22:41:12]

Description was changed from

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

This then uses that API to implement tab dragging in mus+ash. Tabs can
now be dragged inside tab strips. They can be dragged out of tabstrips
into new windows. They can be dragged back into preexisting
windows. Finally, dragging to the side activates the correct window
manager actions.

BUG=599549
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900
==========

[Elliot Glaysher, 2016-06-24 17:41:55]

This splits the change in two; the ash+views change is here:
https://codereview.chromium.org/2099513003

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

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:235: // when a client sends
this message and when the window manager starts acting
On 2016/06/22 23:47:56, sky wrote:
> There is a delay, but isn't this sent before the client has ack'd the current
> event so that mus should know the cursor position it sent to the client and
can
> pass that to the windowmanager?

Experimentally, the delay is actually pretty big and can lead to the cursor
outside the tab. (I don't entirely understand why they aren't equivalent
though.)

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:372:
OnMoveLoopCompleted(uint32 change_id, bool success);
On 2016/06/22 23:47:57, sky wrote:
> Can't you use OnChangeCompleted() for this?

OnMoveLoopCompleted() does additional things like calling the closure (which
previously was directly from a run loop, and now, is indirectly from a run loop
passed in from mus.)

[sky, 2016-06-24 19:27:01]

I'll look at your other path now.

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

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:235: // when a client sends
this message and when the window manager starts acting
On 2016/06/24 17:41:55, Elliot Glaysher wrote:
> On 2016/06/22 23:47:56, sky wrote:
> > There is a delay, but isn't this sent before the client has ack'd the
current
> > event so that mus should know the cursor position it sent to the client and
> can
> > pass that to the windowmanager?
> 
> Experimentally, the delay is actually pretty big and can lead to the cursor
> outside the tab. (I don't entirely understand why they aren't equivalent
> though.)

It's worth understanding why this happens.

https://codereview.chromium.org/2060513002/diff/130001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:372:
OnMoveLoopCompleted(uint32 change_id, bool success);
On 2016/06/24 17:41:55, Elliot Glaysher wrote:
> On 2016/06/22 23:47:57, sky wrote:
> > Can't you use OnChangeCompleted() for this?
> 
> OnMoveLoopCompleted() does additional things like calling the closure (which
> previously was directly from a run loop, and now, is indirectly from a run
loop
> passed in from mus.)

Isn't that an implementation detail though? By that I mean you could still call
OnChangeCompleted and WindowTreeClient can know the change corresponds to a move
as it supplied the change_id.

[sky, 2016-06-24 19:59:01]

https://codereview.chromium.org/2060513002/diff/170001/chrome/browser/ui/view...
File chrome/browser/ui/views/tabs/window_finder_mus.h (right):

https://codereview.chromium.org/2060513002/diff/170001/chrome/browser/ui/view...
chrome/browser/ui/views/tabs/window_finder_mus.h:8: #include
"chrome/browser/ui/views/tabs/window_finder.h"
Should these files be moved to your other patch?

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:633: callback.Run(false);
Why are you calling callback.Run(false) here? I recommend either making 631 a
DCHECK, or if you really want to handle starting another move loop then we you
need a cancel api.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:1154:
window_manager_delegate_->OnWmPerformMoveLoop(change_id, window_id,
This and OnWmCancelMoveLoop() should take the window, not a window_id. Also, the
delegate shouldn't need the change_id, it should be cached here and then
implicitly used in OnWmMoveLoopCompleted (or if you prefer supply a callback to
OnWmPerformMoveLoop that binds the change_id.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
File components/mus/public/cpp/window.h (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
components/mus/public/cpp/window.h:224: // Tells the window manager to take
control of moving the window. Locally,
Update comment.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
File components/mus/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
components/mus/public/cpp/window_tree_client.h:185: void
PerformWindowMove(Window* window, const gfx::Point& cursor_location,
nit: run git cl format

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

https://codereview.chromium.org/2060513002/diff/170001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:372:
OnMoveLoopCompleted(uint32 change_id, bool success);
Seem my comment in other patch, I think you should use OnChangeCompleted and the
client can figure out if it corresponds to a move.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
File components/mus/ws/window_manager_state.h (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_manager_state.h:60: void StartMoveLoop(uint32_t
change_id, WindowId window_id,
In general only the functions from clients use a WindowId, the rest of the code
passes around ServerWindow. For example, SetCapture on line 53 takes the
ServerWindow, not the WindowId.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1428: WindowManagerState* wms =
display_root->window_manager_state();
Make sure you deal with a null display_root. This would happen if the window
isn't parented to a drawn root.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1446: wms->StartMoveLoop(wm_change_id,
window->id(), window->bounds());
I tend think this code should call WMS::StartMoveLoop() and it should handle all
the internals, e.g. move SetCapture, generating changing and call to
WmPerformMoveLoop there.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1455: if (!success)
This should also fail if this client didn't initiate the current move.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1460: WindowManagerState* wms =
display_root->window_manager_state();
Similar comment about display_root.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1580: if (window_manager_state_) {
nit: early return.

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1594:
window_manager_state_->SetCapture(nullptr, false);
Similar comment about having WMS handle all the details of canceling the drag.

[Elliot Glaysher, 2016-06-28 22:32:37]

This patch moves the tracking code from WindowManagerState to WindowServer. It
also threads through whether this move loop was created by touch or mouse. (and
I've updated the other patch to use that state.)

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
File components/mus/ws/window_tree.cc (right):

https://codereview.chromium.org/2060513002/diff/170001/components/mus/ws/wind...
components/mus/ws/window_tree.cc:1446: wms->StartMoveLoop(wm_change_id,
window->id(), window->bounds());
On 2016/06/24 19:59:01, sky wrote:
> I tend think this code should call WMS::StartMoveLoop() and it should handle
all
> the internals, e.g. move SetCapture, generating changing and call to
> WmPerformMoveLoop there.

I went through trying to move it, but it would require access both to
window_manager_internal_ and to ClientWindowIdForWindow().

[sky, 2016-06-29 00:04:09]

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
File components/mus/public/cpp/lib/in_flight_change.h (right):

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/cpp/lib/in_flight_change.h:149:
InFlightMoveLoopChange(Window* window);
explicit

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:1053:
current_move_loop_change_ = 0;
Please move current_move_loop_change_ = 0 before the Run(), that way we don't
think we're in a move if the caller calls back.

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:1157: if
(window_manager_delegate_) {
If there is no window for id, immediately call OnWmMoveLoopCompleted.

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:1166: if
(window_manager_delegate_)
As WmCancelMoveLoop is async, it's entirely possible that
OnWmMoveLoopCompleted() has already been called. This code should protect
against that, otherwise the delegate is going to be in a very confused state.

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
File components/mus/public/cpp/window_tree_client.h (right):

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/cpp/window_tree_client.h:184: // Performs a window move.
This call blocks but spins up a nested message
Update comment (no longer blocks).

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
File components/mus/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/interfaces/window_manager.mojom:147:
OnWmMoveLoopCompleted(uint32 change_id, bool completed);
Can't the client call WmResponse() ?

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

https://codereview.chromium.org/2060513002/diff/230001/components/mus/public/...
components/mus/public/interfaces/window_tree.mojom:246: CancelWindowMove(uint32
window_id);
Can this take change_id, which is meant to uniquely identify the move rather
than window_id?

https://codereview.chromium.org/2060513002/diff/230001/components/mus/ws/wind...
File components/mus/ws/window_server.h (right):

https://codereview.chromium.org/2060513002/diff/230001/components/mus/ws/wind...
components/mus/ws/window_server.h:211: bool in_move_loop() { return
!!current_move_loop_.get(); }
nit: const

[Elliot Glaysher, 2016-06-29 19:57:17]

Move to using WmResponse() and further removed window_ids from
WindowTreeClient's interface.

(I resolved those change_ids to ::mus::Window* objects for the delegate layer
though, based on comments in a previous patchset about not passing ids to that
layers.)

[sky, 2016-06-29 21:47:19]

Nice, LGTM

[Elliot Glaysher, 2016-06-29 21:48:27]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-06-29 21:51:26]

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

[Elliot Glaysher, 2016-06-29 22:01:29]

erg@chromium.org changed reviewers:
+ dcheng@chromium.org

[Elliot Glaysher, 2016-06-29 22:01:30]

+dcheng for review of mojom files.

[commit-bot: I haz the power, 2016-06-29 22:16:31]

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

[commit-bot: I haz the power, 2016-06-29 22:16:38]

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

[dcheng, 2016-06-30 13:16:50]

Sorry for the delay. I'll finish this reviewing this in Tokyo AM. My main
comment for now is: can we add comments to describe where WindowManager and
WindowTree live? Is there an easy way to tell what process a mus service lives
in?

[sky, 2016-06-30 16:42:55]

WindowTree lives in mus. WindowManager may be provided by clients
connecting to mus (in general ash/mus is the windowmanager, but there
are also examples that don't use ash/mus). This is done using
https://cs.chromium.org/chromium/src/components/mus/public/interfaces/window_...
.

  -Scott

On Thu, Jun 30, 2016 at 6:16 AM,  <dcheng@chromium.org> wrote:
> Sorry for the delay. I'll finish this reviewing this in Tokyo AM. My main
> comment for now is: can we add comments to describe where WindowManager and
> WindowTree live? Is there an easy way to tell what process a mus service
> lives
> in?
>
> https://codereview.chromium.org/2060513002/

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

[dcheng, 2016-07-01 01:35:33]

On 2016/06/30 16:42:55, sky wrote:
> WindowTree lives in mus. WindowManager may be provided by clients
> connecting to mus (in general ash/mus is the windowmanager, but there
> are also examples that don't use ash/mus). This is done using
>
https://cs.chromium.org/chromium/src/components/mus/public/interfaces/window_...
> .
> 
>   -Scott

Thanks for the explanation. I think I've asked about this before, but sadly I
can't find it: is there any general diagrams of the process architecture of
mus+ash and how it integrates with Chrome? What does it mean for a client to
connect to mus? Is the browser process a client of mus? What about the
renderers? I think that's really the biggest missing piece for me in terms of
doing the security review. Sorry if these are dumb questions.

> 
> On Thu, Jun 30, 2016 at 6:16 AM,  <mailto:dcheng@chromium.org> wrote:
> > Sorry for the delay. I'll finish this reviewing this in Tokyo AM. My main
> > comment for now is: can we add comments to describe where WindowManager and
> > WindowTree live? Is there an easy way to tell what process a mus service
> > lives
> > in?
> >
> > https://codereview.chromium.org/2060513002/
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2016-07-01 16:40:25]

On Thu, Jun 30, 2016 at 6:35 PM,  <dcheng@chromium.org> wrote:
> On 2016/06/30 16:42:55, sky wrote:
>> WindowTree lives in mus. WindowManager may be provided by clients
>> connecting to mus (in general ash/mus is the windowmanager, but there
>> are also examples that don't use ash/mus). This is done using
>>
>
https://cs.chromium.org/chromium/src/components/mus/public/interfaces/window_...
>> .
>>
>> -Scott
>
> Thanks for the explanation. I think I've asked about this before, but sadly
> I
> can't find it: is there any general diagrams of the process architecture of
> mus+ash and how it integrates with Chrome?

Sadly no. I plan on creating said document next week.

> What does it mean for a client to
> connect to mus?

What I mean by that is when an app calls
ConnectToInterface("mojo:mus"). Mus has a client library that most
places use though, so this is a bit hidden from them.
https://chromium.googlesource.com/chromium/src/+/master/components/mus/public...
is the place that establishes the connection.

> Is the browser process a client of mus?

Yes.

> What about the renderers?

Mus supports an embedding api. The embedding api allows one client
(WindowTreeClient) that has a connection to mus (WindowTree) to embed
another client (WIndowTreeClient) in a window. When this happens the
embedded client handles most rendering and events of the window. See
https://chromium.googlesource.com/chromium/src/+/master/components/mus/public...
. In this mode the embedded client doesn't directly connect to mus,
rather the client that called Embed() supplies the WindowTreeClient.
The supplied WindowTreeClient ends up with a connection to mus by way
of WindowTree passing itself to the new client (see
https://chromium.googlesource.com/chromium/src/+/master/components/mus/public...
).

RenderWidgetHostViewMus requests a WindowTreeClient from the renderer
and calls Embed (see
https://chromium.googlesource.com/chromium/src/+/master/content/browser/rende...
). So, renderers don't directly connect to mus, but they end up with a
connection to mus and a client because of how we've wired things up.

Does that make sense?

> I think that's really the biggest missing piece for me in terms
> of
> doing the security review. Sorry if these are dumb questions.

There is no dumb question. I apologize for the lack of documentation
here. I will write some next week and send it your way to make sure it
makes sense.

  -Scott

>
>>
>> On Thu, Jun 30, 2016 at 6:16 AM, <mailto:dcheng@chromium.org> wrote:
>> > Sorry for the delay. I'll finish this reviewing this in Tokyo AM. My
>> > main
>> > comment for now is: can we add comments to describe where WindowManager
>> > and
>> > WindowTree live? Is there an easy way to tell what process a mus service
>> > lives
>> > in?
>> >
>> > https://codereview.chromium.org/2060513002/
>>
>> --
>> 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
>
>
> https://codereview.chromium.org/2060513002/

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

[dcheng, 2016-07-06 09:27:53]

At a high-level, I think this is probably OK. The change is a bit inscrutable to
me still, even after reading a bunch of code. I think several things will help:
- The high-level doc that sky@ will work on. There's a lot of moving parts here,
and I feel kind of clueless as someone who's not so familiar with the code /
terminology.
- A few more comments in the code to explain terminology. For example, what is a
move loop?  I guess this is just for handling the window dragging case, but I'm
honestly not too sure.

In any case... mojo LGTM.

https://codereview.chromium.org/2060513002/diff/250001/components/mus/public/...
File components/mus/public/cpp/lib/window_tree_client.cc (right):

https://codereview.chromium.org/2060513002/diff/250001/components/mus/public/...
components/mus/public/cpp/lib/window_tree_client.cc:647: base::WrapUnique(new
InFlightMoveLoopChange(window)));
Nit: I'd suggest writing base::MakeUnique<InFlightMoveLoopChange>(window)

https://codereview.chromium.org/2060513002/diff/250001/components/mus/ws/wind...
File components/mus/ws/window_server.cc (right):

https://codereview.chromium.org/2060513002/diff/250001/components/mus/ws/wind...
components/mus/ws/window_server.cc:438: if (current_move_loop_)
I'm not 100% sure about this, but it feels like these should just
DCHECK(current_move_loop_): it seems like these probably shouldn't be called if
we're not in a move loop anyway.

https://codereview.chromium.org/2060513002/diff/250001/components/mus/ws/wind...
File components/mus/ws/window_server.h (right):

https://codereview.chromium.org/2060513002/diff/250001/components/mus/ws/wind...
components/mus/ws/window_server.h:208: bool in_move_loop() const { return
!!current_move_loop_.get(); }
Nit: the .get() here can be omitted

[Elliot Glaysher, 2016-07-06 19:28:25]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-07-06 19:28:25]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2060513002/#ps330001
(title: "dcheng nits")

[commit-bot: I haz the power, 2016-07-06 19:29:14]

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, 2016-07-06 21:44:25]

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

[commit-bot: I haz the power, 2016-07-06 21:44:27]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Elliot Glaysher, 2016-07-06 21:46:30]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2016-07-06 21:47: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, 2016-07-07 00:41:27]

Description was changed from

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900
==========

[commit-bot: I haz the power, 2016-07-07 00:41:29]

Committed patchset #18 (id:330001)

[commit-bot: I haz the power, 2016-07-07 00:41:48]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-07 00:44:30]

Description was changed from

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900
==========

to

==========
mus: Add an API for client initiated window moves.

This adds a set of APIs so the client can perform a blocking window
move. The client asks the window server to perform a window move, which
forwards this request to the window manager. We want to do this instead
of having the client perform all moves itself because the window manager
performs special actions, like docking a window when dragged to the
side.

BUG=622900

Committed: https://crrev.com/6d0134e5306fcdc9b27865b4171e35ee929692ce
Cr-Commit-Position: refs/heads/master@{#404021}
==========

[commit-bot: I haz the power, 2016-07-07 00:44:33]

Patchset 18 (id:??) landed as
https://crrev.com/6d0134e5306fcdc9b27865b4171e35ee929692ce
Cr-Commit-Position: refs/heads/master@{#404021}