Simplify WindowManager::OnWmSetBounds

Simplify WindowManager::OnWmSetBounds

If a top level window requests a change in bounds, that request will get plumbed
into the window manager. At that point, the window manager can veto the bounds
change.

This change simplifies the control flow about by having the window manager
always responding "no" (false) to the client while also setting the window
bounds itself based on the request.

By giving a response of false to the client, it will revert the currently
pending InFlightBoundsChange. If the bounds change initiated by the window
manager arrives first to the client, then the inflight change's revert will
be updated, and when the negative response arrives, it's a noop.

BUG=672151
Review-Url: https://codereview.chromium.org/2778823002
Cr-Commit-Position: refs/heads/master@{#459928}
Committed: https://chromium.googlesource.com/chromium/src/+/69875b5a5f926383fe0e2d476bf6c42c30b8b741

[Fady Samuel, 2017-03-27 16:44:57]

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

[Fady Samuel, 2017-03-27 16:45:33]

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

[Fady Samuel, 2017-03-27 16:45:33]

Hi Scott,

Is this along the lines of what you were expecting. This seems to
fix the issue I was encountering with surface synchronization too. :-)

Thanks for your help! :-)

Fady

[commit-bot: I haz the power, 2017-03-27 16:45:43]

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

[Fady Samuel, 2017-03-27 16:54:58]

Description was changed from

==========
Simplify WindowManager::OnWmSetBounds

If a top level window requests a change in bounds, that request will get plumbed
into the window manager. At that point, the window manager can veto the bounds
change.

This change simplifies the control flow about by having the window manager
always responsing "no" (false) to the client while also setting the window
bounds itself based on the request.

By giving a response of false to the client, it will revert the currently
pending InFlightBoundsChange. If the bounds change initiated by the window
manager arrives first to the client, then the inflight change's revert will
be updated, and when the negative response arrives, it's a noop.

BUG=672151
==========

to

==========
Simplify WindowManager::OnWmSetBounds

If a top level window requests a change in bounds, that request will get plumbed
into the window manager. At that point, the window manager can veto the bounds
change.

This change simplifies the control flow about by having the window manager
always responding "no" (false) to the client while also setting the window
bounds itself based on the request.

By giving a response of false to the client, it will revert the currently
pending InFlightBoundsChange. If the bounds change initiated by the window
manager arrives first to the client, then the inflight change's revert will
be updated, and when the negative response arrives, it's a noop.

BUG=672151
==========

[Fady Samuel, 2017-03-27 17:01:44]

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

[commit-bot: I haz the power, 2017-03-27 17:02:55]

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

[sadrul, 2017-03-27 17:06:16]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2017-03-27 17:06:17]

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1417:
window_manager_internal_client_->WmResponse(change_id, result);
Maybe the WM does not need to ack the bounds change at all?

[Fady Samuel, 2017-03-27 17:11:42]

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1417:
window_manager_internal_client_->WmResponse(change_id, result);
On 2017/03/27 17:06:16, sadrul wrote:
> Maybe the WM does not need to ack the bounds change at all?

Right now the window server, and client generate in flight change IDs for this
request. Maybe we don't need these inflight changes?

[sky, 2017-03-27 17:14:57]

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1417:
window_manager_internal_client_->WmResponse(change_id, result);
On 2017/03/27 17:11:42, Fady Samuel wrote:
> On 2017/03/27 17:06:16, sadrul wrote:
> > Maybe the WM does not need to ack the bounds change at all?
> 
> Right now the window server, and client generate in flight change IDs for this
> request. Maybe we don't need these inflight changes?

Things will get out of order if you do that. I agree with Sadrul. For clarity
please add a WmSetBoundsResponse(change_id) that takes no bool, only the change
id. Mus will always respond with false to the SetBounds request and the client
will do the right thing.

[Fady Samuel, 2017-03-27 17:16:38]

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

[commit-bot: I haz the power, 2017-03-27 17:17:09]

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

[Fady Samuel, 2017-03-27 18:15:36]

PTAL Scott!

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2778823002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1417:
window_manager_internal_client_->WmResponse(change_id, result);
On 2017/03/27 17:14:56, sky wrote:
> On 2017/03/27 17:11:42, Fady Samuel wrote:
> > On 2017/03/27 17:06:16, sadrul wrote:
> > > Maybe the WM does not need to ack the bounds change at all?
> > 
> > Right now the window server, and client generate in flight change IDs for
this
> > request. Maybe we don't need these inflight changes?
> 
> Things will get out of order if you do that. I agree with Sadrul. For clarity
> please add a WmSetBoundsResponse(change_id) that takes no bool, only the
change
> id. Mus will always respond with false to the SetBounds request and the client
> will do the right thing.

