Compute minimum widths for the toolbar components.

Compute minimum widths for the toolbar components.

This includes the toolbar itself, the browser actions area, the location bar,
and location bar components like the EV bubble view.  The omnibox itself (i.e.
the editable control) is given a minimum width of 150 px (which matches the EV
bubble min width).

* Also fixes a bug where the EV bubble was not allowed to shrink as intended.
* Also fixes a bug where the toolbar requested 3px too much size by default when
  the "toolbar origin chip" was enabled.

BUG=58915
TEST=Try to shrink the browser width, make sure the omnibox never gets "too narrow"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271409

[Peter Kasting, 2014-05-09 03:01:04]

This is not yet landable.  I have not yet written an origin chip "minimum width"
routine, and without it, the code here forces the browser window min width to be
large enough to accommodate the origin chip's entire desired width -- not
acceptable for long hostnames.  This routine is going to be somewhat hairy; we
have two different pieces of text in the chip (the cert holder and the
host-or-other-text label) and they should elide differently.  I have to decide
the elision priority and the overall rules of how the chip should interact with
the rest of the omnibox.  Ugh.

However, the remaining changes here are basically independent of that change,
and as such are reviewable ahead of time.

In particular, note the starred bugfixes in the CL description, as they explain
a couple of changes in the code.

[Greg Billock, 2014-05-12 23:09:44]

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:111: const int
kSearchButtonInset = 1;
Would it make sense to shift the images to include this inset?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:705: gfx::Size
LocationBarView::GetPreferredSize() {
I was expecting to see this in a new GetMinimumSize. Is the rationale that this
control is always going to expand to fill the available size anyway?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:716: if
(origin_chip_view_->ShouldShow())
This is origin_chip_view_->visible() in the Layout method. Is the idea that
preferred size should show no matter what since we may be showing it or not
depending on selection state?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:726:
GetToolbarModel()->GetEVCertName()).width();
I remember the GetEVCertName being sensitive to initialization order. Is this
method sequenced well with that? (I remember tests finding this.)

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: }
I'm not sure I'd be willing to swear this is all the possibilities. :-) Should
we parallelize this more by making better state accessors for this, and would
that help elsewhere in the system as well?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:731: leading_width +=
kItemPadding - (base::i18n::IsRTL() ? 1 : 0);
Maybe bump kEditLeadingInternalSpace out of Layout()?

Also, I think it'd be easier to read this if they went in the same order as
Layout() (or at least, that's what I'm looking at while reviewing this.)

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:737: trailing_width +=
IncrementalMinimumWidth(star_view_) +
Should this be delegated to a LocationBarLayout method to parallelize Layout?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: trailing_width +=
IncrementalMinimumWidth((*i));
Need the keyword hint view here? Or since that's optional it needn't be
included?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1084: int
LocationBarView::IncrementalMinimumWidth(views::View* view) {
MinimumWidthIncludingPadding ?

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:216: // other
words: ContainerMinSize() < width() - resize < ClampTo(MAX).
update comment (MinimumNonemptyWidth)

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:217: int min_width
= std::min(
how about "preferred_width"

[Peter Kasting, 2014-05-13 00:55:33]

New patch set up.  This still doesn't address origin chip min width.  It:
(1) Responds to patch set 1 review comments
(2) Changes the omnibox and EV bubble to use character-based min widths (as Mac
already does) instead of pixel-based ones
(3) Moves some declarations/definitions around in omnibox_view_views.* to better
match where functions are actually declared in the various base classes (while I
was touching the file anyway)

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:111: const int
kSearchButtonInset = 1;
On 2014/05/12 23:09:44, Greg Billock wrote:
> Would it make sense to shift the images to include this inset?

I think that's reasonable.  I'd rather do it separately, though.  I filed
http://crbug.com/372608 for this.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:705: gfx::Size
LocationBarView::GetPreferredSize() {
On 2014/05/12 23:09:44, Greg Billock wrote:
> I was expecting to see this in a new GetMinimumSize. Is the rationale that
this
> control is always going to expand to fill the available size anyway?

Yeah, there's no such thing as a "preferred size" for the location bar distinct
from its minimum size, so just defining GetPreferredSize() takes care of both.

Alternately I could probably try to rename this as GetMinimumSize() and not have
a preferred size, but that's a little weirder/more error-prone as the default
View::GetPreferredSize() then will probably return 0 width, so anyone who
happens to call that will get a weird result.  Leaving the code this way --
which is how it was already working, just with an inaccurate minimum value --
seems fine.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:716: if
(origin_chip_view_->ShouldShow())
On 2014/05/12 23:09:44, Greg Billock wrote:
> This is origin_chip_view_->visible() in the Layout method. Is the idea that
> preferred size should show no matter what since we may be showing it or not
> depending on selection state?

Layout() sets |origin_chip_view_| to be visible based on the result of calling
ShouldShow(), so this is functionally equivalent in terms of determining what
the visibility will actually be, but without this function actually touching the
existing visibility of |origin_chip_view_| (since it's wrong for
GetPreferredSize() to actually change the state of children etc.).

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:726:
GetToolbarModel()->GetEVCertName()).width();
On 2014/05/12 23:09:44, Greg Billock wrote:
> I remember the GetEVCertName being sensitive to initialization order. Is this
> method sequenced well with that? (I remember tests finding this.)

I don't think I understand what you're referencing.  initialization order of
what?

Note that this function does check IsInitialized() at the top, if you're
referring to whether LocationBarView::Init() has been called.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: }
On 2014/05/12 23:09:44, Greg Billock wrote:
> I'm not sure I'd be willing to swear this is all the possibilities. :-) Should
> we parallelize this more by making better state accessors for this, and would
> that help elsewhere in the system as well?

