Update Location Bar Bubbles Layout for Material Design

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 02c1de32f88b077a8ad2ffd5585e8fdcceed6496..78d3cb46a45330f33da01990b3702aefe26a6093 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -215,17 +215,26 @@ void LocationBarView::Init() {
   // Determine the font for use inside the bubbles.  The bubble background
   // images have 1 px thick edges, which we don't want to overlap.
   const int kBubbleInteriorVerticalPadding = 1;
-  const int bubble_vertical_padding =
+  const int bubble_height =
+      GetPreferredSize().height() -
       (GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING) +
-          kBubbleInteriorVerticalPadding) * 2;
-  const gfx::FontList bubble_font_list(font_list.DeriveWithHeightUpperBound(
-      location_height - bubble_vertical_padding));
+       kBubbleInteriorVerticalPadding + GetVerticalEdgeThickness()) *
+          2;

[Peter Kasting, 2015/10/05 21:53:14]

It's not really clear to me why you moved away from the previous calculation.

I'm also not sure whether this height includes or excludes the 1 px border of
the bubble itself.  I assume excludes based on your usage below?

[jonross, 2015/10/06 18:00:14]

Before MD bubbles were inset further than other items in the location bar.
During the switch to MD layout this was leading to needing a negative inset,
which we rejected.

Bubble padding was separated from the rest of padding, and bubble height was
calculated separately from GetInternalHeight which already adds the default
padding.

However this spot was missed.

bubble_height here does incorporate the 1 pixel border of the bubble in order to
determine the size available for the actual text.

[Peter Kasting, 2015/10/06 19:15:41]

OK.  Just make sure that the lock in a non-EV case (e.g.
https://codereview.chromium.org/ ) is positioned at the same place as the lock
inside the EV bubble (e.g. https://paypal.com/ ).  We used to have problems a
long time ago where when the EV or keyword search bubbles appeared, the icons in
them no longer aligned horizontally with the icons in other cases (e.g. the
omnibox dropdown).

Nit: I might order the terms in this expression differently so they went
logically from top to bottom:

  const int bubble_padding =
      GetVerticalEdgeThickness() +
      GetLayoutConstant(LOCATION_BAR_BUBBLE_VERTICAL_PADDING) +
      kBubbleInteriorVerticalPadding;
  const int bubble_content_height =
      GetPreferredSize().height() - (bubble_padding * 2);

+  const int desired_bubble_font_size =
+      GetLayoutConstant(LOCATION_BAR_BUBBLE_FONT_PIXEL_SIZE);

[Peter Kasting, 2015/10/05 21:53:14]

I still think we should eliminate this and just let the autosizing code pick the
font size.  Sebastien didn't really speak to this question in depth, so if he
feels strongly about the 11 px size I'd like to understand why; perhaps there's
a way to encode his preferences in the code in a way that doesn't hardcode the
font size.

[jonross, 2015/10/06 18:00:14]

I agree with removing this. Current calculations result in 12px size. If this is
deemed undesirable I'd recommend that we change the specification for the
bubbles internal padding. 

+  gfx::FontList bubble_font_list = font_list;
+  if (bubble_font_list.GetFontSize() != desired_bubble_font_size) {
+    bubble_font_list = bubble_font_list.DeriveWithSizeDelta(
+        desired_bubble_font_size - bubble_font_list.GetFontSize());
+  }
+  bubble_font_list = bubble_font_list.DeriveWithHeightUpperBound(bubble_height);
 
   const SkColor background_color =
       GetColor(SecurityStateModel::NONE, LocationBarView::BACKGROUND);
-  ev_bubble_view_ = new EVBubbleView(
-      bubble_font_list, GetColor(SecurityStateModel::EV_SECURE, SECURITY_TEXT),
-      background_color, this);
+  const SkColor ev_text_color =
+      GetColor(SecurityStateModel::EV_SECURE, SECURITY_TEXT);
+  ev_bubble_view_ =
+      new EVBubbleView(bubble_font_list, ev_text_color, background_color, this);
   ev_bubble_view_->set_drag_controller(this);
   AddChildView(ev_bubble_view_);
 
@@ -250,9 +259,11 @@ void LocationBarView::Init() {
   ime_inline_autocomplete_view_->SetVisible(false);
   AddChildView(ime_inline_autocomplete_view_);
 
-  const SkColor text_color = GetColor(SecurityStateModel::NONE, TEXT);
+  const SkColor selected_text_color = GetColor(
+      SecurityStateModel::NONE,
+      ui::MaterialDesignController::IsModeMaterial() ? BUBBLE_TEXT : TEXT);
   selected_keyword_view_ = new SelectedKeywordView(
-      bubble_font_list, text_color, background_color, profile());
+      bubble_font_list, selected_text_color, background_color, profile());
   AddChildView(selected_keyword_view_);
 
   suggested_text_view_ = new views::Label(base::string16(), font_list);
@@ -283,6 +294,7 @@ void LocationBarView::Init() {
   mic_search_view_->SetVisible(false);
   AddChildView(mic_search_view_);
 
+  const SkColor text_color = GetColor(SecurityStateModel::NONE, TEXT);
   for (ContentSettingsType type :
        ContentSettingBubbleModel::GetSupportedBubbleTypes()) {
     ContentSettingImageView* content_blocked_view = new ContentSettingImageView(
@@ -348,7 +360,10 @@ SkColor LocationBarView::GetColor(
       switch (security_level) {
         case SecurityStateModel::EV_SECURE:
         case SecurityStateModel::SECURE:
-          color = SkColorSetRGB(7, 149, 0);
+          if (ui::MaterialDesignController::IsModeMaterial())
+            color = SkColorSetRGB(11, 128, 67);
+          else
+            color = SkColorSetRGB(7, 149, 0);
           break;
 
         case SecurityStateModel::SECURITY_POLICY_WARNING:
@@ -371,6 +386,9 @@ SkColor LocationBarView::GetColor(
           color, GetColor(security_level, BACKGROUND));
     }
 
+    case BUBBLE_TEXT:

[Peter Kasting, 2015/10/05 21:53:14]

If this is now only used for the search chit, we should probably name it more
specifically, e.g. "KEYWORD_SEARCH_TEXT".

[jonross, 2015/10/06 18:00:14]

Done.

+      return SkColorSetRGB(51, 103, 214);
+
     default:
       NOTREACHED();
       return GetColor(security_level, TEXT);
@@ -860,6 +878,9 @@ int LocationBarView::GetHorizontalEdgeThickness() const {
 }
 
 int LocationBarView::GetVerticalEdgeThickness() const {
+  // In Material Design horizontal layout disregards the border.

[Peter Kasting, 2015/10/05 21:53:13]

Nit: Remove "horizontal"?

[jonross, 2015/10/06 18:00:14]

Done.

+  if (ui::MaterialDesignController::IsModeMaterial())
+    return 0;

[Peter Kasting, 2015/10/05 21:53:14]

This will change the following besides the bubble calculations:

* The rect we fill with the background color in popup mode
* The round rect we fill with the background color in normal mode
* The location of things like the icon at the front and the star at the end of
the location bar

Make sure all these have been modified in a way that makes them more correct in
MD mode.

[jonross, 2015/10/06 18:00:13]

This improves the background color for both modes. There was a gap introduced
with the shift to programmatic rendering of the border. I want to do a follow-up
where I adjust the rounded corners of the background to match the md-corners.

The other icons at the beginning/end of the location bar make use of a different
padding value now. Which already incorporated the border thickness.

   return is_popup_mode_ ? kPopupEdgeThickness : kNormalEdgeThickness;
 }