[omnibox] Get keyword search working on the button row

This CL triggers keyword search mode when the keyword button is pressed
on the button row. The button label is taken from SelectedKeywordView,
which is refactored slightly to expose internationalized label names.
The refactor also eliminates persistent member |profile_| because it is
only used in one place where a parameter suffices.

Bug: 1046523
Change-Id: Ifb0c0d14059cfb4d866b04d2c63cb97796713024
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2033907
Commit-Queue: Orin Jaworski <orinj@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737933}

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

@@ -213,8 +213,8 @@ void LocationBarView::Init() {
   ime_inline_autocomplete_view_ =
       AddChildView(std::move(ime_inline_autocomplete_view));
 
-  selected_keyword_view_ = AddChildView(
-      std::make_unique<SelectedKeywordView>(this, font_list, profile_));
+  selected_keyword_view_ =
+      AddChildView(std::make_unique<SelectedKeywordView>(this, font_list));
 
   keyword_hint_view_ =
       AddChildView(std::make_unique<KeywordHintView>(this, profile_));
@@ -511,7 +511,7 @@ void LocationBarView::Layout() {
     // Call this even if keyword doesn't change. Let the View decide what to do
     // about a11y.
     bool keyword_changed = selected_keyword_view_->keyword() != keyword;
-    selected_keyword_view_->SetKeyword(keyword);
+    selected_keyword_view_->SetKeyword(keyword, profile_);
     if (keyword_changed) {
       const TemplateURL* template_url =
           TemplateURLServiceFactory::GetForProfile(profile_)

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

@@ -19,12 +19,27 @@
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
 
+// static
+SelectedKeywordView::KeywordLabelNames
+SelectedKeywordView::GetKeywordLabelNames(const base::string16& keyword,
+                                          TemplateURLService* service) {
+  KeywordLabelNames names;
+  if (service) {
+    bool is_extension_keyword = false;
+    names.short_name =
+        service->GetKeywordShortName(keyword, &is_extension_keyword);
+    names.full_name = is_extension_keyword
+                          ? names.short_name
+                          : l10n_util::GetStringFUTF16(
+                                IDS_OMNIBOX_KEYWORD_TEXT_MD, names.short_name);
+  }
+  return names;
+}
+
 SelectedKeywordView::SelectedKeywordView(LocationBarView* location_bar,
-                                         const gfx::FontList& font_list,
-                                         Profile* profile)
+                                         const gfx::FontList& font_list)
     : IconLabelBubbleView(font_list, location_bar),
-      location_bar_(location_bar),
-      profile_(profile) {
+      location_bar_(location_bar) {
   full_label_.SetFontList(font_list);
   full_label_.SetVisible(false);
   partial_label_.SetFontList(font_list);
@@ -69,26 +84,22 @@ void SelectedKeywordView::OnThemeChanged() {
     SetCustomImage(gfx::Image());
 }
 
-void SelectedKeywordView::SetKeyword(const base::string16& keyword) {
+void SelectedKeywordView::SetKeyword(const base::string16& keyword,
+                                     Profile* profile) {
   if (keyword_ != keyword) {
     keyword_ = keyword;
     if (keyword.empty())
       return;
-    DCHECK(profile_);
-    TemplateURLService* model =
-        TemplateURLServiceFactory::GetForProfile(profile_);
-    if (!model)
+
+    DCHECK(profile);
+    TemplateURLService* service =
+        TemplateURLServiceFactory::GetForProfile(profile);
+    if (!service)
       return;
 
-    bool is_extension_keyword;
-    const base::string16 short_name =
-        model->GetKeywordShortName(keyword, &is_extension_keyword);
-    const base::string16 full_name =
-        is_extension_keyword ? short_name
-                             : l10n_util::GetStringFUTF16(
-                                   IDS_OMNIBOX_KEYWORD_TEXT_MD, short_name);
-    full_label_.SetText(full_name);
-    partial_label_.SetText(short_name);
+    KeywordLabelNames names = GetKeywordLabelNames(keyword, service);
+    full_label_.SetText(names.full_name);
+    partial_label_.SetText(names.short_name);
 
     // Update the label now so ShouldShowLabel() works correctly when the parent
     // class is calculating the preferred size. It will be updated again in

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

@@ -15,6 +15,7 @@
 
 class LocationBarView;
 class Profile;
+class TemplateURLService;
 
 namespace gfx {
 class FontList;
@@ -24,9 +25,19 @@ class Size;
 // SelectedKeywordView displays the tab-to-search UI in the location bar view.
 class SelectedKeywordView : public IconLabelBubbleView {
  public:
+  struct KeywordLabelNames {
+    base::string16 short_name;
+    base::string16 full_name;
+  };
+  // Returns the short and long names that can be used to describe keyword
+  // behavior, e.g. "Search google.com" or an equivalent translation, with
+  // consideration for bidirectional text safety using |service|. Empty
+  // names are returned if service is null.
+  static KeywordLabelNames GetKeywordLabelNames(const base::string16& keyword,
+                                                TemplateURLService* service);
+
   SelectedKeywordView(LocationBarView* location_bar,
-                      const gfx::FontList& font_list,
-                      Profile* profile);
+                      const gfx::FontList& font_list);
   ~SelectedKeywordView() override;
 
   // Sets the icon for this chip to |image|.  If there is no custom image (i.e.
@@ -40,7 +51,7 @@ class SelectedKeywordView : public IconLabelBubbleView {
   SkColor GetForegroundColor() const override;
 
   // The current keyword, or an empty string if no keyword is displayed.
-  void SetKeyword(const base::string16& keyword);
+  void SetKeyword(const base::string16& keyword, Profile* profile);
   const base::string16& keyword() const { return keyword_; }
 
   using IconLabelBubbleView::label;
@@ -67,8 +78,6 @@ class SelectedKeywordView : public IconLabelBubbleView {
   views::Label full_label_;
   views::Label partial_label_;
 
-  Profile* profile_;
-
   // True when the chip icon has been changed via SetCustomImage().
   bool using_custom_image_ = false;
 

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

@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/omnibox/omnibox_theme.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
+#include "chrome/browser/ui/views/location_bar/selected_keyword_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_match_cell_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_popup_contents_view.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_tab_switch_button.h"
@@ -29,6 +30,7 @@
 #include "components/omnibox/common/omnibox_features.h"
 #include "components/strings/grit/components_strings.h"
 #include "components/vector_icons/vector_icons.h"
+#include "third_party/metrics_proto/omnibox_event.pb.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
@@ -152,8 +154,15 @@ void OmniboxResultView::SetMatch(const AutocompleteMatch& match) {
         match_.description, match_.description_class, deemphasize);
   }
 
-  // When button row feature is enabled, |keyword_button_| is used instead.
-  if (!OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) {
+  // With button row, |keyword_button_| is used instead of |keyword_view_|.
+  if (OmniboxFieldTrial::IsSuggestionButtonRowEnabled()) {
+    const OmniboxEditModel* edit_model =
+        popup_contents_view_->model()->edit_model();
+    const base::string16& keyword = edit_model->keyword();
+    const auto names = SelectedKeywordView::GetKeywordLabelNames(
+        keyword, edit_model->client()->GetTemplateURLService());
+    keyword_button_->SetText(names.full_name);
+  } else {
     AutocompleteMatch* keyword_match = match_.associated_keyword.get();
     keyword_view_->SetVisible(keyword_match != nullptr);
     if (keyword_match) {
@@ -326,7 +335,6 @@ void OmniboxResultView::SetRichSuggestionImage(const gfx::ImageSkia& image) {
 ////////////////////////////////////////////////////////////////////////////////
 // views::ButtonListener overrides:
 
-// |button| is the tab switch button.
 void OmniboxResultView::ButtonPressed(views::Button* button,
                                       const ui::Event& event) {
   if (button == suggestion_tab_switch_button_ || button == tab_switch_button_) {
@@ -358,7 +366,22 @@ void OmniboxResultView::ButtonPressed(views::Button* button,
 
     popup_contents_view_->model()->set_popup_closes_on_blur(true);
   } else if (button == keyword_button_) {
-    // TODO(orinj): Implement.
+    // TODO(orinj): Clear out existing suggestions, particularly this one, as
+    // once we AcceptKeyword, we are really in a new scope state and holding
+    // onto old suggestions is confusing and error prone. Without this check,
+    // a second click of the button violates assumptions in |AcceptKeyword|.
+    if (popup_contents_view_->model()->edit_model()->is_keyword_hint()) {
+      auto method = metrics::OmniboxEventProto::INVALID;
+      if (event.IsKeyEvent()) {
+        method = metrics::OmniboxEventProto::KEYBOARD_SHORTCUT;
+      } else if (event.IsMouseEvent()) {
+        method = metrics::OmniboxEventProto::CLICK_HINT_VIEW;
+      } else if (event.IsGestureEvent()) {
+        method = metrics::OmniboxEventProto::TAP_HINT_VIEW;
+      }
+      DCHECK_NE(method, metrics::OmniboxEventProto::INVALID);
+      popup_contents_view_->model()->edit_model()->AcceptKeyword(method);
+    }
   } else if (button == pedal_button_) {
     DCHECK(match_.pedal);
     // Pedal action intent means we execute the match instead of opening it.