Update bool WindowManager::OnWmSetBounds() to match with its desirable behavior.

Update bool WindowManager::OnWmSetBounds() to match with its desirable behavior.

Function WindowManagerDelegate::OnWmSetBounds should have no return value, take a const gfx::Rect& bounds and let the client set whatever bounds it wants on the window. WindowTreeClient::WmSetBounds would call WmResponse with false always. 

Doing this means the WindowManagerDelegate can set whatever bounds it wants, and by supplying false to WmResponse we ensure the client fallsback to the most recent value from the server, which is either the original bounds (if the delegate didn't change anything) or the bounds the delegate changed the window too (which may be the supplied bounds).

BUG=672151

[thanhph, 2016-12-01 16:09:39]

Description was changed from

==========
Make sure window bound is set in sync with window_tree_client bounds.

BUG = 668277
==========

to

==========
App launcher crashes if display resolution is too small

BUG = 668277
==========

[thanhph, 2016-12-01 16:10:57]

Description was changed from

==========
App launcher crashes if display resolution is too small

BUG = 668277
==========

to

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds are in sync with window_tree_host_->GetBounds()

BUG = 668277
==========

[thanhph, 2016-12-01 16:11:10]

Description was changed from

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds are in sync with window_tree_host_->GetBounds()

BUG = 668277
==========

to

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds are in sync with window_tree_host_->GetBounds().

BUG = 668277
==========

[thanhph, 2016-12-01 16:12:41]

Description was changed from

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds are in sync with window_tree_host_->GetBounds().

BUG = 668277
==========

to

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

BUG = 668277
==========

[thanhph, 2016-12-01 21:16:11]

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

[commit-bot: I haz the power, 2016-12-01 21:16:38]

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, 2016-12-01 21:39:33]

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

[commit-bot: I haz the power, 2016-12-01 21:39:34]

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

[thanhph, 2016-12-01 21:45:59]

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

[commit-bot: I haz the power, 2016-12-01 21:46:22]

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, 2016-12-01 22:07:09]

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

[commit-bot: I haz the power, 2016-12-01 22:07:10]

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

[thanhph, 2016-12-01 23:39:15]

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

[commit-bot: I haz the power, 2016-12-01 23:39:40]

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, 2016-12-01 23:56:48]

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

[commit-bot: I haz the power, 2016-12-01 23:56:49]

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

[thanhph, 2016-12-02 00:31:48]

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

[commit-bot: I haz the power, 2016-12-02 00:33:08]

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, 2016-12-02 00:55:48]

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

[commit-bot: I haz the power, 2016-12-02 00:55:50]

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

[thanhph, 2016-12-02 14:47:32]

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

[commit-bot: I haz the power, 2016-12-02 14:47:42]

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, 2016-12-02 15:58:44]

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

[commit-bot: I haz the power, 2016-12-02 15:58:45]

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

[Fady Samuel, 2016-12-02 23:18:24]

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

[Fady Samuel, 2016-12-02 23:18:25]

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/tests/window_server_test_base.cc (right):

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/tests/window_server_test_base.cc:121: void
WindowServerTestBase::OnWmSetBounds(Window* window, gfx::Rect* bounds) {}
This is changing behavior.

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1319:
window_manager_delegate_->OnWmSetBounds(window, &transit_bounds_in_dip);
Add window->SetBounds(transit_bounds_in_dip)?

[thanhph, 2016-12-02 23:46:40]

Description was changed from

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

BUG = 668277
==========

to

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

BUG=668277
==========

[thanhph, 2016-12-05 17:54:00]

Hi Fady, still failing on WindowServerTest.SetBoundsSecurity. I'll debug it more
in a bit. Thanks!

Thanh.

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/tests/window_server_test_base.cc (right):

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/tests/window_server_test_base.cc:121: void
WindowServerTestBase::OnWmSetBounds(Window* window, gfx::Rect* bounds) {}
On 2016/12/02 23:18:25, Fady Samuel wrote:
> This is changing behavior.

