MD: Implement tab button for location bar's tab to search message

chrome/browser/ui/views/location_bar/keyword_hint_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/keyword_hint_view.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/search_engines/template_url_service.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/color_utils.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 
+namespace {
+
+class TabKeyImage : public views::View {
+ public:
+  TabKeyImage() {}

[Peter Kasting, 2015/10/29 03:48:43]

Nit: Avoid inlining these methods even for classes declared in a .cc file

Also I would probably suggest an explicit destructor if you're going to give an
explicit constructor.

+
+  // views::View implementation

[Peter Kasting, 2015/10/29 03:48:43]

Nit: Just "// views::View:"

+  gfx::Size GetPreferredSize() const override { return gfx::Size(34, 18); }

[Peter Kasting, 2015/10/29 03:48:43]

We definitely shouldn't hardcode the width, and we can avoid hardcoding the
height as well.  For at least the width, measure the size of the string you're
going to put inside, and then add some fixed amount of internal padding.  For
the height, either use this method, or size this based on the height of the
omnibox interior minus some amount of external padding.

Probably what you want is to fix the external and internal padding, derive the
correct font height in the constructor, measure the string once there, and then
store that size and return it as the preferred size.

I don't know whether the padding amounts should be the same (for one or both) as
for the EV/keyword search bubbles.

+
+  void OnPaint(gfx::Canvas* canvas) override {

[Peter Kasting, 2015/10/29 03:48:43]

It seems like you probably want to SaveAndUnscale() the canvas here, and scale
your drawings inside, so you can do things like have a 1 px border at all scale
sizes, right?

+    gfx::RectF round_rect_bounds(GetLocalBounds());
+    round_rect_bounds.Inset(0.5f, 0.5f);
+    SkScalar radius = SkIntToScalar(2);

[Peter Kasting, 2015/10/29 03:48:43]

Does this match the EV/keyword search bubble corners?  I know the omnibox as a
whole uses 2.5 pt corners in MD.

+    SkPaint paint;
+    paint.setAntiAlias(true);
+    paint.setStrokeWidth(1);
+    paint.setColor(
+        color_utils::DeriveDefaultIconColor(GetNativeTheme()->GetSystemColor(
+            ui::NativeTheme::kColorId_TextfieldDefaultColor)));
+    paint.setStyle(SkPaint::kStroke_Style);
+    canvas->sk_canvas()->drawRoundRect(gfx::RectFToSkRect(round_rect_bounds),
+                                       radius, radius, paint);
+
+    gfx::Rect text_bounds(GetLocalBounds());
+    const int kTopPadding = 3;
+    const int kBottomPadding = 2;
+    text_bounds.Inset(0, kTopPadding, 0, kBottomPadding);

[Peter Kasting, 2015/10/29 03:48:43]

I'm concerned about this implementation with different font faces that have
wildly different metrics, e.g. Hindi.

I think you probably want to just use a Label and let it auto-vertically-center
the ascent portion of the font within the entire View height, like what we
should be doing with the EV/keyword search bubbles.

Another option is that we could try to manually align the baseline of the word
"tab" in this image with that of the string around this ("Press ____ to search
XYZ").  Doing that might look better but will require a bit of careful plumbing.

+    canvas->DrawStringRectWithFlags(
+        l10n_util::GetStringUTF16(IDS_TAB_KEY),
+        gfx::FontList().DeriveWithHeightUpperBound(text_bounds.height()),
+        paint.getColor(), text_bounds, gfx::Canvas::TEXT_ALIGN_CENTER);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TabKeyImage);
+};
+
+// Creates and returns a view for depicting the tab key.
+views::View* CreateTabKeyImage() {

[Peter Kasting, 2015/10/29 03:48:43]

Nit: For clarity (that the caller is expected to take ownership), this should
probably return a scoped_ptr<View>.

+  if (ui::MaterialDesignController::IsModeMaterial())
+    return new TabKeyImage();
+
+  views::ImageView* tab = new views::ImageView();
+  tab->SetImage(ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+      IDR_OMNIBOX_KEYWORD_HINT_TAB));
+  return tab;
+}
+
+}  // namespace
 
 KeywordHintView::KeywordHintView(Profile* profile,
                                  const gfx::FontList& font_list,
                                  SkColor text_color,
                                  SkColor background_color)
-    : profile_(profile),
-      tab_image_(new views::ImageView()) {
+    : profile_(profile), tab_key_image_(CreateTabKeyImage()) {
   leading_label_ =
       CreateLabel(font_list, text_color, background_color);
-  tab_image_->SetImage(
-      ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
-          IDR_OMNIBOX_KEYWORD_HINT_TAB));
-  AddChildView(tab_image_);
+  AddChildView(tab_key_image_);
   trailing_label_ =
       CreateLabel(font_list, text_color, background_color);
 }
 
 KeywordHintView::~KeywordHintView() {
 }
 
 void KeywordHintView::SetKeyword(const base::string16& keyword) {
   keyword_ = keyword;
   if (keyword_.empty())
@@ skipping 14 matching lines @@
       message_id, base::string16(), short_name, &content_param_offsets);
   DCHECK_EQ(2U, content_param_offsets.size());
   leading_label_->SetText(
       keyword_hint.substr(0, content_param_offsets.front()));
   trailing_label_->SetText(keyword_hint.substr(content_param_offsets.front()));
 }
 
 gfx::Size KeywordHintView::GetPreferredSize() const {
   // Height will be ignored by the LocationBarView.
   return gfx::Size(leading_label_->GetPreferredSize().width() +
-                       tab_image_->GetPreferredSize().width() +
+                       tab_key_image_->GetPreferredSize().width() +
                        trailing_label_->GetPreferredSize().width(),
                    0);
 }
 
 gfx::Size KeywordHintView::GetMinimumSize() const {
   // Height will be ignored by the LocationBarView.
-  return tab_image_->GetPreferredSize();
+  return tab_key_image_->GetPreferredSize();
 }
 
 void KeywordHintView::Layout() {
-  int tab_width = tab_image_->GetPreferredSize().width();
-  bool show_labels = (width() != tab_width);
+  gfx::Size tab_size = tab_key_image_->GetPreferredSize();
+  bool show_labels = (width() != tab_size.width());
   gfx::Size leading_size(leading_label_->GetPreferredSize());
   leading_label_->SetBounds(0, 0, show_labels ? leading_size.width() : 0,
                             height());
-  tab_image_->SetBounds(leading_label_->bounds().right(), 0, tab_width,
-                        height());
+  tab_key_image_->SetBounds(leading_label_->bounds().right(),
+                            (height() - tab_size.height()) / 2,
+                            tab_size.width(), tab_size.height());
   gfx::Size trailing_size(trailing_label_->GetPreferredSize());
-  trailing_label_->SetBounds(tab_image_->bounds().right(), 0,
-                             show_labels ? trailing_size.width() : 0,
-                             height());
+  trailing_label_->SetBounds(tab_key_image_->bounds().right(), 0,
+                             show_labels ? trailing_size.width() : 0, height());
 }
 
 const char* KeywordHintView::GetClassName() const {
   return "KeywordHintView";
 }
 
 views::Label* KeywordHintView::CreateLabel(const gfx::FontList& font_list,
                                            SkColor text_color,
                                            SkColor background_color) {
   views::Label* label = new views::Label(base::string16(), font_list);
   label->SetEnabledColor(text_color);
   label->SetBackgroundColor(background_color);
   AddChildView(label);
   return label;
 }