Done.

[Fady Samuel, 2017-03-27 18:15:42]

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

[commit-bot: I haz the power, 2017-03-27 18:16:18]

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

[Fady Samuel, 2017-03-27 18:16:37]

Will do a bit more cleanup.

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

https://codereview.chromium.org/2778823002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:1411: &bounds_in_dip);
Actually, this doesn't seem to need to be an out param anymore. Will change.

[Fady Samuel, 2017-03-27 18:26:04]

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

[commit-bot: I haz the power, 2017-03-27 18:26:20]

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

[Fady Samuel, 2017-03-27 18:26:32]

Passing bounds as const ref to OnWmSetBounds now. PTAL!

[commit-bot: I haz the power, 2017-03-27 19:22:30]

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

[commit-bot: I haz the power, 2017-03-27 19:22:31]

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

[sky, 2017-03-27 19:22:51]

LGTM

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

https://codereview.chromium.org/2778823002/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2074: WmResponse(change_id, false);
Document why false is used.

[Fady Samuel, 2017-03-27 20:31:11]

CQ'ing

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

https://codereview.chromium.org/2778823002/diff/80001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:2074: WmResponse(change_id, false);
On 2017/03/27 19:22:51, sky wrote:
> Document why false is used.

Done.

[Fady Samuel, 2017-03-27 20:31:27]

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

[Fady Samuel, 2017-03-27 20:31:28]

+tsepez@ for mojom

[Fady Samuel, 2017-03-27 20:31:33]

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

[commit-bot: I haz the power, 2017-03-27 20:32:00]

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-03-27 20:34:27]

lgtm

[commit-bot: I haz the power, 2017-03-27 21:29:42]

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

[commit-bot: I haz the power, 2017-03-27 21:29:44]

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

[Fady Samuel, 2017-03-27 21:30:31]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-03-27 21:30:32]

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/2778823002/#ps100001
(title: "Add comment in WmSetBoundsResponse")

[commit-bot: I haz the power, 2017-03-27 21:31:50]

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

[Fady Samuel, 2017-03-27 22:14:26]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-03-27 22:14:26]

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/2778823002/#ps50015
(title: "Update expectations for DragTestInteractive.DragTest")

[commit-bot: I haz the power, 2017-03-27 22:15:15]

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-27 23:15:24]

CQ is committing da patch.

Bot data: {"patchset_id": 50015, "attempt_start_ts": 1490652866320900,
"parent_rev": "ec764054b12a8e21e1fe958c3f1e4d47df671066", "commit_rev":
"69875b5a5f926383fe0e2d476bf6c42c30b8b741"}

[commit-bot: I haz the power, 2017-03-27 23:17:08]

Description was changed from

==========
Simplify WindowManager::OnWmSetBounds

If a top level window requests a change in bounds, that request will get plumbed
into the window manager. At that point, the window manager can veto the bounds
change.

This change simplifies the control flow about by having the window manager
always responding "no" (false) to the client while also setting the window
bounds itself based on the request.

By giving a response of false to the client, it will revert the currently
pending InFlightBoundsChange. If the bounds change initiated by the window
manager arrives first to the client, then the inflight change's revert will
be updated, and when the negative response arrives, it's a noop.

BUG=672151
==========

to

==========
Simplify WindowManager::OnWmSetBounds

If a top level window requests a change in bounds, that request will get plumbed
into the window manager. At that point, the window manager can veto the bounds
change.

This change simplifies the control flow about by having the window manager
always responding "no" (false) to the client while also setting the window
bounds itself based on the request.

By giving a response of false to the client, it will revert the currently
pending InFlightBoundsChange. If the bounds change initiated by the window
manager arrives first to the client, then the inflight change's revert will
be updated, and when the negative response arrives, it's a noop.

BUG=672151

Review-Url: https://codereview.chromium.org/2778823002
Cr-Commit-Position: refs/heads/master@{#459928}
Committed:
https://chromium.googlesource.com/chromium/src/+/69875b5a5f926383fe0e2d476bf6...
==========

[commit-bot: I haz the power, 2017-03-27 23:17:09]

Committed patchset #7 (id:50015) as
https://chromium.googlesource.com/chromium/src/+/69875b5a5f926383fe0e2d476bf6...