Acknowledged.

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/120001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1319:
window_manager_delegate_->OnWmSetBounds(window, &transit_bounds_in_dip);
On 2016/12/02 23:18:25, Fady Samuel wrote:
> Add window->SetBounds(transit_bounds_in_dip)?

Done.

[thanhph, 2016-12-06 16:55:26]

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

[commit-bot: I haz the power, 2016-12-06 16:55:53]

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, 2016-12-06 16:58:44]

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

[commit-bot: I haz the power, 2016-12-06 16:58:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[thanhph, 2016-12-06 17:02:59]

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

[commit-bot: I haz the power, 2016-12-06 17:03:25]

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, 2016-12-06 17:23:14]

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

[commit-bot: I haz the power, 2016-12-06 17:23:15]

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

[thanhph, 2016-12-06 18:13:12]

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

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

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

[thanhph, 2016-12-06 18:33:59]

The CQ bit was unchecked by thanhph@chromium.org

[thanhph, 2016-12-06 18:35:30]

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

[commit-bot: I haz the power, 2016-12-06 18:36:07]

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, 2016-12-06 19:39:26]

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

[commit-bot: I haz the power, 2016-12-06 19:39:27]

Dry run: This issue passed the CQ dry run.

[thanhph, 2016-12-06 20:07:45]

thanhph@chromium.org changed reviewers:
+ mfomitchev@chromium.org, sky@chromium.org
- fsamuel@chromium.org

[thanhph, 2016-12-06 20:07:46]

sky, mfomitchev: Please review changes in 

I get the patch to pass all the test with this latest cl.

In ui/public/cpp/window_manager_delegate.h, when I tried to change from

void OnWmSetBounds(aura::Window* window, gfx::Rect* bounds) override;

to

void OnWmSetBounds(aura::Window* window, const gfx::Rect& bounds) override;

but the bug is not fixed (crash after chrome launcher is clicked for small
screen size i.e: ./chrome --mash --screen-config=800x800). 

Let me know if I can leave the patch as it's. Thanks!
Thanh.

[thanhph, 2016-12-06 20:10:35]

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

[thanhph, 2016-12-06 20:12:37]

On 2016/12/06 20:07:46, thanhph wrote:
> sky, mfomitchev: Please review changes in 
> 
> I get the patch to pass all the test with this latest cl.
> 
> In ui/public/cpp/window_manager_delegate.h, when I tried to change from
> 
> void OnWmSetBounds(aura::Window* window, gfx::Rect* bounds) override;
> 
> to
> 
> void OnWmSetBounds(aura::Window* window, const gfx::Rect& bounds) override;
> 
> but the bug is not fixed (crash after chrome launcher is clicked for small
> screen size i.e: ./chrome --mash --screen-config=800x800). 
> 
> Let me know if I can leave the patch as it's. Thanks!
> Thanh.

+fady. The WindowServerTest.SetBoundsSecurity doesn't crash with my new patch as
it did in incorrect patch set 9.

[Fady Samuel, 2016-12-06 20:50:08]

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1326:
window_manager_internal_client_->WmResponse(change_id, false);
I'm confused about this? Shouldn't the second argument be *true* not false?

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1311:
window_manager_internal_client_->WmResponse(change_id, false);
I'm confused about this. Shouldn't the second argument be *true* and not false?

[mfomitchev, 2016-12-06 20:55:20]

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1323:
window->SetBounds(transit_bounds_in_dip);
Don't we still need to call 
window_manager_delegate_->OnWmSetBounds(window, &bounds)
here?
And then NOT set the bounds on the window?

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1308:
window->SetBoundsFromServer(transit_bounds);
Same here - don't we still need to call OnWmSetBounds?

[mfomitchev, 2016-12-06 20:57:44]

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1326:
window_manager_internal_client_->WmResponse(change_id, false);
On 2016/12/06 20:50:07, Fady Samuel wrote:
> I'm confused about this? Shouldn't the second argument be *true* not false?

