Several cleanup items, and one visible change:

* Eliminte the distinction between "item to item padding" and "item to edge
  padding" because the two values are always equal.
* Don't bother supporting "height 0 = use preferred height" in
  location_bar_layout.*, since only one caller uses it at this point and it's
  easier to understand the code by just making it explicit.
* Switch to using a views::Painter for the popup mode background as well,
  instead of explicitly drawing the images.  This will make it easy to switch
  both modes to ninebox painting in the future.
* Try to reorder code in order to declare variables as close to their use as
  possible, and in the order that they're accessed.
* Visible change: Instead of assuming the edit always has 1 px. of "internal
  space", calculate the correct conditions for which that's true.  This results
  in the OmniboxViewViews text moving right by 1 px. in the LTR case.

BUG=231005,239902
TEST=With "views textfield" on, address bar text moves 1 px. right
R=sky@chromium.org

Review URL: https://codereview.chromium.org/16025004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202916 0039d316-1c4b-4281-b951-d872f2087c98

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

@@ -18,12 +18,10 @@ namespace {
 // Amount of padding at the edges of the bubble.
 //
 // This can't be statically initialized because
-// LocationBarView::GetEdgeItemPadding() depends on whether we are
-// using desktop or touch layout, and this in turn depends on the
-// command line.
+// LocationBarView::GetItemPadding() depends on whether we are using desktop or
+// touch layout, and this in turn depends on the command line.
 int GetBubbleOuterPadding() {
-  return LocationBarView::GetEdgeItemPadding() -
-      LocationBarView::kBubblePadding;
+  return LocationBarView::GetItemPadding() - LocationBarView::kBubblePadding;
 }
 }  // namespace
 

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

