Compute minimum widths for the toolbar components.

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/command_line.h"
@@ skipping 87 matching lines @@
 
 #if !defined(OS_CHROMEOS)
 #include "chrome/browser/ui/views/first_run_bubble.h"
 #endif
 
 using content::WebContents;
 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();
   return web_contents ? chrome::FindBrowserWithWebContents(web_contents) : NULL;
 }
 #endif
 
 // Given a containing |height| and a |base_font_list|, shrinks the font size
 // until the font list will fit within |height| while having its cap height
 // vertically centered.  Returns the correctly-sized font list.
@@ skipping 572 matching lines @@
 
   if (is_popup_mode_) {
     state->AddStateFlag(ui::AX_STATE_READ_ONLY);
   } else {
     state->set_value_callback =
         base::Bind(&LocationBarView::AccessibilitySetValue,
                    weak_ptr_factory_.GetWeakPtr());
   }
 }
 
 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;
+    return min_size;
+  gfx::Size search_button_min_size(search_button_->GetMinimumSize());
+  min_size.SetToMax(search_button_min_size);
 
-  gfx::Size origin_chip_view_min_size(origin_chip_view_->GetMinimumSize());
-  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;
 }
 
 void LocationBarView::Layout() {
   if (!IsInitialized())
     return;
 
   animated_host_label_->SetVisible(false);
   origin_chip_view_->SetVisible(origin_chip_view_->ShouldShow());
   selected_keyword_view_->SetVisible(false);
@@ skipping 151 matching lines @@
                                        location_height, true, 0, kItemPadding,
                                        kItemPadding, keyword_hint_view_);
     if (keyword_hint_view_->keyword() != keyword)
       keyword_hint_view_->SetKeyword(keyword);
   }
 
   // Perform 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;
   full_width -= search_button_->visible() ?
       search_button_reserved_width : horizontal_edge_thickness;
   int entry_width = full_width;
   leading_decorations.LayoutPass1(&entry_width);
   trailing_decorations.LayoutPass1(&entry_width);
   leading_decorations.LayoutPass2(&entry_width);
   trailing_decorations.LayoutPass2(&entry_width);
@@ skipping 139 matching lines @@
   return delegate_->GetToolbarModel();
 }
 
 WebContents* LocationBarView::GetWebContents() {
   return delegate_->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() &&
       browser_->window()->IsMaximized()) ? 0 : vertical_edge_thickness();
 }
 
 bool LocationBarView::RefreshContentSettingViews() {
   bool visibility_changed = false;
   for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
        i != content_setting_views_.end(); ++i) {
@@ skipping 350 matching lines @@
       bounds().x());
 
   OmniboxPopupView* popup = omnibox_view_->model()->popup_model()->view();
   if (popup->IsOpen())
     popup->UpdatePopupAppearance();
 }
 
 void LocationBarView::OnFocus() {
   // Focus the view widget first which implements accessibility for
   // Chrome OS.  It is noop on Win. This should be removed once
   // Chrome OS migrates to aura, which uses Views' textfield that receives

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

So we've migrated to Aura now, which is why we can ditch this, right?

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

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

   // focus. See crbug.com/106428.
   NotifyAccessibilityEvent(ui::AX_EVENT_FOCUS, false);
 
   // Then focus the native location view which implements accessibility for
   // Windows.
   omnibox_view_->SetFocus();
 }
 
 void LocationBarView::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
@@ skipping 225 matching lines @@
 void LocationBarView::ModelChanged(const SearchModel::State& old_state,
                                    const SearchModel::State& new_state) {
   const bool visible = !GetToolbarModel()->input_in_progress() &&
       new_state.voice_search_supported;
   if (mic_search_view_->visible() != visible) {
     mic_search_view_->SetVisible(visible);
     Layout();
   }
 }