I think false is right here - we want the client to always take the bounds from
the server - see
https://cs.chromium.org/chromium/src/services/ui/ws/window_tree.cc?q=WmRespon...

[thanhph, 2016-12-06 21:41:05]

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1323:
window->SetBounds(transit_bounds_in_dip);
On 2016/12/06 20:55:20, mfomitchev wrote:
> Don't we still need to call 
> window_manager_delegate_->OnWmSetBounds(window, &bounds)
> here?
> And then NOT set the bounds on the window?

The window bound is already set here (window->SetBounds(transit_bounds_in_dip))
so I removed the window_manager_delegate_->OnWmSetBounds(window, &bounds)

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1326:
window_manager_internal_client_->WmResponse(change_id, false);
On 2016/12/06 20:57:44, mfomitchev wrote:
> On 2016/12/06 20:50:07, Fady Samuel wrote:
> > I'm confused about this? Shouldn't the second argument be *true* not false?
> 
> I think false is right here - we want the client to always take the bounds
from
> the server - see
>
https://cs.chromium.org/chromium/src/services/ui/ws/window_tree.cc?q=WmRespon...

Thanks for the reference Mikhail. Let me know if you have concerns.

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1308:
window->SetBoundsFromServer(transit_bounds);
On 2016/12/06 20:55:20, mfomitchev wrote:
> Same here - don't we still need to call OnWmSetBounds?

I still the same argument as above. window->SetBoundsFromServer(transit_bounds)
already sets the bounds on the window so I removed 
window_manager_delegate_->OnWmSetBounds(window->GetWindow(), &bounds).

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1311:
window_manager_internal_client_->WmResponse(change_id, false);
On 2016/12/06 20:50:08, Fady Samuel wrote:
> I'm confused about this. Shouldn't the second argument be *true* and not
false?

Same answer as your above comment.

[mfomitchev, 2016-12-06 21:57:11]

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1323:
window->SetBounds(transit_bounds_in_dip);
On 2016/12/06 21:41:04, thanhph wrote:
> On 2016/12/06 20:55:20, mfomitchev wrote:
> > Don't we still need to call 
> > window_manager_delegate_->OnWmSetBounds(window, &bounds)
> > here?
> > And then NOT set the bounds on the window?
> 
> The window bound is already set here
(window->SetBounds(transit_bounds_in_dip))
> so I removed the window_manager_delegate_->OnWmSetBounds(window, &bounds)

window_manager_delegate_ (which is ash::mus::WindowManager in this case) may not
set the bounds that WindowTreeClient is trying to set though, which is likely
the reason this bug exists. It is important to let WindowManager handle this
though, and so we can't get rid of this call. Let's chat more in person
tomorrow. We can probably land this refactoring anyway, but it doesn't seem like
it fixes the crash.

[mfomitchev, 2016-12-06 22:11:35]

I suspect the problem here is that InFlightBoundsChange::Revert() is reverting
the change which was made by the server in response to WM's request.

I.e.
1. client asks for the bounds change
2. Server asks WM to make the change
3. WM sets *different* bounds
3.1.1. The different bounds are set on the server
3.1.2. The different bounds are set on the client
3.2.1. The "false" WmResponse for the original request is returned to the
server.
3.2.2. The server communicates the failure to the client.
3.2.3. The client reverts the change, reverting to the bounds before the
sequence started, and reverting the *correct* change made in 3.1.2.

 If so, we need to make InFlightBoundsChange::Revert smarter.

[sky, 2016-12-06 23:14:52]

On 2016/12/06 22:11:35, mfomitchev wrote:
> I suspect the problem here is that InFlightBoundsChange::Revert() is reverting
> the change which was made by the server in response to WM's request.
> 
> I.e.
> 1. client asks for the bounds change
> 2. Server asks WM to make the change
> 3. WM sets *different* bounds
> 3.1.1. The different bounds are set on the server
> 3.1.2. The different bounds are set on the client

