alternate ntp: implement right-aligned search token

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

Index: chrome/browser/ui/views/location_bar/location_bar_layout.cc
diff --git a/chrome/browser/ui/views/location_bar/location_bar_layout.cc b/chrome/browser/ui/views/location_bar/location_bar_layout.cc
index b328772cd19548af81d15a76d770644efa37359c..8434db6d3904bea72adb733c9d96c2c03c675628 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_layout.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_layout.cc
@@ -8,32 +8,48 @@
 #include "ui/gfx/rect.h"
 #include "ui/views/view.h"
 
+namespace {
+
+enum DecorationType {
+  // Decoration is always visible.
+  NORMAL = 0,
+  // If there is not enough available space in the location bar, the decoration
+  // will reduce its width either to its minimal width or to zero (making it
+  // invisible), whichever fits.  |LocationBarDecoration::max_fraction| must be
+  // 0.
+  AUTO_COLLAPSE,
+  // Decoration is a separator, only visible if it's not leading, or not
+  // trailing, or not next to another separator.
+  SEPARATOR,
+};
+
+}  // namespace
+
+
 // Description of a decoration to be added inside the location bar, either to
 // the left or to the right.
 struct LocationBarDecoration {
-  LocationBarDecoration(int y,
+  LocationBarDecoration(DecorationType type,
+                        int y,
                         int height,
-                        bool auto_collapse,
                         double max_fraction,
                         int edge_item_padding,
                         int item_padding,
                         int builtin_padding,
                         views::View* view);
 
+  // The type of decoration.
+  DecorationType type;
+
   // 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_fraction| 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.
+  // decorations. If non-zero, |type| must not be AUTO_COLLAPSE.
   double max_fraction;
 
   // Padding to use if the decoration is the first element next to the edge.
@@ -52,24 +68,24 @@ struct LocationBarDecoration {
   double computed_width;
 };
 
-LocationBarDecoration::LocationBarDecoration(int y,
+LocationBarDecoration::LocationBarDecoration(DecorationType type,
+                                             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),
+    : type(type),
+      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(type == NORMAL || max_fraction == 0.0);
   DCHECK(max_fraction >= 0.0);

[Peter Kasting, 2013/01/17 04:14:22]

Nit: It's longer, but this seems less redundant to me:

if (type == NORMAL)
  DCHECK_GE(max_fraction, 0);
else
  DCHECK_EQ(0, max_fraction);

[kuan, 2013/01/17 17:57:43]

Done.

 }
 
@@ -95,34 +111,44 @@ void LocationBarLayout::AddDecoration(int y,
                                       int item_padding,
                                       int builtin_padding,
                                       views::View* view) {
-  decorations_.push_back(new LocationBarDecoration(y, height, auto_collapse,
-      max_fraction, edge_item_padding, item_padding, builtin_padding, view));
+  decorations_.push_back(new LocationBarDecoration(
+      auto_collapse ? AUTO_COLLAPSE : NORMAL, y, height, max_fraction,
+      edge_item_padding, item_padding, builtin_padding, view));
 }
 
 void LocationBarLayout::AddDecoration(int height,
                                       int builtin_padding,
                                       views::View* view) {
-  decorations_.push_back(new LocationBarDecoration(
-      LocationBarView::kVerticalEdgeThickness, height, false, 0,
+  decorations_.push_back(new LocationBarDecoration(NORMAL,

[Peter Kasting, 2013/01/17 04:14:22]

Nit: This should go on the next line so all the lines of args have the same
indenting.

[kuan, 2013/01/17 17:57:43]

Done.

+      LocationBarView::kVerticalEdgeThickness, height, 0,
       LocationBarView::GetEdgeItemPadding(), LocationBarView::GetItemPadding(),
       builtin_padding, view));
 }
 
-void LocationBarLayout::LayoutPass1(int* entry_width) {
+void LocationBarLayout::AddSeparator(int y,
+                                     int height,
+                                     int padding,
+                                     views::View* view) {
+  // Edge item padding won't apply since a separator won't be by the edge, so
+  // use 0 for more accurate evaluation of |entry_width| in LayoutPass1().
+  decorations_.push_back(new LocationBarDecoration(SEPARATOR, y, height, 0,
+      0, padding, 0, view));

[Peter Kasting, 2013/01/17 04:14:22]

Nit: Indent even

[kuan, 2013/01/17 17:57:43]

Done.

+}
 
+void LocationBarLayout::LayoutPass1(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) {
+    if ((*it)->type != 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) {
+    if ((*it)->type != AUTO_COLLAPSE && (*it)->max_fraction == 0.0) {

[Peter Kasting, 2013/01/17 04:14:22]

Nit: Personally, I prefer parens around binary sub-expressions.

[kuan, 2013/01/17 17:57:43]

Done.

       (*it)->computed_width = (*it)->view->GetPreferredSize().width();
       *entry_width -= (*it)->computed_width;
     }
@@ -144,14 +170,21 @@ void LocationBarLayout::LayoutPass2(int *entry_width) {
 }
 
 void LocationBarLayout::LayoutPass3(gfx::Rect* bounds, int* available_width) {
+  SetVisibilityForDecorations(available_width);
+  HideUnneededSeparators(available_width);
+  SetBoundsForDecorations(bounds);
+}
+
+void LocationBarLayout::SetVisibilityForDecorations(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) {
+    if ((*it)->type == 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.
+      // 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();
@@ -165,25 +198,67 @@ void LocationBarLayout::LayoutPass3(gfx::Rect* bounds, int* available_width) {
     } 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);
+
+    if ((*it)->view->visible())
       first_visible = false;
-      int x;
-      if (position_ == LEFT_EDGE)
-        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 (position_ == LEFT_EDGE) {
-        bounds->set_x(bounds->x() + padding + (*it)->computed_width -
-                      (*it)->builtin_padding);
+  }
+}
+
+void LocationBarLayout::HideUnneededSeparators(int* available_width) {
+  // Initialize |last_visible_was_a_separator| to true so that the leading

[Peter Kasting, 2013/01/17 04:14:22]

Nit: the -> any

[kuan, 2013/01/17 17:57:43]

Done.

+  // separators will be hidden.
+  bool last_visible_was_a_separator = true;

[Peter Kasting, 2013/01/17 04:14:22]

Nit: Seems like you don't need this.  You can just check if |trailing_separator|
is pointing at anything.

[kuan, 2013/01/17 17:57:43]

Done.

+  ScopedVector<LocationBarDecoration>::iterator trailing_separator =

[Peter Kasting, 2013/01/17 04:14:22]

Nit: This type name is long.  I suggest a private:

typedef ScopedVector<LocationBarDecoration> Decorations;

...in the header, then use it in this file.

[kuan, 2013/01/17 17:57:43]

Done.

+      decorations_.end();
+  for (ScopedVector<LocationBarDecoration>::iterator it(decorations_.begin());
+       it != decorations_.end(); ++it) {
+    if ((*it)->type == SEPARATOR) {
+      if (last_visible_was_a_separator) {
+        (*it)->view->SetVisible(false);
+        // Add back what was subtracted when setting this separator visible in
+        // LayoutPass1().
+        (*available_width) += (*it)->item_padding + (*it)->computed_width;
+      } else {
+        last_visible_was_a_separator = true;
+        trailing_separator = it;
       }
+    } else if ((*it)->view->visible()) {
+      last_visible_was_a_separator = false;
+      trailing_separator = decorations_.end();
+    }
+  }
+  // If there's a trailing separator, hide it.
+  if (trailing_separator != decorations_.end()) {
+    (*trailing_separator)->view->SetVisible(false);
+    // Add back what was subtracted when setting this separator visible in
+    // LayoutPass1().
+    (*available_width) += (*trailing_separator)->item_padding +
+                          (*trailing_separator)->computed_width;
+  }
+}
+
+void LocationBarLayout::SetBoundsForDecorations(gfx::Rect* bounds) {
+  bool first_visible = true;
+  for (ScopedVector<LocationBarDecoration>::iterator it(decorations_.begin());
+       it != decorations_.end(); ++it) {
+    if (!(*it)->view->visible())

[Peter Kasting, 2013/01/17 04:14:22]

Nit: It probably makes sense to use a temp for either or both of (*it) and
(*it)->view.

[kuan, 2013/01/17 17:57:43]

Done.

+      continue;
+    int padding = -(*it)->builtin_padding +
+        (first_visible ? (*it)->edge_item_padding : (*it)->item_padding);
+    first_visible = false;
+    int x;

[Peter Kasting, 2013/01/17 04:14:22]

Nit: I'd use ?: here so that the variable isn't declared without being
initialized.  (Use parens around the subexpressions.)

[kuan, 2013/01/17 17:57:43]

Done.

+    if (position_ == LEFT_EDGE)
+      x = bounds->x() + padding;
+    else
+      x = bounds->right() - 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 (position_ == LEFT_EDGE) {
+      bounds->set_x(bounds->x() + padding + (*it)->computed_width -
+                    (*it)->builtin_padding);

[Peter Kasting, 2013/01/17 04:14:22]

Nit: We don't indent even in the midst of a single subexpression.  I suggest
putting the whole subexpression on this second line if it will fit; otherwise
indent 4.

[kuan, 2013/01/17 17:57:43]

Done.

     }
   }
   int final_padding = first_visible ? edge_edit_padding_ : item_edit_padding_;