views: impose minimum width on non-Harmony snappables

views: impose minimum width on non-Harmony snappables

Some dialogs, like the permission bubble, have very narrow
preferred contents view widths, but titles that require wider
dialogs. Instead of these dialogs individually imposing
minimum widths, require all snappable non-Harmony dialogs to
have a minimum width.

BUG=726184
Review-Url: https://codereview.chromium.org/2907873002
Cr-Commit-Position: refs/heads/master@{#475900}
Committed: https://chromium.googlesource.com/chromium/src/+/436d5e22db0b978fc69612f62f8a9432622d8fe1

[Elly Fong-Jones, 2017-05-26 17:19:03]

Description was changed from

==========
views: impose minimum width on non-Harmony snappables

Some dialogs, like the permission bubble, have very narrow
preferred contents view widths, but titles that require wider
dialogs. Instead of these dialogs individually imposing
minimum widths, require all snappable non-Harmony dialogs to
have a minimum width.

BUG=726184
==========

to

==========
views: impose minimum width on non-Harmony snappables

Some dialogs, like the permission bubble, have very narrow
preferred contents view widths, but titles that require wider
dialogs. Instead of these dialogs individually imposing
minimum widths, require all snappable non-Harmony dialogs to
have a minimum width.

BUG=726184
==========

[Elly Fong-Jones, 2017-05-26 17:19:03]

ellyjones@chromium.org changed reviewers:
+ pkasting@chromium.org

[Elly Fong-Jones, 2017-05-26 17:19:09]

pkasting: ptal? :)

[Peter Kasting, 2017-05-26 23:59:11]

LGTM

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right):

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:86: // than this
value.
Nit: Dunno whether you want to say "we could try to factor in the desired title
width as well here, but it's not worth the complication."

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:87: return min_width <
320 ? 320 : min_width;
Nit: Simpler:

  return std::max(min_width, 320);

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right):

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:81: int
GetSnappedDialogWidth(int min_width) const override;
It seems like rather than overriding here we should put this implementation in
the base class.

We expect users to be using either the "chrome" or "harmony" layout provider as
their concrete provider, so the only cases affected would be unittests.  Did you
originally place this here and not there to avoid affecting tests?

[Elly Fong-Jones, 2017-05-30 14:55:37]

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/chrome_layout_provider.cc (right):

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:86: // than this
value.
On 2017/05/26 23:59:10, Peter Kasting wrote:
> Nit: Dunno whether you want to say "we could try to factor in the desired
title
> width as well here, but it's not worth the complication."

Done.

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.cc:87: return min_width <
320 ? 320 : min_width;
On 2017/05/26 23:59:10, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return std::max(min_width, 320);

Done.

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right):

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:81: int
GetSnappedDialogWidth(int min_width) const override;
On 2017/05/26 23:59:10, Peter Kasting wrote:
> It seems like rather than overriding here we should put this implementation in
> the base class.
> 
> We expect users to be using either the "chrome" or "harmony" layout provider
as
> their concrete provider, so the only cases affected would be unittests.  Did
you
> originally place this here and not there to avoid affecting tests?

I put this here because I think we are trying to keep Harmony-specific (or
Chrome-specific) stuff out of //ui/views, but I was on the fence about whether
this is Chrome-specific or not. If it seems like it's not, then I can gladly
move this to the base class :)

[Peter Kasting, 2017-05-30 16:17:54]

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
File chrome/browser/ui/views/harmony/chrome_layout_provider.h (right):

https://codereview.chromium.org/2907873002/diff/1/chrome/browser/ui/views/har...
chrome/browser/ui/views/harmony/chrome_layout_provider.h:81: int
GetSnappedDialogWidth(int min_width) const override;
On 2017/05/30 14:55:36, Elly Fong-Jones wrote:
> On 2017/05/26 23:59:10, Peter Kasting wrote:
> > It seems like rather than overriding here we should put this implementation
in
> > the base class.
> > 
> > We expect users to be using either the "chrome" or "harmony" layout provider
> as
> > their concrete provider, so the only cases affected would be unittests.  Did
> you
> > originally place this here and not there to avoid affecting tests?
> 
> I put this here because I think we are trying to keep Harmony-specific (or
> Chrome-specific) stuff out of //ui/views, but I was on the fence about whether
> this is Chrome-specific or not. If it seems like it's not, then I can gladly
> move this to the base class :)

I would probably move it under the justification that it's behavior we want to
affect other code that lives in ui/views/.

[Elly Fong-Jones, 2017-05-30 18:50:06]

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

[Elly Fong-Jones, 2017-05-30 18:50:10]

sky: ptal? :)

[sky, 2017-05-30 22:51:18]

LGTM

[Elly Fong-Jones, 2017-05-31 12:51:26]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-05-31 12:51:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2907873002/#ps40001
(title: "move into ui/views")

[commit-bot: I haz the power, 2017-05-31 12:51:43]

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-05-31 13:59:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496235086067020,
"parent_rev": "5ab0c1a69a67befc6a0ace956a8f617a670a72f1", "commit_rev":
"436d5e22db0b978fc69612f62f8a9432622d8fe1"}

[commit-bot: I haz the power, 2017-05-31 13:59:19]

Description was changed from

==========
views: impose minimum width on non-Harmony snappables

Some dialogs, like the permission bubble, have very narrow
preferred contents view widths, but titles that require wider
dialogs. Instead of these dialogs individually imposing
minimum widths, require all snappable non-Harmony dialogs to
have a minimum width.

BUG=726184
==========

to

==========
views: impose minimum width on non-Harmony snappables

Some dialogs, like the permission bubble, have very narrow
preferred contents view widths, but titles that require wider
dialogs. Instead of these dialogs individually imposing
minimum widths, require all snappable non-Harmony dialogs to
have a minimum width.

BUG=726184

Review-Url: https://codereview.chromium.org/2907873002
Cr-Commit-Position: refs/heads/master@{#475900}
Committed:
https://chromium.googlesource.com/chromium/src/+/436d5e22db0b978fc69612f62f8a...
==========

[commit-bot: I haz the power, 2017-05-31 13:59:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/436d5e22db0b978fc69612f62f8a...