This results in WindowTreeClient::OnWindowBoundsChanged() being called, which
should update the revert values of the InFlightChange
(InFlightBoundsChange::SetRevertValueFrom).

> 3.2.1. The "false" WmResponse for the original request is returned to the
> server.
> 3.2.2. The server communicates the failure to the client.
> 3.2.3. The client reverts the change, reverting to the bounds before the
> sequence started, and reverting the *correct* change made in 3.1.2.

As 3.1.2 update the revert value, the revert here should go to the bounds
communicated from the server in 3.1.2. The unittest
WindowTreeClientWmTest.SetBoundsFailedWithPendingChange should exercise this
sequence.

>  If so, we need to make InFlightBoundsChange::Revert smarter.

[chromium-reviews, 2016-12-06 23:34:25]

Ah, I see, thanks. We'll dig more into this then.

On Tue, Dec 6, 2016 at 6:14 PM <sky@chromium.org> wrote:

> On 2016/12/06 22:11:35, mfomitchev wrote:
> > I suspect the problem here is that InFlightBoundsChange::Revert() is
> reverting
> > the change which was made by the server in response to WM's request.
> >
> > I.e.
> > 1. client asks for the bounds change
> > 2. Server asks WM to make the change
> > 3. WM sets *different* bounds
> > 3.1.1. The different bounds are set on the server
> > 3.1.2. The different bounds are set on the client
>
> This results in WindowTreeClient::OnWindowBoundsChanged() being called,
> which
> should update the revert values of the InFlightChange
> (InFlightBoundsChange::SetRevertValueFrom).
>
>
> > 3.2.1. The "false" WmResponse for the original request is returned to the
> > server.
> > 3.2.2. The server communicates the failure to the client.
> > 3.2.3. The client reverts the change, reverting to the bounds before the
> > sequence started, and reverting the *correct* change made in 3.1.2.
>
> As 3.1.2 update the revert value, the revert here should go to the bounds
> communicated from the server in 3.1.2. The unittest
> WindowTreeClientWmTest.SetBoundsFailedWithPendingChange should exercise
> this
> sequence.
>
>
> > If so, we need to make InFlightBoundsChange::Revert smarter.
>
>
>
> https://codereview.chromium.org/2548513002/
>

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

[thanhph, 2016-12-07 17:36:20]

On 2016/12/06 23:34:25, chromium-reviews wrote:
> Ah, I see, thanks. We'll dig more into this then.
> 
> On Tue, Dec 6, 2016 at 6:14 PM <mailto:sky@chromium.org> wrote:
> 
> > On 2016/12/06 22:11:35, mfomitchev wrote:
> > > I suspect the problem here is that InFlightBoundsChange::Revert() is
> > reverting
> > > the change which was made by the server in response to WM's request.
> > >
> > > I.e.
> > > 1. client asks for the bounds change
> > > 2. Server asks WM to make the change
> > > 3. WM sets *different* bounds
> > > 3.1.1. The different bounds are set on the server
> > > 3.1.2. The different bounds are set on the client
> >
> > This results in WindowTreeClient::OnWindowBoundsChanged() being called,
> > which
> > should update the revert values of the InFlightChange
> > (InFlightBoundsChange::SetRevertValueFrom).
> >
> >
> > > 3.2.1. The "false" WmResponse for the original request is returned to the
> > > server.
> > > 3.2.2. The server communicates the failure to the client.
> > > 3.2.3. The client reverts the change, reverting to the bounds before the
> > > sequence started, and reverting the *correct* change made in 3.1.2.
> >
> > As 3.1.2 update the revert value, the revert here should go to the bounds
> > communicated from the server in 3.1.2. The unittest
> > WindowTreeClientWmTest.SetBoundsFailedWithPendingChange should exercise
> > this
> > sequence.
> >
> >
> > > If so, we need to make InFlightBoundsChange::Revert smarter.
> >
> >
> >
> > https://codereview.chromium.org/2548513002/
> >
> 
> -- 
> 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.