Better state accessors for what?  I don't know what you're proposing.

This code exactly matches the conditionals in Layout().

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:731: leading_width +=
kItemPadding - (base::i18n::IsRTL() ? 1 : 0);
On 2014/05/12 23:09:44, Greg Billock wrote:
> Maybe bump kEditLeadingInternalSpace out of Layout()?

Yeah, that seems like a good idea.  Done.

> Also, I think it'd be easier to read this if they went in the same order as
> Layout() (or at least, that's what I'm looking at while reviewing this.)

My initial code was more like that, but I ended up finding it more readable to
place this line at the bottom of the leading section instead of the top.

In general, most of the code here does follow Layout()'s ordering, but that's an
effect of how they're both trying to do things in "visibility order" (outside to
inside) instead of a primary cause.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:737: trailing_width +=
IncrementalMinimumWidth(star_view_) +
On 2014/05/12 23:09:44, Greg Billock wrote:
> Should this be delegated to a LocationBarLayout method to parallelize Layout?

That won't help (it will make code strictly longer and more complex) unless we
move to having long-lived LocationBarLayout objects (e.g. class-scoped instead
of function-scoped).  That might be a good thing but when I considered it I
decided it was a larger change, with more ramifications, than I wanted to think
through for this -- for example, it requires splitting the LocationBarLayout
functionality into "stuff that's permanent" (like what decorations exist) versus
"stuff that's transient" (like their computed widths).  This might already work
today, but verifying that fell into that same "I don't want to think deeply
about this in the middle of this other change" buckets.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: trailing_width +=
IncrementalMinimumWidth((*i));
On 2014/05/12 23:09:44, Greg Billock wrote:
> Need the keyword hint view here? Or since that's optional it needn't be
> included?

