Update Location Bar Bubbles Layout for Material Design

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/i18n/rtl.h"
@@ skipping 197 matching lines @@
         font_list.DeriveWithSizeDelta(desired_font_size - current_font_size);
   }
   // Shrink large fonts to make them fit.
   // TODO(pkasting): Stretch the location bar instead in this case.
   const int location_height = GetInternalHeight(true);
   font_list = font_list.DeriveWithHeightUpperBound(location_height);
 
   // 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_);
 
   // Initialize the Omnibox view.
   omnibox_view_ = new OmniboxViewViews(
       this, profile(), command_updater(), is_popup_mode_, this, font_list);
   omnibox_view_->Init();
   omnibox_view_->SetFocusable(true);
   AddChildView(omnibox_view_);
 
   // 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(base::string16(), font_list);
   ime_inline_autocomplete_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   ime_inline_autocomplete_view_->SetAutoColorReadabilityEnabled(false);
   ime_inline_autocomplete_view_->set_background(
       views::Background::CreateSolidBackground(GetNativeTheme()->GetSystemColor(
           ui::NativeTheme::kColorId_TextfieldSelectionBackgroundFocused)));
   ime_inline_autocomplete_view_->SetEnabledColor(
       GetNativeTheme()->GetSystemColor(
           ui::NativeTheme::kColorId_TextfieldSelectionColor));
   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);
   suggested_text_view_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   suggested_text_view_->SetAutoColorReadabilityEnabled(false);
   suggested_text_view_->SetEnabledColor(
       GetColor(SecurityStateModel::NONE, LocationBarView::DEEMPHASIZED_TEXT));
   suggested_text_view_->SetVisible(false);
   AddChildView(suggested_text_view_);
 
@@ skipping 10 matching lines @@
       l10n_util::GetStringUTF16(IDS_TOOLTIP_MIC_SEARCH));
   mic_search_view_->SetImage(
       views::Button::STATE_NORMAL,
       ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
           IDR_OMNIBOX_MIC_SEARCH));
   mic_search_view_->SetImageAlignment(views::ImageButton::ALIGN_CENTER,
                                       views::ImageButton::ALIGN_MIDDLE);
   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(
         type, this, bubble_font_list, text_color, background_color);
     content_setting_views_.push_back(content_blocked_view);
     content_blocked_view->SetVisible(false);
     AddChildView(content_blocked_view);
   }
 
   zoom_view_ = new ZoomView(delegate_);
@@ skipping 45 matching lines @@
       return color_utils::AlphaBlend(
           GetColor(security_level, TEXT),
           GetColor(security_level, BACKGROUND),
           128);
 
     case SECURITY_TEXT: {
       SkColor color;
       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:
           return GetColor(security_level, DEEMPHASIZED_TEXT);
           break;
 
         case SecurityStateModel::SECURITY_ERROR:
           color = SkColorSetRGB(162, 0, 0);
           break;
 
         case SecurityStateModel::SECURITY_WARNING:
           return GetColor(security_level, TEXT);
           break;
 
         default:
           NOTREACHED();
           return GetColor(security_level, TEXT);
       }
       return color_utils::GetReadableColor(
           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);
   }
 }
 
 void LocationBarView::ZoomChangedForActiveTab(bool can_show_bubble) {
   DCHECK(zoom_view_);
   if (RefreshZoomView()) {
     Layout();
@@ skipping 469 matching lines @@
 }
 
 int LocationBarView::GetHorizontalEdgeThickness() const {
   // In maximized popup mode, there isn't any edge.
   return (is_popup_mode_ && browser_ && browser_->window() &&
           browser_->window()->IsMaximized()) ?
       0 : GetVerticalEdgeThickness();
 }
 
 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;
 }
 
 int LocationBarView::VerticalPadding() const {
   return is_popup_mode_ ?
       kPopupEdgeThickness : GetLayoutConstant(LOCATION_BAR_VERTICAL_PADDING);
 }
 
 bool LocationBarView::RefreshContentSettingViews() {
   bool visibility_changed = false;
@@ skipping 536 matching lines @@
 
 void LocationBarView::ModelChanged(const SearchModel::State& old_state,
                                    const SearchModel::State& new_state) {
   const bool visible = !GetToolbarModel()->input_in_progress() &&
       new_state.voice_search_supported;
   if (mic_search_view_->visible() != visible) {
     mic_search_view_->SetVisible(visible);
     Layout();
   }
 }