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

components/omnibox/browser/omnibox_view.h

Index: components/omnibox/browser/omnibox_view.h
diff --git a/components/omnibox/browser/omnibox_view.h b/components/omnibox/browser/omnibox_view.h
index 462070f53bbace1d8ba5d7db571c993bc5c9d973..4f335fbb8a79ac32cf38a73fa5d1c6b65f35cb9f 100644
--- a/components/omnibox/browser/omnibox_view.h
+++ b/components/omnibox/browser/omnibox_view.h
@@ -95,14 +95,9 @@ class OmniboxView {
                                         bool update_popup,
                                         bool notify_text_changed) = 0;
 
-  // Sets the edit to forced query mode.  Practically speaking, this means that
-  // if the edit is not in forced query mode, its text is set to "?" with the
-  // cursor at the end, and if the edit is in forced query mode (its first
-  // non-whitespace character is '?'), the text after the '?' is selected.
-  //
-  // In the future we should display the search engine UI for the default engine
-  // rather than '?'.
-  virtual void SetForcedQuery() = 0;
+  // Transitions the user into keyword mode with their default search provider,
+  // preserving and selecting the user's text if they already typed in a query.
+  virtual void EnterKeywordModeForDefaultSearchProvider() = 0;
 
   // Returns true if all text is selected or there is no text at all.
   virtual bool IsSelectAll() const = 0;
@@ -239,9 +234,26 @@ class OmniboxView {
   static base::string16 SanitizeTextForPaste(const base::string16& text);
 
  protected:
+  // This state is needed by |On[Before|After]PossibleChange|.
+  struct TextState {

[Peter Kasting, 2016/06/04 02:17:18]

Nit: I would name and document this as follows:

  // Tracks important state that may change between OnBeforePossibleChange() and
  // OnAfterPossibleChange().
  struct State {

("TextState" seems inaccurate because the keyword isn't really part of the
"text".)

[Tom (Use chromium acct), 2016/06/04 20:39:09]

Done.

+    base::string16 text;
+    base::string16 keyword;
+    bool is_keyword_selected;
+    size_t sel_start;
+    size_t sel_end;
+  };
+
   OmniboxView(OmniboxEditController* controller,
               std::unique_ptr<OmniboxClient> client);
 
+  // Fills |state| with the current text state.
+  void GetTextState(TextState& state);

[Peter Kasting, 2016/06/04 02:17:18]

Nit: Arguments which the function modifies must be passed by pointer, not
non-const ref.

[Tom (Use chromium acct), 2016/06/04 20:39:09]

Done.

+
+  // Returns the delta between |before| and |after|.  The resulting
+  // |TextStateChange| is passed to |OmniboxEditModel::OnAfterPossibleChange|.

[Peter Kasting, 2016/06/04 02:17:18]

Nit: No || on type names.  Actually, assuming you follow my documentation
suggestion for the state change class (see comments elsewhere), I'd just remove
this second sentence, as those comments cover it better.

[Tom (Use chromium acct), 2016/06/04 20:39:10]

Done.

+  OmniboxEditModel::TextStateChange GetTextStateChange(const TextState& before,
+                                                       const TextState& after);
+
   // Internally invoked whenever the text changes in some way.
   virtual void TextChanged();