Shouldn't be included since it auto-collapses and thus has a min-width of 0.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1084: int
LocationBarView::IncrementalMinimumWidth(views::View* view) {
On 2014/05/12 23:09:44, Greg Billock wrote:
> MinimumWidthIncludingPadding ?

I would agree if this returned the value unconditionally, but since it's based
on whether the view is visible, I think the functionality is unclear enough as
to need the comment in the header -- and that comment in turn suggests that this
is the appropriate function name.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:216: // other
words: ContainerMinSize() < width() - resize < ClampTo(MAX).
On 2014/05/12 23:09:44, Greg Billock wrote:
> update comment (MinimumNonemptyWidth)

Done.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/tool...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:217: int min_width
= std::min(
On 2014/05/12 23:09:44, Greg Billock wrote:
> how about "preferred_width"

OK.

[Greg Billock, 2014-05-13 16:08:15]

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:726:
GetToolbarModel()->GetEVCertName()).width();
On 2014/05/13 00:55:33, Peter Kasting wrote:
> On 2014/05/12 23:09:44, Greg Billock wrote:
> > I remember the GetEVCertName being sensitive to initialization order. Is
this
> > method sequenced well with that? (I remember tests finding this.)
> 
> I don't think I understand what you're referencing.  initialization order of
> what?
> 
> Note that this function does check IsInitialized() at the top, if you're
> referring to whether LocationBarView::Init() has been called.

I think IsInitialized will do it -- basically I remember running into problems
calling GetEVCertName too soon from the origin chip code, but if it's
initialized I think that's the sequence we want to make sure of -- sometimes
it's hard to remember all the ways View methods get called and in what order.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: }
On 2014/05/13 00:55:33, Peter Kasting wrote:
> On 2014/05/12 23:09:44, Greg Billock wrote:
> > I'm not sure I'd be willing to swear this is all the possibilities. :-)
Should
> > we parallelize this more by making better state accessors for this, and
would
> > that help elsewhere in the system as well?
> 
> Better state accessors for what?  I don't know what you're proposing.
> 
> This code exactly matches the conditionals in Layout().

Basically a way to collapse these multi-clause conditionals into accessors that
have meaningful names like "HasEVCertBubble" or "HasKeywordHint".

I'm pretty confident this code is right, but it's easy to imagine Layout()
deviating from this as time goes on.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1458: // Chrome OS
migrates to aura, which uses Views' textfield that receives
So we've migrated to Aura now, which is why we can ditch this, right?

https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:366: void
OmniboxViewViews::SetTextAndSelectedRange(const base::string16& text,
These are all just re-ordered, right?

[Peter Kasting, 2014-05-13 20:50:14]

Just responding to your feedback, still haven't implemented origin chip min
width.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:726:
GetToolbarModel()->GetEVCertName()).width();
On 2014/05/13 16:08:15, Greg Billock wrote:
> On 2014/05/13 00:55:33, Peter Kasting wrote:
> > On 2014/05/12 23:09:44, Greg Billock wrote:
> > > I remember the GetEVCertName being sensitive to initialization order. Is
> this
> > > method sequenced well with that? (I remember tests finding this.)
> > 
> > I don't think I understand what you're referencing.  initialization order of
> > what?
> > 
> > Note that this function does check IsInitialized() at the top, if you're
> > referring to whether LocationBarView::Init() has been called.
> 
> I think IsInitialized will do it -- basically I remember running into problems
> calling GetEVCertName too soon from the origin chip code, but if it's
> initialized I think that's the sequence we want to make sure of -- sometimes
> it's hard to remember all the ways View methods get called and in what order.

I think I recall the problem you're referencing, and if I do, then it was indeed
a case of trying to deref something before IsInitialized() returned true.  So I
think we're good here.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:730: }
On 2014/05/13 16:08:15, Greg Billock wrote:
> On 2014/05/13 00:55:33, Peter Kasting wrote:
> > On 2014/05/12 23:09:44, Greg Billock wrote:
> > > I'm not sure I'd be willing to swear this is all the possibilities. :-)
> Should
> > > we parallelize this more by making better state accessors for this, and
> would
> > > that help elsewhere in the system as well?
> > 
> > Better state accessors for what?  I don't know what you're proposing.
> > 
> > This code exactly matches the conditionals in Layout().
> 
> Basically a way to collapse these multi-clause conditionals into accessors
that
> have meaningful names like "HasEVCertBubble" or "HasKeywordHint".

Ah.  Sure, that's reasonable.  Done.

It's unfortunate that other places in Layout() need some of the same
keyword-related data, but I think the result is still an improvement.

https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1458: // Chrome OS
migrates to aura, which uses Views' textfield that receives
On 2014/05/13 16:08:15, Greg Billock wrote:
> So we've migrated to Aura now, which is why we can ditch this, right?

IIRC, msw is actually removing this in another changelist after I asked the same
question.  :)

