Compute minimum widths for the toolbar components.

chrome/browser/ui/views/location_bar/location_bar_view.cc

Index: chrome/browser/ui/views/location_bar/location_bar_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 82d54b9e7dbf59efae0ea0a717de7f63a6a05a01..8d87f155575e1538f932b561a443a8ed2fd1b718 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -105,6 +105,11 @@ using views::View;
 
 namespace {
 
+// The search button images are made to look as if they overlay the normal edge
+// images, but to align things, the search button needs to be inset horizontally
+// by 1 px.
+const int kSearchButtonInset = 1;

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

Would it make sense to shift the images to include this inset?

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

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

+
 #if !defined(OS_CHROMEOS)
 Browser* GetBrowserFromDelegate(LocationBarView::Delegate* delegate) {
   WebContents* web_contents = delegate->GetWebContents();
@@ -698,17 +703,53 @@ void LocationBarView::GetAccessibleState(ui::AXViewState* state) {
 }
 
 gfx::Size LocationBarView::GetPreferredSize() {

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

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?

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

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.

-  gfx::Size background_min_size(border_painter_->GetMinimumSize());
+  // Compute minimum height.
+  gfx::Size min_size(border_painter_->GetMinimumSize());
   if (!IsInitialized())
-    return background_min_size;
-
-  gfx::Size origin_chip_view_min_size(origin_chip_view_->GetMinimumSize());
+    return min_size;
   gfx::Size search_button_min_size(search_button_->GetMinimumSize());
-  gfx::Size min_size(background_min_size);
   min_size.SetToMax(search_button_min_size);
-  min_size.set_width(origin_chip_view_min_size.width() +
-                     background_min_size.width() +
-                     search_button_min_size.width());
+
+  // Compute width of omnibox-leading content.
+  const int horizontal_edge_thickness = GetHorizontalEdgeThickness();
+  int leading_width = horizontal_edge_thickness;
+  if (origin_chip_view_->ShouldShow())

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

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?

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

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

+    leading_width += origin_chip_view_->GetMinimumSize().width();
+  if (!omnibox_view_->model()->keyword().empty() &&
+      !omnibox_view_->model()->is_keyword_hint()) {
+    // The selected keyword view can collapse completely.
+  } else if (!toolbar_origin_chip_view_ &&
+      !chrome::ShouldDisplayOriginChipV2() &&
+      (GetToolbarModel()->GetSecurityLevel(false) == ToolbarModel::EV_SECURE)) {
+    leading_width += kBubblePadding +
+        ev_bubble_view_->GetMinimumSizeForLabelText(
+            GetToolbarModel()->GetEVCertName()).width();

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

I remember the GetEVCertName being sensitive to initialization order. Is this
method sequenced well with that? (I remember tests finding this.)

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

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.

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

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.

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

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.

+  } else if (!origin_chip_view_->visible()) {
+    leading_width +=
+        kItemPadding + location_icon_view_->GetMinimumSize().width();
+  }

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

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?

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

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

This code exactly matches the conditionals in Layout().

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

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.

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

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.

+  leading_width += kItemPadding - (base::i18n::IsRTL() ? 1 : 0);

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

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

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

Yeah, that seems like a good idea.  Done.
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.

+
+  // Compute width of omnibox-trailing content.
+  int trailing_width = search_button_->visible() ?
+      (search_button_->GetMinimumSize().width() + kSearchButtonInset) :
+      horizontal_edge_thickness;
+  trailing_width += IncrementalMinimumWidth(star_view_) +

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

Should this be delegated to a LocationBarLayout method to parallelize Layout?

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

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.

+      IncrementalMinimumWidth(translate_icon_view_) +
+      IncrementalMinimumWidth(open_pdf_in_reader_view_) +
+      IncrementalMinimumWidth(manage_passwords_icon_view_) +
+      IncrementalMinimumWidth(zoom_view_) +
+      IncrementalMinimumWidth(generated_credit_card_view_) +
+      IncrementalMinimumWidth(mic_search_view_) + kItemPadding;
+  for (PageActionViews::const_iterator i(page_action_views_.begin());
+       i != page_action_views_.end(); ++i)
+    trailing_width += IncrementalMinimumWidth((*i));
+  for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
+       i != content_setting_views_.end(); ++i)
+    trailing_width += IncrementalMinimumWidth((*i));

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

Need the keyword hint view here? Or since that's optional it needn't be
included?

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

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

+
+  const int kMinimumOmniboxWidth = 150;
+  min_size.set_width(leading_width + kMinimumOmniboxWidth + trailing_width);
   return min_size;
 }
 
@@ -880,10 +921,6 @@ void LocationBarView::Layout() {
   const int horizontal_edge_thickness = GetHorizontalEdgeThickness();
   int full_width = width() - horizontal_edge_thickness - origin_chip_width;
 
-  // The search button images are made to look as if they overlay the normal
-  // edge images, but to align things, the search button needs to be inset
-  // horizontally by 1 px.
-  const int kSearchButtonInset = 1;
   const gfx::Size search_button_size(search_button_->GetPreferredSize());
   const int search_button_reserved_width =
       search_button_size.width() + kSearchButtonInset;
@@ -1043,6 +1080,11 @@ WebContents* LocationBarView::GetWebContents() {
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private:
 
+// static
+int LocationBarView::IncrementalMinimumWidth(views::View* view) {

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

MinimumWidthIncludingPadding ?

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

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.

+  return view->visible() ? (kItemPadding + view->GetMinimumSize().width()) : 0;
+}
+
 int LocationBarView::GetHorizontalEdgeThickness() const {
   // In maximized popup mode, there isn't any edge.
   return (is_popup_mode_ && browser_ && browser_->window() &&