Fix keyword search erroneously not triggering in two obscure cases:

(1) User types something that doesn't look at all like a keyword (e.g. "go om") and gets a non-default dropdown result that does look like a keyword ("google.com").  Arrowing down to this result, the user then presses space after the keyword name.
(2) Similar to the case above, but this time the non-default result in question looks like a keyword + some more text, with no whitespace between ("google.comxxx").  The user arrows to this result, arrows back to just after the keyword, and presses space.

Item (2) was happening because ShouldAllowExactKeywordMatch() (which has now been renamed) was always looking at the old |user_text_| as the "before change" text, which was wrong if there was temporary text, since the user text was not visible at that point.  Fixed by getting the actual displayed text in that case.

Item (1) was happening for similar reasons, but in MaybeAcceptKeywordBySpace() instead.  This could have been fixed by the same change as above.  However, I elected to change this function to look at the |keyword_| instead of the previously-displayed text, as in https://chromiumcodereview.appspot.com/9289034/ patch set 1.  Not only is this slightly simpler, it is more robust against future changes.  Let's say that someday we want to allow a provider to show a keyword hint for keyword "foobar" on input of "foo ".  By checking against |keyword_| in this function, we ensure that in that future case, the space after "foo" will never trigger keyword search mode for "foobar".  (If we ever bother to fix our bugs with keywords containing spaces, this might apply today for those as well.)

I also tried to change the naming and comments of some things for clarity.

BUG=none
TEST=none
Review URL: https://chromiumcodereview.appspot.com/9570064

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@124745 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/autocomplete/autocomplete_edit.cc

@@ -832,6 +832,7 @@ void AutocompleteEditModel::OnPopupDataChanged(
 }
 
 bool AutocompleteEditModel::OnAfterPossibleChange(
+    const string16& old_text,
     const string16& new_text,
     size_t selection_start,
     size_t selection_end,
@@ -863,7 +864,6 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
     return false;
   }
 
-  const string16 old_user_text = user_text_;
   // If the user text has not changed, we do not want to change the model's
   // state associated with the text.  Otherwise, we can get surprising behavior
   // where the autocompleted text unexpectedly reappears, e.g. crbug.com/55983
@@ -881,21 +881,29 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
   // Update the popup for the change, in the process changing to keyword mode
   // if the user hit space in mid-string after a keyword.
   // |allow_exact_keyword_match_| will be used by StartAutocomplete() method,
-  // which will be called by |view_->UpdatePopup()|. So we can safely clear
-  // this flag afterwards.
-  allow_exact_keyword_match_ =
-      text_differs && allow_keyword_ui_change &&
+  // which will be called by |view_->UpdatePopup()|; so after that returns we
+  // can safely reset this flag.
+  allow_exact_keyword_match_ = text_differs && allow_keyword_ui_change &&
       !just_deleted_text && no_selection &&
-      ShouldAllowExactKeywordMatch(old_user_text, user_text_, selection_start);
+      CreatedKeywordSearchByInsertingSpaceInMiddle(old_text, user_text_,
+                                                   selection_start);
   view_->UpdatePopup();
   allow_exact_keyword_match_ = false;
 
-  // Change to keyword mode if the user has typed a keyword name and is now
-  // pressing space after the name. Accepting the keyword will update our
-  // state, so in that case there's no need to also return true here.
+  // Change to keyword mode if the user is now pressing space after a keyword
+  // name.  Note that if this is the case, then even if there was no keyword
+  // hint when we entered this function (e.g. if the user has used space to
+  // replace some selected text that was adjoined to this keyword), there will
+  // be one now because of the call to UpdatePopup() above; so it's safe for
+  // MaybeAcceptKeywordBySpace() to look at |keyword_| and |is_keyword_hint_| to
+  // determine what keyword, if any, is applicable.
+  //
+  // If MaybeAcceptKeywordBySpace() accepts the keyword and returns true, that
+  // will have updated our state already, so in that case we don't also return
+  // true from this function.
   return !(text_differs && allow_keyword_ui_change && !just_deleted_text &&
-           no_selection && selection_start == user_text_.length() &&
-           MaybeAcceptKeywordBySpace(old_user_text, user_text_));
+           no_selection && (selection_start == user_text_.length()) &&
+           MaybeAcceptKeywordBySpace(user_text_));
 }
 
 void AutocompleteEditModel::PopupBoundsChangedTo(const gfx::Rect& bounds) {
@@ -1030,44 +1038,41 @@ void AutocompleteEditModel::RevertTemporaryText(bool revert_popup) {
 }
 
 bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
-    const string16& old_user_text,
-    const string16& new_user_text) {
+    const string16& new_text) {
+  size_t keyword_length = new_text.length() - 1;
   return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
-      inline_autocomplete_text_.empty() && new_user_text.length() >= 2 &&
-      IsSpaceCharForAcceptingKeyword(*new_user_text.rbegin()) &&
-      !IsWhitespace(*(new_user_text.rbegin() + 1)) &&
-      (old_user_text.length() + 1 >= new_user_text.length()) &&
-      !new_user_text.compare(0, new_user_text.length() - 1, old_user_text,
-                             0, new_user_text.length() - 1) &&
+      inline_autocomplete_text_.empty() &&
+      (keyword_.length() == keyword_length) &&
+      IsSpaceCharForAcceptingKeyword(new_text[keyword_length]) &&
+      !new_text.compare(0, keyword_length, keyword_, 0, keyword_length) &&
       AcceptKeyword();
 }
 
