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

components/omnibox/browser/omnibox_edit_model.h

 // Copyright 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.
 
 #ifndef COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_EDIT_MODEL_H_
 #define COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_EDIT_MODEL_H_
 
 #include <stddef.h>
 
 #include "base/compiler_specific.h"
@@ skipping 18 matching lines @@
 class OmniboxPopupModel;
 class OmniboxView;
 
 namespace gfx {
 class Image;
 class Rect;
 }
 
 // Reasons why the Omnibox could change into keyword mode.
 // These numeric values are used in UMA logs; do not change them.
 enum EnteredKeywordModeMethod {

[Peter Kasting, 2016/04/08 00:39:56]

Nit: I suggest changing this to an enum class, renaming it
KeywordModeEntryMethod, and eliminating the "ENTERED_KEYWORD_MODE_VIA_" on all
the values.

Then you can use |keyword_mode_entry_method| for variable/param names to match
the enum name.

[Peter Kasting, 2016/04/13 02:52:16]

Still applicable.

   ENTERED_KEYWORD_MODE_VIA_TAB = 0,
   ENTERED_KEYWORD_MODE_VIA_SPACE_AT_END = 1,
   ENTERED_KEYWORD_MODE_VIA_SPACE_IN_MIDDLE = 2,
+  ENTERED_KEYWORD_MODE_VIA_KEYBOARD_SHORTCUT = 3,
+  ENTERED_KEYWORD_MODE_VIA_QUESTION_MARK = 4,
   ENTERED_KEYWORD_MODE_NUM_ITEMS
 };
 
 class OmniboxEditModel {
  public:
   // Did the Omnibox focus originate via the user clicking on the Omnibox or on
   // the Fakebox?
   enum FocusSource {
     INVALID = 0,
     OMNIBOX = 1,
     FAKEBOX = 2
   };
 
   struct State {
     State(bool user_input_in_progress,
           const base::string16& user_text,
           const base::string16& gray_text,
           const base::string16& keyword,
           bool is_keyword_hint,
+          EnteredKeywordModeMethod keyword_entered_method,
           bool url_replacement_enabled,
           OmniboxFocusState focus_state,
           FocusSource focus_source,
           const AutocompleteInput& autocomplete_input);
     State(const State& other);
     ~State();
 
     bool user_input_in_progress;
     const base::string16 user_text;
     const base::string16 gray_text;
     const base::string16 keyword;
     const bool is_keyword_hint;
+    EnteredKeywordModeMethod keyword_entered_method;
     bool url_replacement_enabled;
     OmniboxFocusState focus_state;
     FocusSource focus_source;
     const AutocompleteInput autocomplete_input;
   };
 
   OmniboxEditModel(OmniboxView* view,
                    OmniboxEditController* controller,
                    scoped_ptr<OmniboxClient> client);
   virtual ~OmniboxEditModel();
@@ skipping 145 matching lines @@
 
   // Accessors for keyword-related state (see comments on keyword_ and
   // is_keyword_hint_).
   const base::string16& keyword() const { return keyword_; }
   bool is_keyword_hint() const { return is_keyword_hint_; }
   bool is_keyword_selected() const {
     return !is_keyword_hint_ && !keyword_.empty();
   }
 
   // Accepts the current keyword hint as a keyword. It always returns true for
-  // caller convenience. |entered_method| indicates how the use entered
-  // keyword mode. This parameter is only used for metrics/logging; it's not
-  // used to change user-visible behavior.
+  // caller convenience. |entered_method| indicates how the user entered
+  // keyword mode.
   bool AcceptKeyword(EnteredKeywordModeMethod entered_method);
 
+  // Set the current keyword to that of the user's default search provider and

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Function comments should be declarative, not imperative.

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+  // update the view so the user sees "Search Google:" in the omnibox.

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Don't say "Search Google" since that's not accurate.  Instead say something
like "...so the user sees the keyword chip in the omnibox."

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+  void SetKeywordWithDefaultSearchProvider(

[Peter Kasting, 2016/04/08 00:39:56]

Nit: Maybe "EnterKeywordModeForDefaultSearchProvider()"?

[Tom (Use chromium acct), 2016/04/12 20:03:05]

Done.

+      EnteredKeywordModeMethod keyword_entered_method);
+
   // Accepts the current temporary text as the user text.
   void AcceptTemporaryTextAsUserText();
 
   // Clears the current keyword.
   void ClearKeyword();
 
   // Returns the current autocomplete result.  This logic should in the future
   // live in AutocompleteController but resides here for now.  This method is
   // used by AutomationProvider::AutocompleteEditGetMatches.
   const AutocompleteResult& result() const {
@@ skipping 74 matching lines @@
   //
   // If |allow_keyword_ui_change| is false then the change should not affect
   // keyword ui state, even if the text matches a keyword exactly. This value
   // may be false when the user is composing a text with an IME.
   bool OnAfterPossibleChange(const base::string16& old_text,
                              const base::string16& new_text,
                              size_t selection_start,
                              size_t selection_end,
                              bool selection_differs,
                              bool text_differs,
+                             bool keyword_differs,
                              bool just_deleted_text,
                              bool allow_keyword_ui_change);
 
   // Called when the current match has changed in the OmniboxController.
   void OnCurrentMatchChanged();
 
   // Name of the histogram tracking cut or copy omnibox commands.
   static const char kCutOrCopyAllTextHistogram[];
 
  private:
@@ skipping 153 matching lines @@
   // Inline autocomplete is allowed if the user has not just deleted text, and
   // no temporary text is showing.  In this case, inline_autocomplete_text_ is
   // appended to the user_text_ and displayed selected (at least initially).
   //
   // NOTE: When the popup is closed there should never be inline autocomplete
   // text (actions that close the popup should either accept the text, convert
   // it to a normal selection, or change the edit entirely).
   bool just_deleted_text_;
   base::string16 inline_autocomplete_text_;
 
+  // If the user types '?' to enter keyword mode, and then presses backspace,
+  // we want a '?' to be displayed on the omnibox.  Without this variable, the
+  // user would immediately be transitioned back into keyword mode.

[Peter Kasting, 2016/04/13 02:52:16]

Nit: Maybe this.  I renamed the variable to match how you seem to use it, but
see comment in the .cc file also.

  // Set when the user is in the process of exiting keyword mode.  This is
  // useful to ensure we don't toggle back into keyword mode in the process of
  // handling the current event.
  bool clearing_keyword_;

[Tom (Use chromium acct), 2016/04/13 23:37:40]

Done.

+  bool just_cleared_keyword_;
+
   // Used by OnPopupDataChanged to keep track of whether there is currently a
   // temporary text.
   //
   // Example of use: If the user types "goog", then arrows down in the
   // autocomplete popup until, say, "google.com" appears in the edit box, then
   // the user_text_ is still "goog", and "google.com" is "temporary text".
   // When the user hits <esc>, the edit box reverts to "goog".  Hit <esc> again
   // and the popup is closed and "goog" is replaced by the permanent_text_,
   // which is the URL of the current page.
   //
@@ skipping 18 matching lines @@
   // selected keyword, or just some input text that looks like a keyword (so we
   // can show a hint to press <tab>).  This is the keyword in either case;
   // is_keyword_hint_ (below) distinguishes the two cases.
   base::string16 keyword_;
 
   // True if the keyword associated with this match is merely a hint, i.e. the
   // user hasn't actually selected a keyword yet.  When this is true, we can use
   // keyword_ to show a "Press <tab> to search" sort of hint.
   bool is_keyword_hint_;
 
+  // Indicates how the user entered keyword mode if the user is actually in
+  // keyword mode.  Otherwise, the value of this variable is undefined.  This
+  // is used to restore the user's search terms upon a call to ClearKeyword().
+  EnteredKeywordModeMethod keyword_entered_method_;
+
   // This is needed to properly update the SearchModel state when the user
   // presses escape.
   bool in_revert_;
 
   // Indicates if the upcoming autocomplete search is allowed to be treated as
   // an exact keyword match.  If this is true then keyword mode will be
   // triggered automatically if the input is "<keyword> <search string>".  We
   // allow this when CreatedKeywordSearchByInsertingSpaceInMiddle() is true.
   // This has no effect if we're already in keyword mode.
   bool allow_exact_keyword_match_;
 
   // The input that was sent to the AutocompleteController. Since no
   // autocomplete query is started after a tab switch, it is possible for this
   // |input_| to differ from the one currently stored in AutocompleteController.
   AutocompleteInput input_;
 
   DISALLOW_COPY_AND_ASSIGN(OmniboxEditModel);
 };
 
 #endif  // COMPONENTS_OMNIBOX_BROWSER_OMNIBOX_EDIT_MODEL_H_