Clean-up positioning and spacing in chrome/browser/ui/views/location_bar/location_bar_view.cc

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 9714e3c17e304bd4319f045a765b344e1fc84bdb..e428ebc5e95883de1ab4e39aeef6bbfad9cc76b1 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -9,6 +9,7 @@
 
 #include "base/command_line.h"
 #include "base/i18n/rtl.h"
+#include "base/memory/scoped_vector.h"
 #include "base/stl_util.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -120,6 +121,187 @@ const int kDesktopScriptBadgeEdgeItemPadding = kDesktopScriptBadgeItemPadding;
 const int kTouchItemPadding = 8;
 const int kTouchEdgeItemPadding = kTouchItemPadding;
 
+// Description of a decoration to be added inside the location bar, either to
+// the left or to the right.
+struct LocationBarDecoration {

[Peter Kasting, 2012/12/21 19:17:01]

Consider defining all this code in a separate .h/.cc file.  Files with one
class/struct each are easier to understand.

[beaudoin, 2013/01/03 03:30:45]

Done. (Although it's still a local struct, but now within the much smaller
location_bar_layout.cc)

+  LocationBarDecoration(int y,
+                        int height,
+                        bool auto_collapse,
+                        double max_fraction,
+                        int edge_item_padding,
+                        int item_padding,
+                        int builtin_padding,
+                        views::View* view)
+      : y(y),
+        height(height),
+        auto_collapse(auto_collapse),
+        max_fraction(max_fraction),
+        edge_item_padding(edge_item_padding),
+        item_padding(item_padding),
+        builtin_padding(builtin_padding),
+        view(view),
+        computed_width(0) {
+    DCHECK(!auto_collapse || max_fraction == 0.0);
+    DCHECK(max_fraction >= 0.0);
+  }
+
+  // Simpler constructor for regular decorations.
+  LocationBarDecoration(int height, int builtin_padding, views::View* view)
+      : y(LocationBarView::kVerticalEdgeThickness),
+        height(height),
+        auto_collapse(false),
+        max_fraction(0),
+        edge_item_padding(LocationBarView::GetEdgeItemPadding()),
+        item_padding(LocationBarView::GetItemPadding()),
+        builtin_padding(builtin_padding),
+        view(view),
+        computed_width(0) {
+    DCHECK(!auto_collapse || max_fraction == 0.0);
+    DCHECK(max_fraction >= 0.0);
+  }
+
+  // The y position of the view inside its parent.
+  int y;
+
+  // If 0, will use the preferred height of the view.
+  int height;
+
+  // True means that, if there is not enough available space in the location
+  // bar, the view will reduce its width either to its minimal width or to zero
+  // (making it invisible), whichever fits. If true, |max_faction| must be 0.
+  bool auto_collapse;
+
+  // Used for resizeable decorations, indicates the maximum fraction of the
+  // location bar that can be taken by this decoration, 0 for non-resizable
+  // decorations. If non-zero, |auto_collapse| must be false.
+  double max_fraction;
+
+  // Padding to use if the decoration is the first element next to the edge.
+  int edge_item_padding;
+
+  // Padding to use if the decoration follows another decoration.
+  int item_padding;
+
+  // Padding built into the decoration and that should be removed, on
+  // both sides, during layout.
+  int builtin_padding;
+
+  views::View* view;
+
+  // The width computed by the layout process.
+  double computed_width;
+};
+
+// First pass of decoration layout process. Pass the full width of the location
+// bar in |entry_width|. This pass will adjust it to account for
+// non-collapsible and non-resizable decorations. |decorations| must always be
+// ordered from the edge of the location bar towards the middle.
+void LayoutDecorationsPass1(
+    ScopedVector<LocationBarDecoration>& decorations,

[Peter Kasting, 2012/12/21 19:17:01]

Instead of some non-scoped functions that each take scoped vectors, consider
something more like:

class LocationBarLayout {
  void AddDecoration(...);
  void LayoutPass1(...);
  ...
}

The caller doesn't really care about the decoration objects except to keep
passing them back to the layout functions, so this abstracts those away.

This might also allow you to make layout cheaper, if you can preserve this
object as a member of the LocationBarView, and not run some of the layout passes
after the first time.  (You might have to modify the pass functions slightly for
this.  For example, maybe you precalculate all the widths of the objects in
different passes, and then you only apply those widths on each subsequent run,
or something.)

Finally -- you can't pass a non-const ref anyway per the style guide.

[beaudoin, 2013/01/03 03:30:45]

New LocationBarLayout class: Done.

However I did not do the precalculation trick because the AddDecoration logic is
performed at every layout. Precalculating would either require a major change to
this approach or else some relatively involved logic in LocationBarLayout to
figure out everything that was added is the same as before and that the
available width is also the same.

An additional problem with any caching is that the views:View* in
LocationBarDecoration is weak and I'm not sure I can keep it beyond the lifetime
of LocationBarView::Layout.

Unless you feel strongly about this, I propose not doing any
caching/precomputation for now.

+    int item_edit_padding,
+    int edge_edit_padding,
+    int* entry_width) {
+
+  bool first_item = true;
+  bool at_least_one_visible = false;
+  for (ScopedVector<LocationBarDecoration>::iterator it(decorations.begin());
+       it != decorations.end(); ++it) {
+    // Autocollapsing decorations are ignored in this pass.
+    if (!(*it)->auto_collapse) {
+      at_least_one_visible = true;
+      *entry_width -= -2 * (*it)->builtin_padding +
+          (first_item ? (*it)->edge_item_padding : (*it)->item_padding);
+    }
+    first_item = false;
+    // Resizing decorations are ignored in this pass.
+    if (!(*it)->auto_collapse && (*it)->max_fraction == 0.0) {
+      (*it)->computed_width = (*it)->view->GetPreferredSize().width();
+      *entry_width -= (*it)->computed_width;
+    }
+  }
+  *entry_width -= at_least_one_visible ? item_edit_padding : edge_edit_padding;
+}
+
+// Second pass of decoration layout process. Pass the |entry_width| computed
+// by the first pass. This pass will adjust it to account for resizable
+// decorations. |decorations| must always be ordered from the edge of the
+// location bar towards the middle.
+void LayoutDecorationsPass2(
+    ScopedVector<LocationBarDecoration>& decorations,
+    int *entry_width) {
+
+  for (ScopedVector<LocationBarDecoration>::iterator it(decorations.begin());
+       it != decorations.end(); ++it) {
+    if ((*it)->max_fraction > 0.0) {
+      int max_width = static_cast<int>(*entry_width * (*it)->max_fraction);
+      (*it)->computed_width = std::min((*it)->view->GetPreferredSize().width(),
+          std::max((*it)->view->GetMinimumSize().width(), max_width));
+      *entry_width -= (*it)->computed_width;
+    }
+  }
+}
+
+// Third and final pass of decoration layout process. Pass the |bounds|
+// corresponding to the entire space available in the location bar. This pass
+// will update it as decorations are laid out. |available_width| measures the
+// empty space within the location bar, taking the decorations and text into
+// account. |decorations| must always be ordered from the edge of the location
+// bar towards the middle.
+void LayoutDecorationsPass3(
+    ScopedVector<LocationBarDecoration>& decorations,
+    int item_edit_padding,
+    int edge_edit_padding,
+    bool left,
+    gfx::Rect* bounds,
+    int* available_width) {
+  bool first_visible = true;
+  for (ScopedVector<LocationBarDecoration>::iterator it(decorations.begin());
+       it != decorations.end(); ++it) {
+    // Collapse decorations if needed.
+    if ((*it)->auto_collapse) {
+      int padding = -2 * (*it)->builtin_padding +
+          (first_visible ? (*it)->edge_item_padding : (*it)->item_padding);
+      // Try preferred size, if it fails try minimum size, if it fails collapse.
+      (*it)->computed_width = (*it)->view->GetPreferredSize().width();
+      if ((*it)->computed_width + padding > *available_width)
+        (*it)->computed_width = (*it)->view->GetMinimumSize().width();
+      if ((*it)->computed_width + padding > *available_width) {
+        (*it)->computed_width = 0;
+        (*it)->view->SetVisible(false);
+      } else {
+        (*it)->view->SetVisible(true);
+        (*available_width) -= (*it)->computed_width + padding;
+      }
+    } else {
+      (*it)->view->SetVisible(true);
+    }
+    // Layout visible decorations.
+    if ((*it)->view->visible()) {
+      int padding = -(*it)->builtin_padding +
+          (first_visible ? (*it)->edge_item_padding : (*it)->item_padding);
+      first_visible = false;
+      int x;
+      if (left)
+        x = bounds->x() + padding;
+      else
+        x = bounds->x() + bounds->width() - padding - (*it)->computed_width;
+      int height = (*it)->height == 0 ?
+          (*it)->view->GetPreferredSize().height() : (*it)->height;
+      (*it)->view->SetBounds(x, (*it)->y, (*it)->computed_width, height);
+      bounds->set_width(bounds->width() - padding - (*it)->computed_width +
+                        (*it)->builtin_padding);
+      if (left) {
+        bounds->set_x(bounds->x() + padding + (*it)->computed_width -
+                      (*it)->builtin_padding);
+      }
+    }
+  }
+  int final_padding = first_visible ? edge_edit_padding : item_edit_padding;
+  bounds->set_width(bounds->width() - final_padding);
+  if (left)
+    bounds->set_x(bounds->x() + final_padding);
+}
+
 }  // namespace
 
 // static
