aura-mus: Make StackAtTop() proxy to the window manager.

aura-mus: Make StackAtTop() proxy to the window manager.

sky convinced me that we can't just let the window server handle this;
reordering assumes that the caller is also the owner of parent, so the
window manager is the one who must make the reorder call.

BUG=663617
TEST=DesktopWindowTreeHostMusTest.Stack*

Review-Url: https://codereview.chromium.org/2652713003
Cr-Commit-Position: refs/heads/master@{#445864}
Committed: https://chromium.googlesource.com/chromium/src/+/180d76b0129bc5361b80d850d3b442d8dfebf401

[Elliot Glaysher, 2017-01-23 22:22:35]

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

[commit-bot: I haz the power, 2017-01-23 22:23:27]

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-01-23 22:26:17]

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

[commit-bot: I haz the power, 2017-01-23 22:26:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Elliot Glaysher, 2017-01-23 22:38:43]

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

[commit-bot: I haz the power, 2017-01-23 22:39:44]

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-01-23 23:07:14]

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

[commit-bot: I haz the power, 2017-01-23 23:07:15]

Dry run: 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_...)

[Elliot Glaysher, 2017-01-23 23:11:06]

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

[commit-bot: I haz the power, 2017-01-23 23:12:13]

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

[Elliot Glaysher, 2017-01-24 00:47:14]

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

[commit-bot: I haz the power, 2017-01-24 00:47:20]

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

[Elliot Glaysher, 2017-01-24 00:47:30]

Description was changed from

==========
aura-mus: Make StackAtTop() proxy to the window manager.

sky convinced me that we can't just let the window server handle this;
reordering assumes that the caller is also the owner of parent, so the
window manager is the one who must make the reorder call.

BUG=663617
==========

to

==========
aura-mus: Make StackAtTop() proxy to the window manager.

sky convinced me that we can't just let the window server handle this;
reordering assumes that the caller is also the owner of parent, so the
window manager is the one who must make the reorder call.

BUG=663617
TEST=DesktopWindowTreeHostMusTest.Stack*
==========

[Elliot Glaysher, 2017-01-24 00:47:30]

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

[sky, 2017-01-24 00:54:02]

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

