Always aligns text at vertically center (Textfield, Label).

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 244eee7851ce6273d79ef7529c8a5b6fd84cd418..3cf3e287126f66cf9f92ecab4f2bfbcee042f375 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -113,38 +113,21 @@ Browser* GetBrowserFromDelegate(LocationBarView::Delegate* delegate) {
   return contents ? chrome::FindBrowserWithWebContents(contents) : NULL;
 }
 
-// Given a containing |height| and a base |font_list|, shrinks the fonts until
-// the primary font will fit within |height| while having its cap height
-// vertically centered.  Returns the |font_y_offset| needed to produce this
-// centering.
-void CalculateFontAndOffsetForHeight(int height,
-                                     gfx::FontList* font_list,
-                                     int* font_y_offset) {
-#if defined(OS_WIN)
-  base::win::ScopedGetDC screen_dc(NULL);
-#endif
-
+// Given a containing |height| and a |base_font_list|, shrinks the font list

[Peter Kasting, 2013/10/18 21:11:09]

Nit: font list -> font size

[Yuki, 2013/10/21 09:44:41]

Done.

+// until the font list will fit within |height| while having its cap height
+// vertically centered.  Returns the shrinked font list.

[Peter Kasting, 2013/10/18 21:11:09]

Nit: shrinked -> correctly-sized

[Yuki, 2013/10/21 09:44:41]

Done.

+gfx::FontList GetLargestFontListWithHeightBound(
+    const gfx::FontList& base_font_list,
+    int height) {
+  gfx::FontList font_list = base_font_list;
   while (true) {
-    // TODO(pkasting): Expand the gfx::Font metrics (and underlying Skia
-    // metrics) enough to expose the cap height directly.
-#if defined(OS_WIN)
-    const gfx::Font& font = font_list->GetPrimaryFont();
-    base::win::ScopedSelectObject font_in_dc(screen_dc, font.GetNativeFont());
-    TEXTMETRIC tm = {0};
-    GetTextMetrics(screen_dc, &tm);
-    int cap_height = font.GetBaseline() - tm.tmInternalLeading;
-    *font_y_offset = ((height - cap_height) / 2) - tm.tmInternalLeading;
-#else
-    // Without cap height available, we fall back to centering the full height.
-    *font_y_offset = (height - font_list->GetHeight()) / 2;
-#endif
-
-    const int font_size = font_list->GetFontSize();
-    if (((*font_y_offset >= 0) &&
-         ((*font_y_offset + font_list->GetHeight()) <= height)) ||
-        (font_size <= 1))
-      return;
-    *font_list = font_list->DeriveFontListWithSize(font_size - 1);
+    const int y_offset = (height - font_list.GetCapHeight()) / 2 -
+                         (font_list.GetBaseline() - font_list.GetCapHeight());

[Peter Kasting, 2013/10/18 21:11:09]

Nit: Indent 4, not even

[Yuki, 2013/10/21 09:44:41]

Done.

+    if ((y_offset >= 0 &&

[Peter Kasting, 2013/10/18 21:11:09]

Nit: Parens around binary subexprs (omnibox subsystem style)

[Yuki, 2013/10/21 09:44:41]

Done.

+         y_offset + font_list.GetHeight() <= height) ||
+        font_list.GetFontSize() <= 1)
+      return font_list;
+    font_list = font_list.DeriveFontListWithSizeDelta(-1);
   }
 }
 
@@ -249,40 +232,35 @@ void LocationBarView::Init() {
   // Shrink large fonts to make them fit.
   // TODO(pkasting): Stretch the location bar instead in this case.
   int location_height = GetInternalHeight(true);
-  int font_y_offset;
-  CalculateFontAndOffsetForHeight(location_height, &font_list, &font_y_offset);
+  font_list = GetLargestFontListWithHeightBound(font_list, location_height);
 
   // Determine the font for use inside the bubbles.
-  gfx::FontList bubble_font_list(font_list);
-  int bubble_font_y_offset;
   // The bubble background images have 1 px thick edges, which we don't want to
   // overlap.
   const int kBubbleInteriorVerticalPadding = 1;
-  CalculateFontAndOffsetForHeight(
-      location_height - ((kBubblePadding + kBubbleInteriorVerticalPadding) * 2),
-      &bubble_font_list, &bubble_font_y_offset);
-  bubble_font_y_offset += kBubbleInteriorVerticalPadding;
+  const int bubble_vertical_padding =
+      (kBubblePadding + kBubbleInteriorVerticalPadding) * 2;
+  const gfx::FontList bubble_font_list(
+      GetLargestFontListWithHeightBound(
+          font_list, location_height - bubble_vertical_padding));
 
   const SkColor background_color =
       GetColor(ToolbarModel::NONE, LocationBarView::BACKGROUND);
   ev_bubble_view_ = new EVBubbleView(
-      bubble_font_list, bubble_font_y_offset,
+      bubble_font_list, kBubbleInteriorVerticalPadding,

[Peter Kasting, 2013/10/18 21:11:09]

Instead of passing this interior padding value to all these bubbles, can't we
just eliminate this arg entirely?  If the font size we've calculated above is
small enough, then vertically centering it within the entire bubble height seems
like it should work fine.

Indeed, given how the border in these classes is implemented, I think the
current behavior actually pushes the font too low, because you're going to add
this border on top, but not on bottom, and then center the font in the remaining
height.

You can test by visiting paypal.com and looking at the font placement within the
green bubble at the beginning of the omnibox.

[Yuki, 2013/10/21 09:44:41]

I've removed kBubbleInteriorVerticalPadding, and it's working fine.

       GetColor(ToolbarModel::EV_SECURE, SECURITY_TEXT), background_color, this);
   ev_bubble_view_->set_drag_controller(this);
   AddChildView(ev_bubble_view_);
 
   // Initialize the Omnibox view.
   location_entry_.reset(CreateOmniboxView(this, profile_, command_updater(),
-                                          is_popup_mode_, this, font_list,
-                                          font_y_offset));
+                                          is_popup_mode_, this, font_list));
   SetLocationEntryFocusable(true);
   location_entry_view_ = location_entry_->AddToView(this);
 
   // Initialize the inline autocomplete view which is visible only when IME is
   // turned on.  Use the same font with the omnibox and highlighted background.
   ime_inline_autocomplete_view_ = new views::Label(string16(), font_list);
