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

 // 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/omnibox/omnibox_view_views.h"
 
 #include <set>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 261 matching lines @@
   // TODO(oshima): IME support
   return text();
 }
 
 void OmniboxViewViews::SetUserText(const base::string16& text,
                                    bool update_popup) {
   saved_selection_for_focus_change_ = gfx::Range::InvalidRange();
   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(
     base::string16::size_type* start,
     base::string16::size_type* end) const {
   const gfx::Range range = GetSelectedRange();
   *start = static_cast<size_t>(range.start());
   *end = static_cast<size_t>(range.end());
 }
 
@@ skipping 115 matching lines @@
   }
 }
 
 bool OmniboxViewViews::HandleEarlyTabActions(const ui::KeyEvent& event) {
   // This must run before accelerator handling invokes a focus change on tab.
   // Note the parallel with SkipDefaultKeyEventProcessing above.
   if (!views::FocusManager::IsTabTraversalKeyEvent(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;
 
   if (event.IsShiftDown() &&
       (model()->popup_model()->selected_line_state() ==
           OmniboxPopupModel::KEYWORD))
     model()->ClearKeyword();
   else
     model()->OnUpOrDownKeyPressed(event.IsShiftDown() ? -1 : 1);
@@ skipping 85 matching lines @@
   SelectRange(saved_temporary_selection_);
   // We got here because the user hit the Escape key. We explicitly don't call
   // TextChanged(), since OmniboxPopupModel::ResetToDefaultMatch() has already
   // been called by now, and it would've called TextChanged() if it was
   // warranted.
 }
 
 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();
 }
 
 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));
 
   // When the user has deleted text, we don't allow inline autocomplete.  Make
   // sure to not flag cases like selecting part of the text and then pasting
   // (or typing) the prefix of that selection.  (We detect these by making
   // sure the caret, which should be after any insertion, hasn't moved
   // forward of the old selection start.)
   const bool just_deleted_text =
       (text_before_change_.length() > new_text.length()) &&
       (new_sel.start() <= sel_before_change_.GetMin());
 
   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();
   else if (delete_at_end_pressed_)
     model()->OnChanged();
 
   return something_changed;
 }
 
 gfx::NativeView OmniboxViewViews::GetNativeView() const {
@@ skipping 510 matching lines @@
     menu_contents->InsertItemWithStringIdAt(
         select_all_position + 1, IDS_SHOW_URL, IDS_SHOW_URL);
   }
 
   // Minor note: We use IDC_ for command id here while the underlying textfield
   // is using IDS_ for all its command ids. This is because views cannot depend
   // on IDC_ for now.
   menu_contents->AddItemWithStringId(IDC_EDIT_SEARCH_ENGINES,
       IDS_EDIT_SEARCH_ENGINES);
 }