https://codereview.chromium.org/2652713003/diff/50001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1522: void
WindowTreeClient::WmStackAtTop(uint32_t wm_change_id, uint32_t window_id) {
How does the window manager know to match up the wm_change_id here? Generally
there is an explicit ack function for the wm_change_ids. See
OnWmMoveLoopCompleted for what I mean. But looking at the implementation here,
I'm not sure you really need the ack. Does it matter?

[Elliot Glaysher, 2017-01-24 01:15:46]

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

[commit-bot: I haz the power, 2017-01-24 01:15:57]

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

[Elliot Glaysher, 2017-01-24 01:16:06]

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

https://codereview.chromium.org/2652713003/diff/50001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1522: void
WindowTreeClient::WmStackAtTop(uint32_t wm_change_id, uint32_t window_id) {
On 2017/01/24 00:54:01, sky wrote:
> How does the window manager know to match up the wm_change_id here? Generally
> there is an explicit ack function for the wm_change_ids.

The incoming wm_change_id gets responded to in
WindowTreeClient::OnChangeCompleted(); we map the |change_id| for reordering the
window to the |wm_change_id| in |in_flight_change_map_|. When the ReorderWindow
change returns, we pass that change's return value back as wm_change_id's return
value. (And you can verify this with the unit tests, which still pass, even
though they're now waiting on the entire chain of client -> server -> window
manager -> server -> client.)

(If you're saying my error conditions below are dropping the response, I just
fixed that.)

> But looking at the implementation here,
> I'm not sure you really need the ack. Does it matter?

That's what I was asking you a few hours back. It does help those unit tests
show that this entire sequence works, but I'm pretty sure that wasn't why--I
don't remember why I was concerned about this or why I convinced you this was
correct.

(Or are you asking about why the ack needs to be forwarded back to the server?)

[commit-bot: I haz the power, 2017-01-24 02:46:00]

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

[commit-bot: I haz the power, 2017-01-24 02:46:01]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-24 17:18:15]

On 2017/01/24 01:16:06, Elliot Glaysher wrote:
>
https://codereview.chromium.org/2652713003/diff/50001/ui/aura/mus/window_tree...
> File ui/aura/mus/window_tree_client.cc (right):
> 
>
https://codereview.chromium.org/2652713003/diff/50001/ui/aura/mus/window_tree...
> ui/aura/mus/window_tree_client.cc:1522: void
> WindowTreeClient::WmStackAtTop(uint32_t wm_change_id, uint32_t window_id) {
> On 2017/01/24 00:54:01, sky wrote:
> > How does the window manager know to match up the wm_change_id here?
Generally
> > there is an explicit ack function for the wm_change_ids.
> 
> The incoming wm_change_id gets responded to in
> WindowTreeClient::OnChangeCompleted(); we map the |change_id| for reordering
the
> window to the |wm_change_id| in |in_flight_change_map_|. When the
ReorderWindow
> change returns, we pass that change's return value back as wm_change_id's
return
> value. (And you can verify this with the unit tests, which still pass, even
> though they're now waiting on the entire chain of client -> server -> window
> manager -> server -> client.)
> 
> (If you're saying my error conditions below are dropping the response, I just
> fixed that.)
> 
> > But looking at the implementation here,
> > I'm not sure you really need the ack. Does it matter?
> 
> That's what I was asking you a few hours back. It does help those unit tests
> show that this entire sequence works, but I'm pretty sure that wasn't why--I
> don't remember why I was concerned about this or why I convinced you this was
> correct.
> 
> (Or are you asking about why the ack needs to be forwarded back to the
server?)

The general flow for messages originating from a client that are forwarded to
the window manager is:
1. client supplies it's own message_id.
2. window-server identifies message as being routed to window-manager. Calls
WindowServer::GenerateWindowManagerChangeId() to generate change id specifically
for window-manager.
3. window-manager does whatever it wants, then calls back with WmResponse and
the change id it was supplied.
4. window-server matches ids and calls back to originating client with the
original message_id and response.

You have parts of this. I don't see part 4.

[sky, 2017-01-24 17:18:23]

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

https://codereview.chromium.org/2652713003/diff/70001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1523: WindowMus* window =
GetWindowByServerId(window_id);
I think you want almost the same logic here as is in OnWindowMusMoveChild. In
fact if you call StackAtTop from this implementation I think it'll generate
everything you need. The only additional call you need id the WmResponse call.

[Elliot Glaysher, 2017-01-24 18:51:00]

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

[commit-bot: I haz the power, 2017-01-24 18:51:34]

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

[Elliot Glaysher, 2017-01-24 20:11:02]

ptal

this calls Window::StackChildAtTop() in WmStackAtTop and assumes it succeeds, as
discussed in person.

[commit-bot: I haz the power, 2017-01-24 20:30:35]

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

[commit-bot: I haz the power, 2017-01-24 20:30:36]

Dry run: This issue passed the CQ dry run.

[sky, 2017-01-24 22:14:23]

LGTM

[Elliot Glaysher, 2017-01-24 22:17:37]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-01-24 22:18:08]

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

[Elliot Glaysher, 2017-01-24 22:28:23]

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

[Elliot Glaysher, 2017-01-24 22:28:24]

+tseepez for mojom review.

[Tom Sepez, 2017-01-24 22:31:15]

On 2017/01/24 22:28:24, Elliot Glaysher wrote:
> +tseepez for mojom review.

This is browser to WM or other privileged process to WM?

[commit-bot: I haz the power, 2017-01-24 22:31:31]

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

[commit-bot: I haz the power, 2017-01-24 22:31:33]

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

[Elliot Glaysher, 2017-01-24 22:32:25]

On 2017/01/24 22:31:15, Tom Sepez wrote:
> On 2017/01/24 22:28:24, Elliot Glaysher wrote:
> > +tseepez for mojom review.
> 
> This is browser to WM or other privileged process to WM?

This is window server to window manager.

[Elliot Glaysher, 2017-01-24 22:34:43]

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

[commit-bot: I haz the power, 2017-01-24 22:35:38]

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

[Tom Sepez, 2017-01-24 22:37:51]

OK, we can trust the IDs then so LGTM.

[Elliot Glaysher, 2017-01-24 22:50:09]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2017-01-24 22:50:14]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-01-24 22:50:14]

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/2652713003/#ps110001
(title: "Add check to window_manager_delegate_ in WmStackAtTop.")

[commit-bot: I haz the power, 2017-01-24 22:51:30]

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-01-24 23:53:32]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1485298214472220,
"parent_rev": "19c538ff5147d06a199e190f55c3873541e78d06", "commit_rev":
"180d76b0129bc5361b80d850d3b442d8dfebf401"}

[commit-bot: I haz the power, 2017-01-24 23:54:07]

Description was changed from

==========
aura-mus: Make StackAtTop() proxy to the window manager.

sky convinced me that we can't just let the window server handle this;
reordering assumes that the caller is also the owner of parent, so the
window manager is the one who must make the reorder call.

BUG=663617
TEST=DesktopWindowTreeHostMusTest.Stack*
==========

to

==========
aura-mus: Make StackAtTop() proxy to the window manager.

sky convinced me that we can't just let the window server handle this;
reordering assumes that the caller is also the owner of parent, so the
window manager is the one who must make the reorder call.

BUG=663617
TEST=DesktopWindowTreeHostMusTest.Stack*

Review-Url: https://codereview.chromium.org/2652713003
Cr-Commit-Position: refs/heads/master@{#445864}
Committed:
https://chromium.googlesource.com/chromium/src/+/180d76b0129bc5361b80d850d3b4...
==========

[commit-bot: I haz the power, 2017-01-24 23:54:08]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/180d76b0129bc5361b80d850d3b4...