The bug actually got fixed by some landed cls by today morning. I'll close this
bug 668277 for now and review what has been updated.

Scott: If changing function bool WindowManager::OnWmSetBounds to return void is
highly desirable at the moment, I can point this code review page to the new bug
and start on it. Thanks!

[sky, 2016-12-07 18:07:55]

As you have all this fresh in your head I would do the change now.

  -Scott

On Wed, Dec 7, 2016 at 9:36 AM,  <thanhph@chromium.org> wrote:
> On 2016/12/06 23:34:25, chromium-reviews wrote:
>> Ah, I see, thanks. We'll dig more into this then.
>>
>> On Tue, Dec 6, 2016 at 6:14 PM <mailto:sky@chromium.org> wrote:
>>
>> > On 2016/12/06 22:11:35, mfomitchev wrote:
>> > > I suspect the problem here is that InFlightBoundsChange::Revert() is
>> > reverting
>> > > the change which was made by the server in response to WM's request.
>> > >
>> > > I.e.
>> > > 1. client asks for the bounds change
>> > > 2. Server asks WM to make the change
>> > > 3. WM sets *different* bounds
>> > > 3.1.1. The different bounds are set on the server
>> > > 3.1.2. The different bounds are set on the client
>> >
>> > This results in WindowTreeClient::OnWindowBoundsChanged() being called,
>> > which
>> > should update the revert values of the InFlightChange
>> > (InFlightBoundsChange::SetRevertValueFrom).
>> >
>> >
>> > > 3.2.1. The "false" WmResponse for the original request is returned to
>> > > the
>> > > server.
>> > > 3.2.2. The server communicates the failure to the client.
>> > > 3.2.3. The client reverts the change, reverting to the bounds before
>> > > the
>> > > sequence started, and reverting the *correct* change made in 3.1.2.
>> >
>> > As 3.1.2 update the revert value, the revert here should go to the
>> > bounds
>> > communicated from the server in 3.1.2. The unittest
>> > WindowTreeClientWmTest.SetBoundsFailedWithPendingChange should exercise
>> > this
>> > sequence.
>> >
>> >
>> > > If so, we need to make InFlightBoundsChange::Revert smarter.
>> >
>> >
>> >
>> > https://codereview.chromium.org/2548513002/
>> >
>>
>> --
>> 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.
>
> The bug actually got fixed by some landed cls by today morning. I'll close
> this
> bug 668277 for now and review what has been updated.
>
> Scott: If changing function bool WindowManager::OnWmSetBounds to return void
> is
> highly desirable at the moment, I can point this code review page to the new
> bug
> and start on it. Thanks!
>
> https://codereview.chromium.org/2548513002/

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

[thanhph, 2016-12-07 18:40:48]

Description was changed from

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

BUG=668277
==========

to

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

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

[thanhph, 2016-12-07 18:40:59]

thanhph@chromium.org changed reviewers:
- fsamuel@chromium.org

[thanhph, 2016-12-07 18:41:44]

Description was changed from

==========
App launcher crashes if display resolution is too small. 

Make sure window->bounds() are in sync with window_tree_host_->GetBounds().

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

to

==========
Update bool WindowManager::OnWmSetBounds() to match with its desirable behavior.

In ui/aura/mus/window_manager_delegate.h, change bool
OnWmSetBounds(aura::Window* window, gfx::Rect* bounds) to void
OnWmSetBounds(aura::Window* window, const gfx::Rect& bounds).

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

[thanhph, 2016-12-07 20:37:16]

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

[commit-bot: I haz the power, 2016-12-07 20:37:58]

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

[thanhph, 2016-12-07 20:48:12]

