Interpret '?' and Ctrl-K or Ctrl-E as putting omnibox in keyword search mode for Default Search Pro…

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

Index: chrome/browser/ui/views/omnibox/omnibox_view_views.cc
diff --git a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
index 8852dfe48c57321a8a6cbf13ad68624259255622..bae2143f14691b5bbfe6757ad76d205d67f2de8b 100644
--- a/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
+++ b/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
@@ -279,13 +279,12 @@ void OmniboxViewViews::SetUserText(const base::string16& text,
   OmniboxView::SetUserText(text, update_popup);
 }
 
-void OmniboxViewViews::SetForcedQuery() {
-  const base::string16 current_text(text());
-  const size_t start = current_text.find_first_not_of(base::kWhitespaceUTF16);
-  if (start == base::string16::npos || (current_text[start] != '?'))
-    OmniboxView::SetUserText(base::ASCIIToUTF16("?"));
-  else
-    SelectRange(gfx::Range(current_text.size(), start + 1));
+void OmniboxViewViews::EnterKeywordModeForDefaultSearchProvider() {
+  // Transition the user into keyword mode using their default search provider.
+  // Select their query if they typed one.
+  model()->EnterKeywordModeForDefaultSearchProvider(
+      KeywordModeEntryMethod::KEYBOARD_SHORTCUT);
+  SelectRange(gfx::Range(text().size(), 0));
 }
 
 void OmniboxViewViews::GetSelectionBounds(
@@ -421,7 +420,7 @@ bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
     return false;
 
   if (model()->is_keyword_hint() && !event.IsShiftDown())
-    return model()->AcceptKeyword(ENTERED_KEYWORD_MODE_VIA_TAB);
+    return model()->AcceptKeyword(KeywordModeEntryMethod::TAB);
 
   if (!model()->popup_model()->IsOpen())
     return false;
@@ -527,6 +526,8 @@ void OmniboxViewViews::OnRevertTemporaryText() {
 void OmniboxViewViews::OnBeforePossibleChange() {
   // Record our state.
   text_before_change_ = text();
+  keyword_before_change_ = model()->keyword();

[Peter Kasting, 2016/06/01 01:07:27]

This may be worth pulling out to a separate CL, but:

It seems like both here and in the Mac implementation, we have a whole bunch of
member variables, which we initialize with state in OnBeforePossibleChange(),
then we make the same state checks in OnAfterPossibleChange(), compute the
differences, and pass them to the edit model's OnAfterPossibleChange().

For one, this is a lot of duplicated code between Views and Mac that I wonder if
we can hoist to somewhere in the cross-platform parent class.

For another (and this is rather orthogonal to the above), having a bunch of
separate members, and a bunch of separate args to
OmniboxEditModel::OnAfterPossibleChange(), is awkward.  And the latter issue
results in everywhere else that wants to call that function (e.g. tests) passing
a bunch of bools, which is not very readable and prone to error.  I wonder if we
should consider creating a couple of bits to help out here, e.g.:
* Some kind of helper function that can return either a struct or a tuple that
represents all the variables we need to track across a possible state change. 
Then this struct/tuple can replace all the separate member variables on this
class.
* A struct to contain all the "XYZ changed" flags, with a null constructor so
tests can easily just call OnAfterPossibleChange(..., StuffChangedStruct()), and
a constructor that takes two of the above structs/tuples (the "before" and
"after" states) and computes its flag values from those.  This latter
constructor can be used in the view's OnAfterPossibleChange().

[Tom (Use chromium acct), 2016/06/02 19:24:40]

Did both.  Here's to DRY

Unfortunately it's on this patch set.  However, I have a commit on my machine
that I can just cherry-pick and put in a different patch set if that's easier
for you.

+  is_keyword_selected_before_change_ = model()->is_keyword_selected();
   sel_before_change_ = GetSelectedRange();
   ime_composing_before_change_ = IsIMEComposing();
 }
@@ -534,9 +535,15 @@ void OmniboxViewViews::OnBeforePossibleChange() {
 bool OmniboxViewViews::OnAfterPossibleChange(bool allow_keyword_ui_change) {
   // See if the text or selection have changed since OnBeforePossibleChange().
   const base::string16 new_text = text();
+  const base::string16 new_keyword = model()->keyword();
+  const bool new_is_keyword_selected = model()->is_keyword_selected();
   const gfx::Range new_sel = GetSelectedRange();
   const bool text_changed = (new_text != text_before_change_) ||
       (ime_composing_before_change_ != IsIMEComposing());
+  const bool keyword_changed =
+      (new_is_keyword_selected != is_keyword_selected_before_change_) ||
+      (new_is_keyword_selected && is_keyword_selected_before_change_ &&
+       new_keyword != keyword_before_change_);
   const bool selection_differs =
       !((sel_before_change_.is_empty() && new_sel.is_empty()) ||
         sel_before_change_.EqualsIgnoringDirection(new_sel));
@@ -552,14 +559,14 @@ bool OmniboxViewViews::OnAfterPossibleChange(bool allow_keyword_ui_change) {
 
   const bool something_changed = model()->OnAfterPossibleChange(
       text_before_change_, new_text, new_sel.start(), new_sel.end(),
-      selection_differs, text_changed, just_deleted_text,
+      selection_differs, text_changed, keyword_changed, just_deleted_text,
       allow_keyword_ui_change && !IsIMEComposing());
 
   // If only selection was changed, we don't need to call model()'s
   // OnChanged() method, which is called in TextChanged().
   // But we still need to call EmphasizeURLComponents() to make sure the text
   // attributes are updated correctly.
-  if (something_changed && text_changed)
+  if (something_changed && (text_changed || keyword_changed))
     TextChanged();
   else if (selection_differs)
     EmphasizeURLComponents();