[mus+ash] Fixes minimum browser window size (store / retrieve kPreferredSize property)

[mus+ash] Fixes minimum browser window size with --mash

Store kPreferredSize property in an aura::Window when Chrome browser frame
is updated upon BrowserView::Layout().
Retrieve it in CustomFrameViewAsh::GetMinimumSize() to ensure that in Mus
the browser window frame honors window size constraints.

This only affects chrome browser frame when running with --mash.

BUG=728455
TEST=BrowserNonClientFrameViewAshTest.FrameMinSizeIsUpdated
     CustomFrameViewAshTest.HonorsMinimumSizeProperty

Review-Url: https://codereview.chromium.org/2914793005
Cr-Commit-Position: refs/heads/master@{#481716}
Committed: https://chromium.googlesource.com/chromium/src/+/1a44d226bb121c7f29cbe2edc3c0215ebc3f7a54

[varkha, 2017-06-01 01:02:54]

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

[commit-bot: I haz the power, 2017-06-01 01:03:32]

Dry run: CQ is trying da patch.

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

[varkha, 2017-06-01 01:12:18]

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

[varkha, 2017-06-01 01:12:19]

sadrul@, can you please take a look? This just demonstrates that mirrored
properties indeed can be used. I am not sure if I can use kPreferredSize like
this or we would need a new property (it seems we only use that now to set up
initial size in some cases). Also I am pretty sure it would be better to set the
property elsewhere, maybe in PreferredSizeChanged or something like that,
although I believe the current scheme works with assumption that the minimum
size is not updated frequently and is probably not _too_ resource intensive.
More suggestions welcome.

Thanks!

[commit-bot: I haz the power, 2017-06-01 02:25:50]

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

[commit-bot: I haz the power, 2017-06-01 02:25:50]

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

[varkha, 2017-06-01 02:35:06]

Test failures suggest that I should be using a new property but other questions
/ comments still stand.

[varkha, 2017-06-01 14:53:32]

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

[commit-bot: I haz the power, 2017-06-01 14:53:48]

Dry run: CQ is trying da patch.

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

[varkha, 2017-06-01 14:55:45]

Patchset #2 (id:20001) has been deleted

[varkha, 2017-06-01 14:55:49]

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

[commit-bot: I haz the power, 2017-06-01 14:56:04]

Dry run: CQ is trying da patch.

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

[varkha, 2017-06-01 16:32:16]

Patchset #2 (id:40001) has been deleted

[varkha, 2017-06-01 16:32:21]

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

[commit-bot: I haz the power, 2017-06-01 16:32:39]

Dry run: CQ is trying da patch.

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

[sadrul, 2017-06-01 17:14:20]

https://codereview.chromium.org/2914793005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2914793005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:348: return
min_size;
This is the only part that I am a bit worried about. Specifically, this
information in ash can be stale.

The information the minimum-size seems to depend on are:
 a. Minimum size of the client_view.
 b. frame_values().
 c. tab-strip visibility.
 d. Minimum width of tabstrip.
 e. left-inset of the tabstrip.
 f. right-inset of the tabstrip.

I think b, c, d, e never change after BrowserNonClientFrameViewMus has been
initialized.

f depends on the profile-switcher icon, but I believe we do not use that
chromeos. So f should also never change after BNCFVMus has been initialized.

So the remaining unknown is (a). I guess the client_view is a BrowserView? Looks
like its minimum size is determined by BrowserViewLayout, which takes the
minimum-size of the following views to determine the minimum-size of the
client_view:
 1. tab-strip.
 2. toolbar.
 3. bookmark bar.
 4. infobar.

Hm ... this is already getting too complex.

Essentially, what I think should happen is that the minimum-size should be set
during/right after BNCFVMus is initialized, and then any time something it
depends on changes. Maybe BrowserView can notify back when anything in 1-4
change?

[commit-bot: I haz the power, 2017-06-01 17:48:18]

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

[commit-bot: I haz the power, 2017-06-01 17:48:19]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-06-19 15:57:36]