Thanks for the comments and discussions. I have uploaded the new cl addressing
function WindowManager::OnWmSetBounds(),i.e, to switch from bool function to
void function.

Thanh.

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1323:
window->SetBounds(transit_bounds_in_dip);
On 2016/12/06 21:57:11, mfomitchev wrote:
> On 2016/12/06 21:41:04, thanhph wrote:
> > On 2016/12/06 20:55:20, mfomitchev wrote:
> > > Don't we still need to call 
> > > window_manager_delegate_->OnWmSetBounds(window, &bounds)
> > > here?
> > > And then NOT set the bounds on the window?
> > 
> > The window bound is already set here
> (window->SetBounds(transit_bounds_in_dip))
> > so I removed the window_manager_delegate_->OnWmSetBounds(window, &bounds)
> 
> window_manager_delegate_ (which is ash::mus::WindowManager in this case) may
not
> set the bounds that WindowTreeClient is trying to set though, which is likely
> the reason this bug exists. It is important to let WindowManager handle this
> though, and so we can't get rid of this call. Let's chat more in person
> tomorrow. We can probably land this refactoring anyway, but it doesn't seem
like
> it fixes the crash.

Done, thanks! I added this back in the new cl.

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/240001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1308:
window->SetBoundsFromServer(transit_bounds);
On 2016/12/06 21:41:04, thanhph wrote:
> On 2016/12/06 20:55:20, mfomitchev wrote:
> > Same here - don't we still need to call OnWmSetBounds?
> 
> I still the same argument as above.
window->SetBoundsFromServer(transit_bounds)
> already sets the bounds on the window so I removed 
> window_manager_delegate_->OnWmSetBounds(window->GetWindow(), &bounds).

Done, I added back window_manager_delegate_->OnWmSetBounds.

[thanhph, 2016-12-07 20:52:15]

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

[mfomitchev, 2016-12-07 20:52:48]

Can you please update the CL description similar to how you updated the bug?

[commit-bot: I haz the power, 2016-12-07 20:52:52]

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

[thanhph, 2016-12-07 20:54:20]

Description was changed from

==========
Update bool WindowManager::OnWmSetBounds() to match with its desirable behavior.

In ui/aura/mus/window_manager_delegate.h, change bool
OnWmSetBounds(aura::Window* window, gfx::Rect* bounds) to void
OnWmSetBounds(aura::Window* window, const gfx::Rect& bounds).

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

to

==========
Update bool WindowManager::OnWmSetBounds() to match with its desirable behavior.

Function WindowManagerDelegate::OnWmSetBounds should have no return value, take
a const gfx::Rect& bounds and let the client set whatever bounds it wants on the
window. WindowTreeClient::WmSetBounds would call WmResponse with false always. 