-bool AutocompleteEditModel::ShouldAllowExactKeywordMatch(
-    const string16& old_user_text,
-    const string16& new_user_text,
-    size_t caret_position) {
+bool AutocompleteEditModel::CreatedKeywordSearchByInsertingSpaceInMiddle(
+    const string16& old_text,
+    const string16& new_text,
+    size_t caret_position) const {
+  DCHECK_GE(new_text.length(), caret_position);
+
   // Check simple conditions first.
-  if (paste_state_ != NONE || caret_position < 2 ||
-      new_user_text.length() <= caret_position ||
-      old_user_text.length() < caret_position ||
-      !IsSpaceCharForAcceptingKeyword(new_user_text[caret_position - 1]) ||
-      IsSpaceCharForAcceptingKeyword(new_user_text[caret_position - 2]) ||
-      new_user_text.compare(0, caret_position - 1, old_user_text,
-                            0, caret_position - 1) ||
-      !new_user_text.compare(caret_position - 1,
-                             new_user_text.length() - caret_position + 1,
-                             old_user_text, caret_position - 1,
-                             old_user_text.length() - caret_position + 1)) {
+  if ((paste_state_ != NONE) || (caret_position < 2) ||
+      (old_text.length() < caret_position) ||
+      (new_text.length() == caret_position))
+    return false;
+  size_t space_position = caret_position - 1;
+  if (!IsSpaceCharForAcceptingKeyword(new_text[space_position]) ||
+      IsWhitespace(new_text[space_position - 1]) ||
+      new_text.compare(0, space_position, old_text, 0, space_position) ||
+      !new_text.compare(space_position, new_text.length() - space_position,
+                        old_text, space_position,
+                        old_text.length() - space_position)) {
     return false;
   }
 
   // Then check if the text before the inserted space matches a keyword.
   string16 keyword;
-  TrimWhitespace(new_user_text.substr(0, caret_position - 1),
-                 TRIM_LEADING, &keyword);
-
-  // Only allow exact keyword match if |keyword| represents a keyword hint.
-  return keyword.length() &&
+  TrimWhitespace(new_text.substr(0, space_position), TRIM_LEADING, &keyword);
+  return !keyword.empty() &&
       !autocomplete_controller_->keyword_provider()->
           GetKeywordForText(keyword).empty();
 }

chrome/browser/autocomplete/autocomplete_edit.h

@@ -319,11 +319,15 @@ class AutocompleteEditModel : public AutocompleteControllerDelegate {
 
   // Called by the OmniboxView after something changes, with details about what
   // state changes occured.  Updates internal state, updates the popup if
-  // necessary, and returns true if any significant changes occurred.
+  // necessary, and returns true if any significant changes occurred.  Note that
+  // |text_differs| may be set even if |old_text| == |new_text|, e.g. if we've
+  // just committed an IME composition.
+  //
   // 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 string16& new_text,
+  bool OnAfterPossibleChange(const string16& old_text,
+                             const string16& new_text,
                              size_t selection_start,
                              size_t selection_end,
                              bool selection_differs,
@@ -412,20 +416,20 @@ class AutocompleteEditModel : public AutocompleteControllerDelegate {
   // If |revert_popup| is true then the popup will be reverted as well.
   void RevertTemporaryText(bool revert_popup);
 
-  // Accepts current keyword if the user only typed a space at the end of
-  // |new_user_text| comparing to the |old_user_text|.
+  // Accepts current keyword if the user just typed a space at the end of
+  // |new_text|.  This handles both of the following cases:
+  //   (assume "foo" is a keyword, | is the input caret, [] is selected text)
+  //   foo| -> foo |      (a space was appended to a keyword)
+  //   foo[bar] -> foo |  (a space replaced other text after a keyword)
   // Returns true if the current keyword is accepted.
-  bool MaybeAcceptKeywordBySpace(const string16& old_user_text,
-                                 const string16& new_user_text);
-
-  // Checks if |allow_exact_keyword_match_| should be set to true according to
-  // the old and new user text and the current caret position. It does not take
-  // other factors into account, e.g. if the view is ready to change the keyword
-  // ui or not. This is only for the case of inserting a space character in the
-  // middle of the text. See the comment of |allow_exact_keyword_match_| below.
-  bool ShouldAllowExactKeywordMatch(const string16& old_user_text,
-                                    const string16& new_user_text,
-                                    size_t caret_position);
+  bool MaybeAcceptKeywordBySpace(const string16& new_text);
+
+  // Checks whether the user inserted a space into |old_text| and by doing so
+  // created a |new_text| that looks like "<keyword> <search phrase>".
+  bool CreatedKeywordSearchByInsertingSpaceInMiddle(
+      const string16& old_text,
+      const string16& new_text,
+      size_t caret_position) const;
 
   // Tries to start an instant preview for |match|. Returns true if instant
   // processed the match.
@@ -547,18 +551,10 @@ class AutocompleteEditModel : public AutocompleteControllerDelegate {
   bool in_revert_;
 
   // Indicates if the upcoming autocomplete search is allowed to be treated as
-  // an exact keyword match. If it's true then keyword mode will be triggered
-  // automatically if the input is "<keyword> <search string>". We only allow
-  // such trigger when:
-  // 1.A single space character is added at the end of a keyword, such as:
-  //   (assume "foo" is a keyword, | is the input caret)
-  //   foo| -> foo |
-  //   foo[bar] -> foo |  ([bar] indicates a selected text "bar")
-  // 2.A single space character is inserted after a keyword when the caret is
-  //   not at the end of the line, such as:
-  //   foo|bar -> foo |bar
-  //
-  // It has no effect if a keyword is already selected.
+  // 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_;
 
   // Last value of InstantCompleteBehavior supplied to |SetSuggestedText|.

chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm

@@ -696,9 +696,9 @@ bool OmniboxViewMac::OnAfterPossibleChange() {
   delete_at_end_pressed_ = false;
 
   const bool something_changed = model_->OnAfterPossibleChange(
-      new_text, new_selection.location, NSMaxRange(new_selection),
-      selection_differs, text_differs, just_deleted_text,
-      !IsImeComposing());
+      text_before_change_, new_text, new_selection.location,
+      NSMaxRange(new_selection), selection_differs, text_differs,
+      just_deleted_text, !IsImeComposing());
 
   if (delete_was_pressed_ && at_end_of_edit)
     delete_at_end_pressed_ = true;

chrome/browser/ui/gtk/omnibox/omnibox_view_gtk.cc

@@ -724,11 +724,8 @@ bool OmniboxViewGtk::OnAfterPossibleChange() {
 
   // See if the text or selection have changed since OnBeforePossibleChange().
   const string16 new_text(GetText());
-  text_changed_ = (new_text != text_before_change_);
-  if (supports_pre_edit_) {
-    text_changed_ =
-        text_changed_ || (pre_edit_.size() != pre_edit_size_before_change_);
-  }
+  text_changed_ = (new_text != text_before_change_) || (supports_pre_edit_ &&
+      (pre_edit_.size() != pre_edit_size_before_change_));
 
   if (text_changed_)
     AdjustTextJustification();
@@ -746,9 +743,9 @@ bool OmniboxViewGtk::OnAfterPossibleChange() {
   delete_at_end_pressed_ = false;
 
   const bool something_changed = model_->OnAfterPossibleChange(
-      new_text, new_sel.selection_min(), new_sel.selection_max(),
-      selection_differs, text_changed_, just_deleted_text,
-      !IsImeComposing());
+      text_before_change_, new_text, new_sel.selection_min(),
+      new_sel.selection_max(), selection_differs, text_changed_,
+      just_deleted_text, !IsImeComposing());
 
   // If only selection was changed, we don't need to call |controller_|'s
   // OnChanged() method, which is called in TextChanged().

chrome/browser/ui/omnibox/omnibox_view_browsertest.cc

@@ -54,6 +54,7 @@ using base::TimeDelta;
 namespace {
 
 const char kSearchKeyword[] = "foo";
+const char kSearchKeyword2[] = "footest.com";
 const wchar_t kSearchKeywordKeys[] = {
   ui::VKEY_F, ui::VKEY_O, ui::VKEY_O, 0
 };
@@ -290,6 +291,12 @@ class OmniboxViewTest : public InProcessBrowserTest,
     template_url->set_short_name(ASCIIToUTF16(kSearchShortName));
     model->Add(template_url);
     model->SetDefaultSearchProvider(template_url);
+
+    TemplateURL* second_url = new TemplateURL();
+    second_url->SetURL(kSearchURL, 0, 0);
+    second_url->set_keyword(ASCIIToUTF16(kSearchKeyword2));
+    second_url->set_short_name(ASCIIToUTF16(kSearchShortName));
+    model->Add(second_url);
   }
 
   void AddHistoryEntry(const TestHistoryEntry& entry, const Time& time) {
@@ -875,6 +882,57 @@ class OmniboxViewTest : public InProcessBrowserTest,
     ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
     ASSERT_EQ(search_keyword, omnibox_view->model()->keyword());
     ASSERT_TRUE(omnibox_view->GetText().empty());
+
+    // Space in the middle of a temporary text, which separates the text into
+    // keyword and replacement portions, should trigger keyword mode.
+    omnibox_view->SetUserText(string16());
+    ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys));
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    AutocompletePopupModel* popup_model = omnibox_view->model()->popup_model();
+    ASSERT_TRUE(popup_model->IsOpen());
+    ASSERT_EQ(ASCIIToUTF16("foobar.com"), omnibox_view->GetText());
+    omnibox_view->model()->OnUpOrDownKeyPressed(1);
+    omnibox_view->model()->OnUpOrDownKeyPressed(-1);
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_LEFT, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0));
+    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
+    ASSERT_EQ(search_keyword, omnibox_view->model()->keyword());
+    ASSERT_EQ(ASCIIToUTF16("bar.com"), omnibox_view->GetText());
+
+    // Space after temporary text that looks like a keyword, when the original
+    // input does not look like a keyword, should trigger keyword mode.
+    omnibox_view->SetUserText(string16());
+    const TestHistoryEntry kHistoryFoo = {
+      "http://footest.com", "Page footest", kSearchText, 1000, 1000, true
+    };
+
+    // Add a history entry to trigger HQP matching with text == keyword when
+    // typing "fo te".
+    ASSERT_NO_FATAL_FAILURE(
+        AddHistoryEntry(kHistoryFoo, Time::Now() - TimeDelta::FromMinutes(10)));
+
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_F, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_O, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_T, 0));
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_E, 0));
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    ASSERT_TRUE(popup_model->IsOpen());
+    string16 search_keyword2(ASCIIToUTF16(kSearchKeyword2));
+    while ((omnibox_view->GetText() != search_keyword2) &&
+           (popup_model->selected_line() < popup_model->result().size() - 1))
+      omnibox_view->model()->OnUpOrDownKeyPressed(1);
+    ASSERT_EQ(search_keyword2, omnibox_view->GetText());
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, 0));
+    ASSERT_FALSE(omnibox_view->model()->is_keyword_hint());
+    ASSERT_EQ(search_keyword2, omnibox_view->model()->keyword());
+    ASSERT_TRUE(omnibox_view->GetText().empty());
   }
 
   void NonSubstitutingKeywordTest() {

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

@@ -594,8 +594,9 @@ bool OmniboxViewViews::OnAfterPossibleChange() {
       (new_sel.start() <= sel_before_change_.GetMin());
 
   const bool something_changed = model_->OnAfterPossibleChange(
-      new_text, new_sel.start(), new_sel.end(), selection_differs,
-      text_changed, just_deleted_text, !textfield_->IsIMEComposing());
+      text_before_change_, new_text, new_sel.start(), new_sel.end(),
+      selection_differs, text_changed, just_deleted_text,
+      !textfield_->IsIMEComposing());
 
   // If only selection was changed, we don't need to call |model_|'s
   // OnChanged() method, which is called in TextChanged().

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

@@ -915,8 +915,8 @@ bool OmniboxViewWin::OnAfterPossibleChangeInternal(bool force_text_changed) {
                                  sel_before_change_.cpMax));
 
   const bool something_changed = model_->OnAfterPossibleChange(
-      new_text, new_sel.cpMin, new_sel.cpMax, selection_differs,
-      text_differs, just_deleted_text, !IsImeComposing());
+      text_before_change_, new_text, new_sel.cpMin, new_sel.cpMax,
+      selection_differs, text_differs, just_deleted_text, !IsImeComposing());
 
   if (selection_differs)
     controller_->OnSelectionBoundsChanged();