https://codereview.chromium.org/2914793005/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2914793005/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:348: return
min_size;
On 2017/06/01 17:14:20, sadrul wrote:
> This is the only part that I am a bit worried about. Specifically, this
> information in ash can be stale.
> 
> The information the minimum-size seems to depend on are:
>  a. Minimum size of the client_view.
>  b. frame_values().
>  c. tab-strip visibility.
>  d. Minimum width of tabstrip.
>  e. left-inset of the tabstrip.
>  f. right-inset of the tabstrip.
> 
> I think b, c, d, e never change after BrowserNonClientFrameViewMus has been
> initialized.
> 
> f depends on the profile-switcher icon, but I believe we do not use that
> chromeos. So f should also never change after BNCFVMus has been initialized.
> 
> So the remaining unknown is (a). I guess the client_view is a BrowserView?
Looks
> like its minimum size is determined by BrowserViewLayout, which takes the
> minimum-size of the following views to determine the minimum-size of the
> client_view:
>  1. tab-strip.
>  2. toolbar.
>  3. bookmark bar.
>  4. infobar.
> 
> Hm ... this is already getting too complex.
> 
> Essentially, what I think should happen is that the minimum-size should be set
> during/right after BNCFVMus is initialized, and then any time something it
> depends on changes. Maybe BrowserView can notify back when anything in 1-4
> change?

Thanks for the ideas. Did you have something like this wiring in mind?

[varkha, 2017-06-19 15:57:41]

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

[commit-bot: I haz the power, 2017-06-19 15:57:55]

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-06-19 17:13:28]

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

[commit-bot: I haz the power, 2017-06-19 17:13:29]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-06-21 20:17:51]

sadrul@, ping?

[sadrul, 2017-06-21 21:09:57]

I think this looks good, yeah! lgtm

https://codereview.chromium.org/2914793005/diff/80001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/2914793005/diff/80001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:389: std::max(min_size.height(),
min_window_size->height()));
Maybe use min_size.SetToMin(...) instead.

[varkha, 2017-06-21 22:52:38]

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

[varkha, 2017-06-21 22:52:39]

sky@ can you please take a look for OWNERS? Thanks!

[varkha, 2017-06-21 23:33:27]

Thanks for the tip, sadrul@!

https://codereview.chromium.org/2914793005/diff/80001/ash/frame/custom_frame_...
File ash/frame/custom_frame_view_ash.cc (right):

https://codereview.chromium.org/2914793005/diff/80001/ash/frame/custom_frame_...
ash/frame/custom_frame_view_ash.cc:389: std::max(min_size.height(),
min_window_size->height()));
On 2017/06/21 21:09:57, sadrul wrote:
> Maybe use min_size.SetToMin(...) instead.

I take it you meant SetToMax(). Done.

[varkha, 2017-06-21 23:33:33]

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

[commit-bot: I haz the power, 2017-06-21 23:33:52]

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-06-22 00:50:06]

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

[commit-bot: I haz the power, 2017-06-22 00:50:07]

Dry run: This issue passed the CQ dry run.

[sky, 2017-06-22 03:41:10]

This looks right to me, but how about test coverage that CustomFrameViewAsh
honors the minimum and that BrowserView updates the minimum?

https://codereview.chromium.org/2914793005/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2914793005/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:350:
gfx::Size min_size(min_width, min_client_view_size.height());
Convert back to old code as there is no reason for the local variable here.

[varkha, 2017-06-22 17:43:23]

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

[commit-bot: I haz the power, 2017-06-22 17:43:39]

Dry run: CQ is trying da patch.

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

[varkha, 2017-06-22 17:50:14]

> test coverage that CustomFrameViewAsh honors the minimum and that BrowserView
updates the minimum.

The former seemed easy, the latter was less trivial so I added the code to
update this property in Ash frame as well and updated the Ash test. Could not
think of anything simpler to force BrowserView through updating the layout and
the property.