Doing this means the WindowManagerDelegate can set whatever bounds it wants, and
by supplying false to WmResponse we ensure the client fallsback to the most
recent value from the server, which is either the original bounds (if the
delegate didn't change anything) or the bounds the delegate changed the window
too (which may be the supplied bounds).

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

[thanhph, 2016-12-07 20:55:23]

On 2016/12/07 20:52:48, mfomitchev wrote:
> Can you please update the CL description similar to how you updated the bug?

Done, thanks!

[mfomitchev, 2016-12-07 20:59:46]

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
File mash/simple_wm/simple_wm.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
mash/simple_wm/simple_wm.cc:141: void SimpleWM::OnWmSetBounds(aura::Window*
window, gfx::Rect* bounds) {
Update the signature.

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
mash/simple_wm/simple_wm.cc:141: void SimpleWM::OnWmSetBounds(aura::Window*
window, gfx::Rect* bounds) {
Here and in all other implementations - I think we need to set the bounds on the
window...?

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
File services/ui/public/cpp/window_manager_delegate.h (right):

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_manager_delegate.h:73: // A client requested the
bounds of |window| to change to |bounds|. Return
Please update the comment

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1324:
window->SetBounds(transit_bounds_in_dip);
This is wrong - we are overwriting the bounds set by WM. I think this should be
removed.

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1327: // Returning false ensures
the client applies the bounds we set below.
We are not setting any bounds below :)
We are setting bounds in the delegate.

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1310:
window->SetBoundsFromServer(transit_bounds);
Ditto - remove

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1312: if (window_manager_internal_client_)
A comment similar to what you have in the Mus WTC would be nice here.

[commit-bot: I haz the power, 2016-12-07 21:14:05]

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

[commit-bot: I haz the power, 2016-12-07 21:14:06]

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

[thanhph, 2016-12-08 00:20:29]

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
File mash/simple_wm/simple_wm.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
mash/simple_wm/simple_wm.cc:141: void SimpleWM::OnWmSetBounds(aura::Window*
window, gfx::Rect* bounds) {
On 2016/12/07 20:59:46, mfomitchev wrote:
> Update the signature.

Done.

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
File services/ui/public/cpp/window_manager_delegate.h (right):

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_manager_delegate.h:73: // A client requested the
bounds of |window| to change to |bounds|. Return
On 2016/12/07 20:59:46, mfomitchev wrote:
> Please update the comment

Done.

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1324:
window->SetBounds(transit_bounds_in_dip);
On 2016/12/07 20:59:46, mfomitchev wrote:
> This is wrong - we are overwriting the bounds set by WM. I think this should
be
> removed.

Done.

https://codereview.chromium.org/2548513002/diff/280001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1327: // Returning false ensures
the client applies the bounds we set below.
On 2016/12/07 20:59:46, mfomitchev wrote:
> We are not setting any bounds below :)
> We are setting bounds in the delegate.

Done, I removed the comment.

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1310:
window->SetBoundsFromServer(transit_bounds);
On 2016/12/07 20:59:46, mfomitchev wrote:
> Ditto - remove

Done.

https://codereview.chromium.org/2548513002/diff/280001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1312: if (window_manager_internal_client_)
On 2016/12/07 20:59:46, mfomitchev wrote:
> A comment similar to what you have in the Mus WTC would be nice here.

Done.

[mfomitchev, 2016-12-08 20:44:23]

As discussed, you should probably sit on the CL until chrome switches to use
Aura Mus, but some comments nevertheless.

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
File mash/simple_wm/simple_wm.cc (right):

https://codereview.chromium.org/2548513002/diff/280001/mash/simple_wm/simple_...
mash/simple_wm/simple_wm.cc:141: void SimpleWM::OnWmSetBounds(aura::Window*
window, gfx::Rect* bounds) {
On 2016/12/08 00:20:29, thanhph wrote:
> On 2016/12/07 20:59:46, mfomitchev wrote:
> > Update the signature.
> 
> Done.

See also my other comment above - I think we need to set the bounds on the
window (in all WM impls, not just here), since the client lib is no longer doing
it.

https://codereview.chromium.org/2548513002/diff/320001/services/ui/public/cpp...
File services/ui/public/cpp/window_manager_delegate.h (right):

https://codereview.chromium.org/2548513002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_manager_delegate.h:73: // Set window bounds.
Please elaborate on what this is actually supposed to do.

https://codereview.chromium.org/2548513002/diff/320001/services/ui/public/cpp...
File services/ui/public/cpp/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/320001/services/ui/public/cpp...
services/ui/public/cpp/window_tree_client.cc:1326: // Returning false ensures
the client applies the bounds we set above in the
"the bounds which the |window_manager_delegate_| sets."

https://codereview.chromium.org/2548513002/diff/320001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2548513002/diff/320001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1313: // delegate.
Ditto

https://codereview.chromium.org/2548513002/diff/320001/ui/aura/window.cc
File ui/aura/window.cc (left):

https://codereview.chromium.org/2548513002/diff/320001/ui/aura/window.cc#oldc...
ui/aura/window.cc:296: if (parent_ && parent_->layout_manager())
Please put this back