https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:366: void
OmniboxViewViews::SetTextAndSelectedRange(const base::string16& text,
On 2014/05/13 16:08:15, Greg Billock wrote:
> These are all just re-ordered, right?

Yes.  The only functional change is the addition of GetMinimumSize().

[Greg Billock, 2014-05-13 21:39:07]

On 2014/05/13 20:50:14, Peter Kasting wrote:
> Just responding to your feedback, still haven't implemented origin chip min
> width.
> 
>
https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:726:
> GetToolbarModel()->GetEVCertName()).width();
> On 2014/05/13 16:08:15, Greg Billock wrote:
> > On 2014/05/13 00:55:33, Peter Kasting wrote:
> > > On 2014/05/12 23:09:44, Greg Billock wrote:
> > > > I remember the GetEVCertName being sensitive to initialization order. Is
> > this
> > > > method sequenced well with that? (I remember tests finding this.)
> > > 
> > > I don't think I understand what you're referencing.  initialization order
of
> > > what?
> > > 
> > > Note that this function does check IsInitialized() at the top, if you're
> > > referring to whether LocationBarView::Init() has been called.
> > 
> > I think IsInitialized will do it -- basically I remember running into
problems
> > calling GetEVCertName too soon from the origin chip code, but if it's
> > initialized I think that's the sequence we want to make sure of -- sometimes
> > it's hard to remember all the ways View methods get called and in what
order.
> 
> I think I recall the problem you're referencing, and if I do, then it was
indeed
> a case of trying to deref something before IsInitialized() returned true.  So
I
> think we're good here.
> 
>
https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:730: }
> On 2014/05/13 16:08:15, Greg Billock wrote:
> > On 2014/05/13 00:55:33, Peter Kasting wrote:
> > > On 2014/05/12 23:09:44, Greg Billock wrote:
> > > > I'm not sure I'd be willing to swear this is all the possibilities. :-)
> > Should
> > > > we parallelize this more by making better state accessors for this, and
> > would
> > > > that help elsewhere in the system as well?
> > > 
> > > Better state accessors for what?  I don't know what you're proposing.
> > > 
> > > This code exactly matches the conditionals in Layout().
> > 
> > Basically a way to collapse these multi-clause conditionals into accessors
> that
> > have meaningful names like "HasEVCertBubble" or "HasKeywordHint".
> 
> Ah.  Sure, that's reasonable.  Done.
> 
> It's unfortunate that other places in Layout() need some of the same
> keyword-related data, but I think the result is still an improvement.
> 
>
https://codereview.chromium.org/271013002/diff/1/chrome/browser/ui/views/loca...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:1458: // Chrome OS
> migrates to aura, which uses Views' textfield that receives
> On 2014/05/13 16:08:15, Greg Billock wrote:
> > So we've migrated to Aura now, which is why we can ditch this, right?
> 
> IIRC, msw is actually removing this in another changelist after I asked the
same
> question.  :)
> 
>
https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
https://codereview.chromium.org/271013002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:366: void
> OmniboxViewViews::SetTextAndSelectedRange(const base::string16& text,
> On 2014/05/13 16:08:15, Greg Billock wrote:
> > These are all just re-ordered, right?
> 
> Yes.  The only functional change is the addition of GetMinimumSize().

cool. all looks good so far.

[Peter Kasting, 2014-05-17 01:34:03]

Actually, Greg, how about I land this much first?  I commented-out the origin
chip min width inclusion in LocationBarView::GetPreferredSize() in order to
prevent people with the chip on from getting excessive minimum window widths.

This way, everybody without the origin chip should get good behavior, and the
changes to fix the origin chip minimum size won't be bundled up with this
change.

[Greg Billock, 2014-05-19 15:11:06]

On 2014/05/17 01:34:03, Peter Kasting wrote:
> Actually, Greg, how about I land this much first?  I commented-out the origin
> chip min width inclusion in LocationBarView::GetPreferredSize() in order to
> prevent people with the chip on from getting excessive minimum window widths.
> 
> This way, everybody without the origin chip should get good behavior, and the
> changes to fix the origin chip minimum size won't be bundled up with this
> change.

I'm happy with that.

LGTM

[Peter Kasting, 2014-05-19 15:12:35]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-05-19 15:13:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/271013002/80001

[commit-bot: I haz the power, 2014-05-19 17:11:26]

Change committed as 271409