-  ime_inline_autocomplete_view_->set_border(
-      views::Border::CreateEmptyBorder(font_y_offset, 0, 0, 0));
   ime_inline_autocomplete_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   ime_inline_autocomplete_view_->SetAutoColorReadabilityEnabled(false);
   ime_inline_autocomplete_view_->set_background(
@@ -296,13 +274,11 @@ void LocationBarView::Init() {
 
   const SkColor text_color = GetColor(ToolbarModel::NONE, TEXT);
   selected_keyword_view_ = new SelectedKeywordView(
-      bubble_font_list, bubble_font_y_offset, text_color, background_color,
-      profile_);
+      bubble_font_list, kBubbleInteriorVerticalPadding, text_color,
+      background_color, profile_);
   AddChildView(selected_keyword_view_);
 
   suggested_text_view_ = new views::Label(string16(), font_list);
-  suggested_text_view_->set_border(
-      views::Border::CreateEmptyBorder(font_y_offset, 0, 0, 0));
   suggested_text_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   suggested_text_view_->SetAutoColorReadabilityEnabled(false);
   suggested_text_view_->SetEnabledColor(GetColor(
@@ -311,7 +287,7 @@ void LocationBarView::Init() {
   AddChildView(suggested_text_view_);
 
   keyword_hint_view_ = new KeywordHintView(
-      profile_, font_list, font_y_offset,
+      profile_, font_list,
       GetColor(ToolbarModel::NONE, LocationBarView::DEEMPHASIZED_TEXT),
       background_color);
   AddChildView(keyword_hint_view_);
@@ -334,7 +310,8 @@ void LocationBarView::Init() {
   for (int i = 0; i < CONTENT_SETTINGS_NUM_TYPES; ++i) {
     ContentSettingImageView* content_blocked_view =
         new ContentSettingImageView(static_cast<ContentSettingsType>(i), this,
-                                    bubble_font_list, bubble_font_y_offset,
+                                    bubble_font_list,
+                                    kBubbleInteriorVerticalPadding,
                                     text_color, background_color);
     content_setting_views_.push_back(content_blocked_view);
     content_blocked_view->SetVisible(false);
@@ -804,8 +781,9 @@ void LocationBarView::Layout() {
       location_needed_width =
           std::min(location_needed_width,
                    location_bounds.width() - suggested_text_size.width());
-      gfx::Rect suggested_text_bounds(location_bounds.origin(),
-                                      suggested_text_size);
+      gfx::Rect suggested_text_bounds(location_bounds.x(), location_bounds.y(),
+                                      suggested_text_size.width(),
+                                      location_bounds.height());
       // TODO(sky): figure out why this needs the -1.
       suggested_text_bounds.Offset(location_needed_width - 1, 0);
       // For non-views the omnibox needs to be shrunk so that the suggest text
@@ -855,8 +833,7 @@ void LocationBarView::Layout() {
     location_bounds.set_width(x);
     ime_inline_autocomplete_view_->SetBounds(
         location_bounds.right(), location_bounds.y(),
-        std::min(width, entry_width),
-        ime_inline_autocomplete_view_->GetPreferredSize().height());
+        std::min(width, entry_width), location_bounds.height());
   }
 
   location_entry_view_->SetBoundsRect(location_bounds);