@@ -640,8 +822,6 @@ void LocationBarView::Layout() {
   // TODO(jhawkins): Remove once crbug.com/101994 is fixed.
   CHECK(location_icon_view_);
 
-  // TODO(sky): baseline layout.
-  int location_y = kVerticalEdgeThickness;
   // In some cases (e.g. fullscreen mode) we may have 0 height.  We still want
   // to position our child views in this case, because other things may be
   // positioned relative to them (e.g. the "bookmark added" bubble if the user
@@ -660,96 +840,26 @@ void LocationBarView::Layout() {
   const int kEdgeEditPadding = GetEdgeItemPadding() - kEditInternalSpace;
   const int kBubbleVerticalPadding = (mode_ == POPUP) ?
       -1 : kBubbleHorizontalPadding;
+  const double kMaxBubbleFraction = 0.5;

[Peter Kasting, 2012/12/21 19:17:01]

Nit: Add comment about this

[beaudoin, 2013/01/03 03:30:45]

Done.

+  const int kBubbleLocationY = kVerticalEdgeThickness + kBubbleVerticalPadding;
+
+  ScopedVector<LocationBarDecoration> left_decorations;
+  ScopedVector<LocationBarDecoration> right_decorations;
 
-  // Start by reserving the padding at the right edge.
-  int entry_width = width() - kEdgeThickness;
-  // No need for edge item padding with action box as it fills
-  // all the area on the right.
-  if (!action_box_button_view_)
-    entry_width -= GetEdgeItemPadding();
-
-  // |location_icon_view_| is visible except when |ev_bubble_view_| or
-  // |selected_keyword_view_| are visible.
-  int location_icon_width = 0;
-  int ev_bubble_width = 0;
+  selected_keyword_view_->SetVisible(false);
   location_icon_view_->SetVisible(false);
   ev_bubble_view_->SetVisible(false);
+  keyword_hint_view_->SetVisible(false);
 
   const string16 keyword(location_entry_->model()->keyword());
   const bool is_keyword_hint(location_entry_->model()->is_keyword_hint());
   const bool show_selected_keyword = !keyword.empty() && !is_keyword_hint;
-  if (show_selected_keyword) {
-    // Assume the keyword might be hidden.
-    entry_width -= (kEdgeThickness + kEdgeEditPadding);
-  } else if (model_->GetSecurityLevel() == ToolbarModel::EV_SECURE) {
-    ev_bubble_view_->SetVisible(true);
-    ev_bubble_view_->SetLabel(model_->GetEVCertName());
-    ev_bubble_width = ev_bubble_view_->GetPreferredSize().width();
-    // We'll adjust this width and take it out of |entry_width| below.
-  } else {
-    location_icon_view_->SetVisible(true);
-    location_icon_width = location_icon_view_->GetPreferredSize().width();
-    entry_width -= (kEdgeThickness + GetEdgeItemPadding() +
-                    location_icon_width + kItemEditPadding);
-  }
-
-  if (action_box_button_view_) {
-    action_box_button_view_->SetVisible(true);
-    entry_width -= action_box_button_view_->width() + GetItemPadding();
-  }
-  if (star_view_ && star_view_->visible())
-    entry_width -= star_view_->GetPreferredSize().width() + GetItemPadding();
-
-  if (script_bubble_icon_view_ && script_bubble_icon_view_->visible()) {
-    entry_width -= script_bubble_icon_view_->GetPreferredSize().width() +
-                   GetItemPadding();
-  }
-
-  if (open_pdf_in_reader_view_ && open_pdf_in_reader_view_->visible()) {
-    entry_width -= open_pdf_in_reader_view_->GetPreferredSize().width() +
-        GetItemPadding();
-  }
-  for (PageActionViews::const_iterator i(page_action_views_.begin());
-       i != page_action_views_.end(); ++i) {
-    if ((*i)->visible())
-      entry_width -= ((*i)->GetPreferredSize().width() + GetItemPadding());
-  }
-  if (zoom_view_->visible())
-    entry_width -= zoom_view_->GetPreferredSize().width() + GetItemPadding();
-  for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
-       i != content_setting_views_.end(); ++i) {
-    if ((*i)->visible())
-      entry_width -= ((*i)->GetPreferredSize().width() + GetItemPadding());
-  }
-  if (web_intents_button_view_->visible()) {
-    entry_width -= web_intents_button_view_->GetPreferredSize().width() +
-        GetItemPadding();
-  }
-  // The gap between the edit and whatever is to its right is shortened.
-  entry_width += kEditInternalSpace;
-
-  // Size the EV bubble after taking star/page actions/content settings out of
-  // |entry_width| so we won't take too much space.
-  if (ev_bubble_width) {
-    // Try to elide the bubble to be no larger than half the total available
-    // space, but never elide it any smaller than 150 px.
-    static const int kMinElidedBubbleWidth = 150;
-    static const double kMaxBubbleFraction = 0.5;
-    const int total_padding =
-        kEdgeThickness + kBubbleHorizontalPadding + kItemEditPadding;
-    ev_bubble_width = std::min(ev_bubble_width, std::max(kMinElidedBubbleWidth,
-        static_cast<int>((entry_width - total_padding) * kMaxBubbleFraction)));
-    entry_width -= (total_padding + ev_bubble_width);
-  }
-
-  const int max_edit_width = location_entry_->GetMaxEditWidth(entry_width);
-  if (max_edit_width < 0)
-    return;
-
   const bool show_keyword_hint = !keyword.empty() && is_keyword_hint;
-  selected_keyword_view_->SetVisible(show_selected_keyword);
-  keyword_hint_view_->SetVisible(show_keyword_hint);
   if (show_selected_keyword) {
+    // Autocollapsible bubble decoration.
+    left_decorations.push_back(new LocationBarDecoration(
+        kBubbleLocationY, 0, true, 0, kBubbleHorizontalPadding,
+        GetItemPadding(), 0, selected_keyword_view_));
     if (selected_keyword_view_->keyword() != keyword) {
       selected_keyword_view_->SetKeyword(keyword);
       const TemplateURL* template_url =
@@ -767,140 +877,94 @@ void LocationBarView::Layout() {
         selected_keyword_view_->set_is_extension_icon(false);
       }
     }
-  } else if (show_keyword_hint) {
-    if (keyword_hint_view_->keyword() != keyword)
-      keyword_hint_view_->SetKeyword(keyword);
+  } else if (model_->GetSecurityLevel() == ToolbarModel::EV_SECURE) {
+    ev_bubble_view_->SetLabel(model_->GetEVCertName());
+    // Bubble decoration that can use a maximum fraction of the width.
+    left_decorations.push_back(new LocationBarDecoration(
+        kBubbleLocationY, 0, false, kMaxBubbleFraction,
+        kBubbleHorizontalPadding, GetItemPadding(), 0, ev_bubble_view_));
+  } else {
+    left_decorations.push_back(new LocationBarDecoration(
+        location_height, location_icon_view_->GetBuiltInHorizontalPadding(),
+        location_icon_view_));
   }
 
-  // Lay out items to the right of the edit field.
-  int offset = width() - kEdgeThickness;
   if (action_box_button_view_) {
-    offset -= action_box_button_view_->width();
-    action_box_button_view_->SetPosition(
-        gfx::Point(offset,
-                   kVerticalEdgeThickness -
-                       ActionBoxButtonView::kBorderOverlap));
-    offset -= GetItemPadding();
-  } else {
-    offset -= GetEdgeItemPadding();
+    right_decorations.push_back(new LocationBarDecoration(
+        kVerticalEdgeThickness - ActionBoxButtonView::kBorderOverlap, 0, false,
+        0, 0, 0, 0, action_box_button_view_));
   }
-
   if (star_view_ && star_view_->visible()) {
-    offset += star_view_->GetBuiltInHorizontalPadding();
-    int star_width = star_view_->GetPreferredSize().width();
-    offset -= star_width;
-    star_view_->SetBounds(offset, location_y, star_width, location_height);
-    offset -= GetItemPadding() - star_view_->GetBuiltInHorizontalPadding();
+    right_decorations.push_back(new LocationBarDecoration(
+        location_height, star_view_->GetBuiltInHorizontalPadding(),
+        star_view_));
   }
-
   if (script_bubble_icon_view_ && script_bubble_icon_view_->visible()) {
-    offset += script_bubble_icon_view_->GetBuiltInHorizontalPadding();
-    int width = script_bubble_icon_view_->GetPreferredSize().width();
-    offset -= width;
-    script_bubble_icon_view_->SetBounds(
-        offset, location_y, width, location_height);
-    offset -= GetItemPadding() -
-        script_bubble_icon_view_->GetBuiltInHorizontalPadding();
+    right_decorations.push_back(new LocationBarDecoration(
+        location_height,
+        script_bubble_icon_view_->GetBuiltInHorizontalPadding(),
+        script_bubble_icon_view_));
   }
-
   if (open_pdf_in_reader_view_ && open_pdf_in_reader_view_->visible()) {
-    offset += open_pdf_in_reader_view_->GetBuiltInHorizontalPadding();
-    int icon_width = open_pdf_in_reader_view_->GetPreferredSize().width();
-    offset -= icon_width;
-    open_pdf_in_reader_view_->SetBounds(offset, location_y,
-                                        icon_width, location_height);
-    offset -= GetItemPadding() -
-        open_pdf_in_reader_view_->GetBuiltInHorizontalPadding();
+    right_decorations.push_back(new LocationBarDecoration(
+        location_height,
+        open_pdf_in_reader_view_->GetBuiltInHorizontalPadding(),
+        open_pdf_in_reader_view_));
   }
-
   for (PageActionViews::const_iterator i(page_action_views_.begin());
        i != page_action_views_.end(); ++i) {
     if ((*i)->visible()) {
-      offset += (*i)->GetBuiltInHorizontalPadding();
-      int page_action_width = (*i)->GetPreferredSize().width();
-      offset -= page_action_width;
-      (*i)->SetBounds(offset, location_y, page_action_width, location_height);
-      offset -= GetItemPadding() - (*i)->GetBuiltInHorizontalPadding();
+      right_decorations.push_back(new LocationBarDecoration(
+          location_height, (*i)->GetBuiltInHorizontalPadding(), (*i)));
     }
   }
-
-  if (zoom_view_->visible()) {
-    int zoom_width = zoom_view_->GetPreferredSize().width();
-    offset -= zoom_width;
-    zoom_view_->SetBounds(offset, location_y, zoom_width, location_height);
-    offset -= GetItemPadding();
-  }
-
-  // We use a reverse_iterator here because we're laying out the views from
-  // right to left but in the vector they're ordered left to right.
+  if (zoom_view_->visible())
+    right_decorations.push_back(new LocationBarDecoration(location_height, 0,
+                                                          zoom_view_));
   for (ContentSettingViews::const_reverse_iterator
        i(content_setting_views_.rbegin()); i != content_setting_views_.rend();
        ++i) {
     if ((*i)->visible()) {
-      offset += (*i)->GetBuiltInHorizontalPadding();
-      int content_blocked_width = (*i)->GetPreferredSize().width();
-      offset -= content_blocked_width;
-      (*i)->SetBounds(offset, location_y + kBubbleVerticalPadding,
-                      content_blocked_width, (*i)->GetPreferredSize().height());
-      offset -= GetItemPadding() - (*i)->GetBuiltInHorizontalPadding();
+      // Bubble decoration.

[Peter Kasting, 2012/12/21 19:17:01]

Comment is too brief.  Why are these "bubble decorations"?

[beaudoin, 2013/01/03 03:30:45]

Actually, comment is not really needed, dropped it.

+      right_decorations.push_back(new LocationBarDecoration(
+          kBubbleLocationY, 0, false, 0, GetEdgeItemPadding(), GetItemPadding(),
+          (*i)->GetBuiltInHorizontalPadding(), (*i)));
     }
   }
-
-  // Now the web intents button.
   if (web_intents_button_view_->visible()) {
-    offset += web_intents_button_view_->GetBuiltInHorizontalPadding();
-    int width = web_intents_button_view_->GetPreferredSize().width();
-    offset -= width;
-    web_intents_button_view_->SetBounds(
-        offset, location_y + kBubbleVerticalPadding, width,
-        web_intents_button_view_->GetPreferredSize().height());
-    offset -= GetItemPadding() -
-              web_intents_button_view_->GetBuiltInHorizontalPadding();
+    right_decorations.push_back(new LocationBarDecoration(
+        kBubbleLocationY, 0, false, 0, GetEdgeItemPadding(), GetItemPadding(),
+        web_intents_button_view_->GetBuiltInHorizontalPadding(),
+        web_intents_button_view_));
   }
-
-  // Now lay out items to the left of the edit field.
-  if (location_icon_view_->visible()) {
-    location_icon_view_->SetBounds(
-        kEdgeThickness + GetEdgeItemPadding() -
-        location_icon_view_->GetBuiltInHorizontalPadding(),
-        location_y, location_icon_width, location_height);
-    offset = location_icon_view_->bounds().right() + kItemEditPadding -
-        location_icon_view_->GetBuiltInHorizontalPadding();
-  } else if (ev_bubble_view_->visible()) {
-    ev_bubble_view_->SetBounds(kEdgeThickness + kBubbleHorizontalPadding,
-        location_y + kBubbleVerticalPadding, ev_bubble_width,
-        ev_bubble_view_->GetPreferredSize().height());
-    offset = ev_bubble_view_->bounds().right() + kItemEditPadding;
-  } else {
-    offset = kEdgeThickness +
-        (show_selected_keyword ? kBubbleHorizontalPadding : kEdgeEditPadding);
+  if (show_keyword_hint) {
+    // Collapsible regular decoration

[Peter Kasting, 2012/12/21 19:17:01]

Nit: Trailing period.  Comment is also pretty brief.

[beaudoin, 2013/01/03 03:30:45]

Comment dropped.

+    right_decorations.push_back(new LocationBarDecoration(
+        kVerticalEdgeThickness, 0, true, 0, GetEdgeItemPadding(),
+        GetItemPadding(), 0, keyword_hint_view_));
+    if (keyword_hint_view_->keyword() != keyword)
+      keyword_hint_view_->SetKeyword(keyword);
   }
 
-  // Now lay out the edit field and views that autocollapse to give it more
-  // room.
-  gfx::Rect location_bounds(offset, location_y, entry_width, location_height);
-  if (show_selected_keyword) {
-    selected_keyword_view_->SetBounds(0, location_y + kBubbleVerticalPadding,
-        0, selected_keyword_view_->GetPreferredSize().height());
-    LayoutView(selected_keyword_view_, kItemEditPadding,
-               AvailableWidth(max_edit_width), true, &location_bounds);
-    location_bounds.set_x(selected_keyword_view_->visible() ?
-        (offset + selected_keyword_view_->width() + kItemEditPadding) :
-        (kEdgeThickness + kEdgeEditPadding));
-  } else if (show_keyword_hint) {
-    keyword_hint_view_->SetBounds(0, location_y, 0, location_height);
-    // Tricky: |entry_width| has already been enlarged by |kEditInternalSpace|.
-    // But if we add a trailing view, it needs to have that enlargement be to
-    // its left.  So we undo the enlargement, then include it in the padding for
-    // the added view.
-    location_bounds.Inset(0, 0, kEditInternalSpace, 0);
-    LayoutView(keyword_hint_view_, kItemEditPadding,
-               AvailableWidth(max_edit_width), false, &location_bounds);
-    if (!keyword_hint_view_->visible()) {
-      // Put back the enlargement that we undid above.
-      location_bounds.Inset(0, 0, -kEditInternalSpace, 0);
-    }
-  }
+  // Perform layout.
+  int full_width = width() - 2 * kEdgeThickness;
+  int entry_width = full_width;  // Starts with full width and update it.

[Peter Kasting, 2012/12/21 19:17:01]

Nit: Grammar.  Comment doesn't add much anyway.

[beaudoin, 2013/01/03 03:30:45]

Comment dropped.

+  LayoutDecorationsPass1(left_decorations, kItemEditPadding, kEdgeEditPadding,
+                         &entry_width);
+  LayoutDecorationsPass1(right_decorations, kItemEditPadding, kEdgeEditPadding,
+                         &entry_width);
+  LayoutDecorationsPass2(left_decorations, &entry_width);
+  LayoutDecorationsPass2(right_decorations, &entry_width);
+
+  int available_width = entry_width - location_entry_->TextWidth();
+  // The bounds must be wide enough for all the decorations to fit.
+  gfx::Rect location_bounds(kEdgeThickness, kVerticalEdgeThickness,
+                            std::max(full_width, full_width - entry_width),
+                            location_height);
+  LayoutDecorationsPass3(left_decorations, kItemEditPadding, kEdgeEditPadding,
+                         true, &location_bounds, &available_width);
+  LayoutDecorationsPass3(right_decorations, kItemEditPadding, kEdgeEditPadding,
+                         false, &location_bounds, &available_width);
 
   // Layout out the suggested text view right aligned to the location
   // entry. Only show the suggested text if we can fit the text from one
@@ -908,16 +972,16 @@ void LocationBarView::Layout() {
   // suggested text. If we can't it means either the suggested text is too big,
   // or the user has scrolled.
 
-  // TODO(sky): We could potentially combine this with the previous step to
-  // force using minimum size if necessary, but currently the chance of showing
-  // keyword hints and suggested text is minimal and we're not confident this
-  // is the right approach for suggested text.
+  // TODO(sky): We could potentially adjust this to take into account suggested
+  // text to force using minimum size if necessary, but currently the chance of
+  // showing keyword hints and suggested text is minimal and we're not confident
+  // this is the right approach for suggested text.
   if (suggested_text_view_) {
     // TODO(sky): need to layout when the user changes caret position.
     int suggested_text_width =
         suggested_text_view_->GetPreferredSize().width();
     int vis_text_width = location_entry_->WidthOfTextAfterCursor();
-    if (vis_text_width + suggested_text_width > entry_width) {
+    if (suggested_text_width > available_width) {
       // Hide the suggested text if the user has scrolled or we can't fit all
       // the suggested text.
       suggested_text_view_->SetBounds(0, 0, 0, 0);
@@ -929,7 +993,7 @@ void LocationBarView::Layout() {
       location_needed_width++;
 #endif
       location_bounds.set_width(std::min(location_needed_width,

[Peter Kasting, 2012/12/21 19:17:01]

Nit: Put this on next line so the args line up

[beaudoin, 2013/01/03 03:30:45]

Done.

-                                         entry_width - suggested_text_width));
+          location_bounds.width() - suggested_text_width));
       // TODO(sky): figure out why this needs the -1.
       suggested_text_view_->SetBounds(location_bounds.right() - 1,
                                       location_bounds.y(),
@@ -1130,29 +1194,6 @@ WebContents* LocationBarView::GetWebContents() const {
   return delegate_->GetWebContents();
 }
 
-int LocationBarView::AvailableWidth(int location_bar_width) {
-  return location_bar_width - location_entry_->TextWidth();
-}
-
-void LocationBarView::LayoutView(views::View* view,
-                                 int padding,
-                                 int available_width,
-                                 bool leading,
-                                 gfx::Rect* bounds) {
-  DCHECK(view && bounds);
-  gfx::Size view_size = view->GetPreferredSize();
-  if ((view_size.width() + padding) > available_width)
-    view_size = view->GetMinimumSize();
-  int desired_width = view_size.width() + padding;
-  view->SetVisible(desired_width < bounds->width());
-  if (view->visible()) {
-    view->SetBounds(
-        leading ? bounds->x() : (bounds->right() - view_size.width()),
-        view->y(), view_size.width(), view->height());
-    bounds->set_width(bounds->width() - desired_width);
-  }
-}
-
 void LocationBarView::RefreshContentSettingViews() {
   for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
        i != content_setting_views_.end(); ++i) {