@@ -44,7 +44,7 @@ struct LocationBarDecoration {
   // The y position of the view inside its parent.
   int y;
 
-  // If 0, will use the preferred height of the view.
+  // The height of the view.
   int height;
 
   // Used for resizeable decorations, indicates the maximum fraction of the
@@ -85,22 +85,16 @@ LocationBarDecoration::LocationBarDecoration(DecorationType type,
       builtin_padding(builtin_padding),
       view(view),
       computed_width(0) {
-  if (type == NORMAL) {
-    DCHECK_GE(max_fraction, 0.0);
-  } else {
-    DCHECK_EQ(0.0, max_fraction);
-  }
+  DCHECK((max_fraction == 0.0) || ((type == NORMAL) && (max_fraction > 0.0)));
 }
 
 
 // LocationBarLayout ---------------------------------------------------------
 
-LocationBarLayout::LocationBarLayout(Position position,
-                                     int item_edit_padding,
-                                     int edge_edit_padding)
+LocationBarLayout::LocationBarLayout(Position position, int item_edit_padding)
     : position_(position),
-      item_edit_padding_(item_edit_padding),
-      edge_edit_padding_(edge_edit_padding) {}
+      item_edit_padding_(item_edit_padding) {
+}
 
 
 LocationBarLayout::~LocationBarLayout() {
@@ -124,7 +118,7 @@ void LocationBarLayout::AddDecoration(int y,
                                       int builtin_padding,
                                       views::View* view) {
   decorations_.push_back(new LocationBarDecoration(
-      NORMAL, y, height, 0, LocationBarView::GetEdgeItemPadding(),
+      NORMAL, y, height, 0, LocationBarView::GetItemPadding(),
       LocationBarView::GetItemPadding(), builtin_padding, view));
 }
 
@@ -140,12 +134,10 @@ void LocationBarLayout::AddSeparator(int y,
 
 void LocationBarLayout::LayoutPass1(int* entry_width) {
   bool first_item = true;
-  bool at_least_one_visible = false;
   for (Decorations::iterator it(decorations_.begin()); it != decorations_.end();
        ++it) {
     // Autocollapsing decorations are ignored in this pass.
     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);
     }
@@ -156,8 +148,7 @@ void LocationBarLayout::LayoutPass1(int* entry_width) {
       *entry_width -= (*it)->computed_width;
     }
   }
-  *entry_width -= at_least_one_visible ? item_edit_padding_ :
-      edge_edit_padding_;
+  *entry_width -= item_edit_padding_;
 }
 
 void LocationBarLayout::LayoutPass2(int *entry_width) {
@@ -248,9 +239,7 @@ void LocationBarLayout::SetBoundsForDecorations(gfx::Rect* bounds) {
     first_visible = false;
     int x = (position_ == LEFT_EDGE) ? (bounds->x() + padding) :
         (bounds->right() - padding - curr->computed_width);
-    int height = curr->height == 0 ?
-        curr->view->GetPreferredSize().height() : curr->height;
-    curr->view->SetBounds(x, curr->y, curr->computed_width, height);
+    curr->view->SetBounds(x, curr->y, curr->computed_width, curr->height);
     bounds->set_width(bounds->width() - padding - curr->computed_width +
                       curr->builtin_padding);
     if (position_ == LEFT_EDGE) {
@@ -258,8 +247,7 @@ void LocationBarLayout::SetBoundsForDecorations(gfx::Rect* bounds) {
           bounds->x() + padding + curr->computed_width - curr->builtin_padding);
     }
   }
-  int final_padding = first_visible ? edge_edit_padding_ : item_edit_padding_;
-  bounds->set_width(bounds->width() - final_padding);
+  bounds->set_width(bounds->width() - item_edit_padding_);
   if (position_ == LEFT_EDGE)
-    bounds->set_x(bounds->x() + final_padding);
+    bounds->set_x(bounds->x() + item_edit_padding_);
 }

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

@@ -26,9 +26,7 @@ class LocationBarLayout {
     RIGHT_EDGE,
   };
 
-  LocationBarLayout(Position position,
-                    int item_edit_padding,
-                    int edge_edit_padding);
+  LocationBarLayout(Position position, int item_edit_padding);
   virtual ~LocationBarLayout();
 
   // Add a decoration, specifying:
@@ -98,9 +96,6 @@ class LocationBarLayout {
   // The padding between the last decoration and the edit box.
   int item_edit_padding_;
 
-  // The padding between the edge and the edit box, if there are no decorations.
-  int edge_edit_padding_;
-
   // The list of decorations to layout.
   Decorations decorations_;
 

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

@@ -152,14 +152,6 @@ const int kBorderRoundCornerWidth = 4;
 // Radius of the round corners inside the location bar.
 const int kBorderCornerRadius = 2;
 
-const int kDesktopItemPadding = 3;
-const int kDesktopEdgeItemPadding = kDesktopItemPadding;
-const int kDesktopScriptBadgeItemPadding = 9;
-const int kDesktopScriptBadgeEdgeItemPadding = kDesktopScriptBadgeItemPadding;
-
-const int kTouchItemPadding = 8;
-const int kTouchEdgeItemPadding = kTouchItemPadding;
-
 }  // namespace
 
 
@@ -208,14 +200,19 @@ LocationBarView::LocationBarView(Browser* browser,
   if (!views::Textfield::IsViewsTextfieldEnabled())
     set_id(VIEW_ID_OMNIBOX);
 
-  if (!is_popup_mode_) {
-    background_painter_.reset(
-        views::Painter::CreateImagePainter(
-            *ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-                IDR_LOCATION_BAR_BORDER).ToImageSkia(),
-            gfx::Insets(kBorderRoundCornerHeight, kBorderRoundCornerWidth,
-                        kBorderRoundCornerHeight, kBorderRoundCornerWidth)));
-  }
+  const int kOmniboxPopupImages[] = {
+    IDR_LOCATIONBG_POPUPMODE_EDGE,
+    IDR_LOCATIONBG_POPUPMODE_CENTER,
+    IDR_LOCATIONBG_POPUPMODE_EDGE,
+  };
+  background_painter_.reset(
+      is_popup_mode_ ?
+      new views::HorizontalPainter(kOmniboxPopupImages) :
+      views::Painter::CreateImagePainter(
+          *ui::ResourceBundle::GetSharedInstance().GetImageNamed(
+              IDR_LOCATION_BAR_BORDER).ToImageSkia(),
+          gfx::Insets(kBorderRoundCornerHeight, kBorderRoundCornerWidth,
+                      kBorderRoundCornerHeight, kBorderRoundCornerWidth)));
 
   edit_bookmarks_enabled_.Init(
       prefs::kEditBookmarksEnabled,
@@ -434,18 +431,14 @@ SkColor LocationBarView::GetColor(ToolbarModel::SecurityLevel security_level,
 
 // static
 int LocationBarView::GetItemPadding() {
+  const int kTouchItemPadding = 8;
   if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH)
     return kTouchItemPadding;
-  return extensions::FeatureSwitch::script_badges()->IsEnabled() ?
-      kDesktopScriptBadgeItemPadding : kDesktopItemPadding;
-}
 
-// static
-int LocationBarView::GetEdgeItemPadding() {
-  if (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH)
-    return kTouchEdgeItemPadding;
+  const int kDesktopScriptBadgeItemPadding = 9;
+  const int kDesktopItemPadding = 3;
   return extensions::FeatureSwitch::script_badges()->IsEnabled() ?
-      kDesktopScriptBadgeEdgeItemPadding : kDesktopEdgeItemPadding;
+      kDesktopScriptBadgeItemPadding : kDesktopItemPadding;
 }
 
 // DropdownBarHostDelegate
@@ -679,10 +672,7 @@ bool LocationBarView::IsLocationEntryFocusableInRootView() const {
 }
 
 gfx::Size LocationBarView::GetPreferredSize() {
-  int sizing_image_id = is_popup_mode_ ?
-      IDR_LOCATIONBG_POPUPMODE_CENTER : IDR_LOCATION_BAR_BORDER;
-  return gfx::Size(
-      0, GetThemeProvider()->GetImageSkiaNamed(sizing_image_id)->height());
+  return background_painter_->GetMinimumSize();
 }
 
 void LocationBarView::Layout() {
@@ -692,29 +682,6 @@ void LocationBarView::Layout() {
   // TODO(jhawkins): Remove once crbug.com/101994 is fixed.
   CHECK(location_icon_view_);
 
-  // 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
-  // hits ctrl-d).
-  const int location_height = GetInternalHeight(false);
-  const int bubble_height = std::max(location_height - (kBubblePadding * 2), 0);
-
-  // The edit has 1 px of horizontal whitespace inside it before the text.
-  const int kEditInternalSpace = 1;
-  // The space between an item and the edit is the normal item space, minus the
-  // edit's built-in space (so the apparent space will be the same).
-  const int kItemEditPadding = GetItemPadding() - kEditInternalSpace;
-  const int kEdgeEditPadding = GetEdgeItemPadding() - kEditInternalSpace;
-  // The largest fraction of the omnibox that can be taken by resizable
-  // bubble decorations such as the EV_SECURE decoration.
-  const double kMaxBubbleFraction = 0.5;
-  const int bubble_location_y = vertical_edge_thickness() + kBubblePadding;
-
-  LocationBarLayout leading_decorations(LocationBarLayout::LEFT_EDGE,
-                                        kItemEditPadding, kEdgeEditPadding);
-  LocationBarLayout trailing_decorations(LocationBarLayout::RIGHT_EDGE,
-                                         kItemEditPadding, kEdgeEditPadding);
-
   selected_keyword_view_->SetVisible(false);
   location_icon_view_->SetVisible(false);
   ev_bubble_view_->SetVisible(false);
@@ -722,17 +689,31 @@ void LocationBarView::Layout() {
   search_token_view_->SetVisible(false);
   search_token_separator_view_->SetVisible(false);
 
+  const int item_padding = GetItemPadding();
+  // The native edit has 1 px of whitespace inside it before the text when the
+  // text is not scrolled off the leading edge.  The views textfield has 1 px of
+  // whitespace before the text in the RTL case only.
+  const int kEditLeadingInternalSpace =
+      (base::i18n::IsRTL() || GetOmniboxViewWin(location_entry_.get())) ? 1 : 0;
+  LocationBarLayout leading_decorations(
+      LocationBarLayout::LEFT_EDGE, item_padding - kEditLeadingInternalSpace);
+  LocationBarLayout trailing_decorations(LocationBarLayout::RIGHT_EDGE,
+                                         item_padding);
+
   const string16 keyword(location_entry_->model()->keyword());
   const bool is_keyword_hint(location_entry_->model()->is_keyword_hint());
   const bool show_search_token = !search_token_view_->text().empty();
-  const bool show_selected_keyword = !keyword.empty() && !is_keyword_hint &&
-      !show_search_token;
-  const bool show_keyword_hint = !keyword.empty() && is_keyword_hint &&
-      !show_search_token;
-  if (show_selected_keyword) {
-    leading_decorations.AddDecoration(
-        bubble_location_y, bubble_height, true, 0, kBubblePadding,
-        GetItemPadding(), 0, selected_keyword_view_);
+  const int bubble_location_y = vertical_edge_thickness() + kBubblePadding;
+  // 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
+  // hits ctrl-d).
+  const int location_height = GetInternalHeight(false);
+  const int bubble_height = std::max(location_height - (kBubblePadding * 2), 0);
+  if (!keyword.empty() && !is_keyword_hint && !show_search_token) {
+    leading_decorations.AddDecoration(bubble_location_y, bubble_height, true, 0,
+                                      kBubblePadding, item_padding, 0,
+                                      selected_keyword_view_);
     if (selected_keyword_view_->keyword() != keyword) {
       selected_keyword_view_->SetKeyword(keyword);
       const TemplateURL* template_url =
@@ -751,9 +732,11 @@ void LocationBarView::Layout() {
     }
   } else if (model_->GetSecurityLevel() == ToolbarModel::EV_SECURE) {
     ev_bubble_view_->SetLabel(model_->GetEVCertName());
-    leading_decorations.AddDecoration(
-        bubble_location_y, bubble_height, false, kMaxBubbleFraction,
-        kBubblePadding, GetItemPadding(), 0, ev_bubble_view_);
+    // The largest fraction of the omnibox that can be taken by the EV bubble.
+    const double kMaxBubbleFraction = 0.5;
+    leading_decorations.AddDecoration(bubble_location_y, bubble_height, false,
+                                      kMaxBubbleFraction, kBubblePadding,
+                                      item_padding, 0, ev_bubble_view_);
   } else {
     leading_decorations.AddDecoration(
         vertical_edge_thickness(), location_height,
@@ -796,36 +779,37 @@ void LocationBarView::Layout() {
     trailing_decorations.AddDecoration(vertical_edge_thickness(),
                                        location_height, 0, zoom_view_);
   }
-  for (ContentSettingViews::const_reverse_iterator
-       i(content_setting_views_.rbegin()); i != content_setting_views_.rend();
+  for (ContentSettingViews::const_reverse_iterator i(
+           content_setting_views_.rbegin()); i != content_setting_views_.rend();
        ++i) {
     if ((*i)->visible()) {
       trailing_decorations.AddDecoration(
-          bubble_location_y, bubble_height, false, 0, GetEdgeItemPadding(),
-          GetItemPadding(), (*i)->GetBuiltInHorizontalPadding(), (*i));
+          bubble_location_y, bubble_height, false, 0, item_padding,
+          item_padding, (*i)->GetBuiltInHorizontalPadding(), (*i));
     }
   }
-  if (show_keyword_hint) {
-    trailing_decorations.AddDecoration(
-        vertical_edge_thickness(), location_height, true, 0,
-        GetEdgeItemPadding(), GetItemPadding(), 0, keyword_hint_view_);
+  if (!keyword.empty() && is_keyword_hint && !show_search_token) {
+    trailing_decorations.AddDecoration(vertical_edge_thickness(),
+                                       location_height, true, 0, item_padding,
+                                       item_padding, 0, keyword_hint_view_);
     if (keyword_hint_view_->keyword() != keyword)
       keyword_hint_view_->SetKeyword(keyword);
   }
   if (show_search_token) {
+    const int token_height = search_token_view_->GetPreferredSize().height();
     if (model_->GetSearchTermsType() == ToolbarModel::URL_LIKE_SEARCH_TERMS) {
-      leading_decorations.AddDecoration(
-          vertical_edge_thickness(), 0, true, 0, GetEdgeItemPadding(),
-          GetItemPadding(), 0, search_token_view_);
+      leading_decorations.AddDecoration(vertical_edge_thickness(), token_height,
+                                        true, 0, item_padding, item_padding, 0,
+                                        search_token_view_);
     } else {
       trailing_decorations.AddSeparator(vertical_edge_thickness(),
-                                        location_height, GetItemPadding(),
+                                        location_height, item_padding,
                                         search_token_separator_view_);
       // This must be the last item in the right decorations list, otherwise
       // trailing_decorations.set_item_padding() makes no sense.
-      trailing_decorations.AddDecoration(
-          vertical_edge_thickness(), 0, true, 0, GetEdgeItemPadding(),
-          GetItemPadding(), 0, search_token_view_);
+      trailing_decorations.AddDecoration(vertical_edge_thickness(),
+                                         token_height, true, 0, item_padding,
+                                         item_padding, 0, search_token_view_);
       trailing_decorations.set_item_edit_padding(
           views::kUnrelatedControlLargeHorizontalSpacing);
     }
@@ -925,23 +909,14 @@ void LocationBarView::Layout() {
 void LocationBarView::OnPaint(gfx::Canvas* canvas) {
   View::OnPaint(canvas);
 
+  // Maximized popup windows don't draw the horizontal edges.  We implement this
+  // by simply expanding the paint area outside the view by the edge thickness.
   const int horizontal_edge_thickness = GetHorizontalEdgeThickness();
-  if (background_painter_.get()) {
-    background_painter_->Paint(canvas, size());
-  } else {
-    canvas->TileImageInt(
-        *GetThemeProvider()->GetImageSkiaNamed(IDR_LOCATIONBG_POPUPMODE_CENTER),
-        0, 0, horizontal_edge_thickness, 0,
-        width() - (horizontal_edge_thickness * 2), height());
-    if (horizontal_edge_thickness != 0) {
-      canvas->DrawImageInt(
-          *GetThemeProvider()->GetImageSkiaNamed(IDR_LOCATIONBG_POPUPMODE_EDGE),
-          0, 0);
-      canvas->DrawImageInt(
-          *GetThemeProvider()->GetImageSkiaNamed(IDR_LOCATIONBG_POPUPMODE_EDGE),
-          width() - horizontal_edge_thickness, 0);
-    }
-  }
+  gfx::Rect background_rect(GetContentsBounds());
+  if (is_popup_mode_ && (horizontal_edge_thickness == 0))
+    background_rect.Inset(-kPopupEdgeThickness, 0);
+  views::Painter::PaintPainterAt(canvas, background_painter_.get(),
+                                 background_rect);
 
   // Draw the background color so that the graphical elements at the edges
   // appear over the correct color.  (The edit draws its own background, so this
@@ -1311,7 +1286,6 @@ bool LocationBarView::SkipDefaultKeyEventProcessing(const ui::KeyEvent& event) {
 
 #if defined(USE_AURA)
   NOTIMPLEMENTED();
-  return false;
 #else
   OmniboxViewWin* omnibox_win = GetOmniboxViewWin(location_entry_.get());
   if (omnibox_win)

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

@@ -315,12 +315,10 @@ class LocationBarView : public LocationBar,
   // otherwise it will be the current height.
   int GetInternalHeight(bool use_preferred_size);
 
-  // Space between items in the location bar.
+  // Space between items in the location bar, as well as between items and the
+  // edges.
   static int GetItemPadding();
 
-  // Space between the edges and the items next to them.
-  static int GetEdgeItemPadding();
-
   // Thickness of the left and right edges of the omnibox, in normal mode.
   static const int kNormalHorizontalEdgeThickness;
   // The same, but for popup mode.

chrome/browser/ui/views/omnibox/omnibox_views.cc

@@ -18,12 +18,14 @@ OmniboxViewViews* GetOmniboxViewViews(OmniboxView* view) {
       static_cast<OmniboxViewViews*>(view) : NULL;
 }
 
-#if defined(OS_WIN) && !defined(USE_AURA)
 OmniboxViewWin* GetOmniboxViewWin(OmniboxView* view) {
+#if defined(OS_WIN) && !defined(USE_AURA)
   return views::Textfield::IsViewsTextfieldEnabled() ?
       NULL : static_cast<OmniboxViewWin*>(view);
-}
+#else
+  return NULL;
 #endif
+}
 
 OmniboxView* CreateOmniboxView(OmniboxEditController* controller,
                                ToolbarModel* toolbar_model,

chrome/browser/ui/views/omnibox/omnibox_views.h

@@ -25,10 +25,8 @@ class View;
 // Return |view| as an OmniboxViewViews, or NULL if it is of a different type.
 OmniboxViewViews* GetOmniboxViewViews(OmniboxView* view);
 
-#if defined(OS_WIN) && !defined(USE_AURA)
 // Return |view| as an OmniboxViewWin, or NULL if it is of a different type.
 OmniboxViewWin* GetOmniboxViewWin(OmniboxView* view);
-#endif
 
 // Creates an OmniboxView of the appropriate type; Views or Win.
 OmniboxView* CreateOmniboxView(OmniboxEditController* controller,