The addition of TopContainerView::ChildPreferredSizeChanged is to avoid the need
to call BrowserView::ChildPreferredSizeChanged from the test - seems like a
correct thing to do in any case since the bookmark bar may have preferred size
changed and when it is not parented by BrowserView directly the update would not
have propagated up.

WDYT?

https://codereview.chromium.org/2914793005/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2914793005/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:350:
gfx::Size min_size(min_width, min_client_view_size.height());
On 2017/06/22 03:41:10, sky wrote:
> Convert back to old code as there is no reason for the local variable here.

Done.

[sky, 2017-06-22 20:17:29]

Fair enough, LGTM

[varkha, 2017-06-22 20:26:01]

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

[varkha, 2017-06-22 20:26:02]

+tsepez@ for services/ui/public/interfaces/OWNERS (adding a synchronized window
property to hold the minimum window size).

[varkha, 2017-06-22 20:26:16]

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

[commit-bot: I haz the power, 2017-06-22 20:26:43]

Dry run: CQ is trying da patch.

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

[varkha, 2017-06-22 20:32:11]

Description was changed from

==========
[mus+ash] Fixes minimum browser window size with --mash

Experiment with storing / retrieving kPreferredSize property

BUG=728455
==========

to

==========
[mus+ash] Fixes minimum browser window size with --mash

Store kPreferredSize property in an aura::Window when Chrome browser frame
is updated upon BrowserView::Layout().
Retrieve it in CustomFrameViewAsh::GetMinimumSize() to ensure that in Mus
the browser window frame honors window size constraints.

This only affects chrome browser frame when running with --mash.

BUG=728455
TEST=BrowserNonClientFrameViewAshTest.FrameMinSizeIsUpdated
     CustomFrameViewAshTest.HonorsMinimumSizeProperty
==========

[Tom Sepez, 2017-06-22 20:47:17]

mojom lgtm

[commit-bot: I haz the power, 2017-06-22 23:12:04]

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

[commit-bot: I haz the power, 2017-06-22 23:12:05]

Dry run: This issue passed the CQ dry run.

[varkha, 2017-06-22 23:22:09]

The CQ bit was checked by varkha@chromium.org

[varkha, 2017-06-22 23:22:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2914793005/#ps140001
(title: "[mus+ash] Fixes minimum browser window size (tests)")

[commit-bot: I haz the power, 2017-06-22 23:22:34]

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

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498173729193500,
"parent_rev": "0ded2d24b4c9a60998244f4de58cb670af82d13c", "commit_rev":
"1a44d226bb121c7f29cbe2edc3c0215ebc3f7a54"}

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

Description was changed from

==========
[mus+ash] Fixes minimum browser window size with --mash

Store kPreferredSize property in an aura::Window when Chrome browser frame
is updated upon BrowserView::Layout().
Retrieve it in CustomFrameViewAsh::GetMinimumSize() to ensure that in Mus
the browser window frame honors window size constraints.

This only affects chrome browser frame when running with --mash.

BUG=728455
TEST=BrowserNonClientFrameViewAshTest.FrameMinSizeIsUpdated
     CustomFrameViewAshTest.HonorsMinimumSizeProperty
==========

to

==========
[mus+ash] Fixes minimum browser window size with --mash

Store kPreferredSize property in an aura::Window when Chrome browser frame
is updated upon BrowserView::Layout().
Retrieve it in CustomFrameViewAsh::GetMinimumSize() to ensure that in Mus
the browser window frame honors window size constraints.

This only affects chrome browser frame when running with --mash.

BUG=728455
TEST=BrowserNonClientFrameViewAshTest.FrameMinSizeIsUpdated
     CustomFrameViewAshTest.HonorsMinimumSizeProperty

Review-Url: https://codereview.chromium.org/2914793005
Cr-Commit-Position: refs/heads/master@{#481716}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a44d226bb121c7f29cbe2edc3c0...
==========

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

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1a44d226bb121c7f29cbe2edc3c0...