Allow space to accept keyword even when inline autocomplete is available.

This CL assumes keyword query is always synchronous.

BUG=70527
TEST=See bug report.

Review URL: http://codereview.chromium.org/6281011

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

chrome/browser/autocomplete/autocomplete_edit.cc

@@ -645,20 +645,12 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
     return false;
   }
 
+  const std::wstring 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
   if (user_text_changed) {
-    const std::wstring new_user_text = UserTextFromDisplayText(new_text);
-
-    // Try to accept the current keyword if the user only typed a space at the
-    // end of content. Model's state and popup will be updated when the keyword
-    // is accepted. So we just need to return false here.
-    if (allow_keyword_ui_change && !selection_differs &&
-        MaybeAcceptKeywordBySpace(new_user_text))
-      return false;
-
-    InternalSetUserText(new_user_text);
+    InternalSetUserText(UserTextFromDisplayText(new_text));
     has_temporary_text_ = false;
 
     // Track when the user has deleted text so we won't allow inline
@@ -667,6 +659,14 @@ bool AutocompleteEditModel::OnAfterPossibleChange(
   }
 
   view_->UpdatePopup();
+
+  // 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.
+  if (text_differs && allow_keyword_ui_change && !just_deleted_text &&
+      MaybeAcceptKeywordBySpace(old_user_text, user_text_))
+    return false;
+
   return true;
 }
 
@@ -782,13 +782,15 @@ bool AutocompleteEditModel::GetURLForText(const std::wstring& text,
 }
 
 bool AutocompleteEditModel::MaybeAcceptKeywordBySpace(
+    const std::wstring& old_user_text,
     const std::wstring& new_user_text) {
   return (paste_state_ == NONE) && is_keyword_hint_ && !keyword_.empty() &&
-      inline_autocomplete_text_.empty() && !user_text_.empty() &&
-      (new_user_text.length() == user_text_.length() + 1) &&
-      !new_user_text.compare(0, user_text_.length(), user_text_) &&
-      IsSpaceCharForAcceptingKeyword(new_user_text[user_text_.length()]) &&
-      !IsWhitespace(user_text_[user_text_.length() - 1]) &&
+      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) &&
       AcceptKeyword();
 }
 

chrome/browser/autocomplete/autocomplete_edit.h

@@ -379,8 +379,10 @@ class AutocompleteEditModel : public NotificationObserver {
   bool GetURLForText(const std::wstring& text, GURL* url) const;
 
   // Accepts current keyword if the user only typed a space at the end of
-  // |new_user_text|. Returns true if the current keyword is accepted.
-  bool MaybeAcceptKeywordBySpace(const std::wstring& new_user_text);
+  // |new_user_text| comparing to the |old_user_text|.
+  // Returns true if the current keyword is accepted.
+  bool MaybeAcceptKeywordBySpace(const std::wstring& old_user_text,
+                                 const std::wstring& new_user_text);
 
   // Checks if a given character is a valid space character for accepting
   // keyword.

chrome/browser/autocomplete/autocomplete_edit_view_browsertest.cc

@@ -254,7 +254,7 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
     model->SetDefaultSearchProvider(template_url);
   }
 
-  void SetupHistory() {
+  void AddHistoryEntry(const TestHistoryEntry& entry, const Time& time) {
     Profile* profile = browser()->profile();
     HistoryService* history_service =
         profile->GetHistoryService(Profile::EXPLICIT_ACCESS);
@@ -277,22 +277,26 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
       ui_test_utils::RunMessageLoop();
     }
 
+    GURL url(entry.url);
+    // Add everything in order of time. We don't want to have a time that
+    // is "right now" or it will nondeterministically appear in the results.
+    history_service->AddPageWithDetails(url, UTF8ToUTF16(entry.title),
+                                        entry.visit_count,
+                                        entry.typed_count, time, false,
+                                        history::SOURCE_BROWSED);
+    history_service->SetPageContents(url, UTF8ToUTF16(entry.body));
+    if (entry.starred)
+      bookmark_model->SetURLStarred(url, string16(), true);
+  }
+
+  void SetupHistory() {
     // Add enough history pages containing |kSearchText| to trigger
     // open history page url in autocomplete result.
     for (size_t i = 0; i < arraysize(kHistoryEntries); i++) {
-      const TestHistoryEntry& cur = kHistoryEntries[i];
-      GURL url(cur.url);
       // Add everything in order of time. We don't want to have a time that
       // is "right now" or it will nondeterministically appear in the results.
       Time t = Time::Now() - TimeDelta::FromHours(i + 1);
-      history_service->AddPageWithDetails(url, UTF8ToUTF16(cur.title),
-                                          cur.visit_count,
-                                          cur.typed_count, t, false,
-                                          history::SOURCE_BROWSED);
-      history_service->SetPageContents(url, UTF8ToUTF16(cur.body));
-      if (cur.starred) {
-        bookmark_model->SetURLStarred(url, string16(), true);
-      }
+      ASSERT_NO_FATAL_FAILURE(AddHistoryEntry(kHistoryEntries[i], t));
     }
   }
 
@@ -738,7 +742,8 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
     ASSERT_TRUE(edit_view->model()->keyword().empty());
     ASSERT_EQ(text + L" bar", edit_view->GetText());
 
-    // Keyword shouldn't be accepted by pressing space with a selected range.
+    // Keyword could be accepted by pressing space with a selected range at the
+    // end of text.
     edit_view->OnBeforePossibleChange();
     edit_view->OnInlineAutocompleteTextMaybeChanged(
         text + L"  ", text.length());
@@ -751,12 +756,36 @@ class AutocompleteEditViewTest : public InProcessBrowserTest,
     edit_view->GetSelectionBounds(&start, &end);
     ASSERT_NE(start, end);
     ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, false, false, false));
-    ASSERT_TRUE(edit_view->model()->is_keyword_hint());
+    ASSERT_FALSE(edit_view->model()->is_keyword_hint());
     ASSERT_EQ(text, edit_view->model()->keyword());
-    ASSERT_EQ(text + L" ", edit_view->GetText());
+    ASSERT_EQ(std::wstring(), edit_view->GetText());
 
-    edit_view->GetSelectionBounds(&start, &end);
-    ASSERT_EQ(start, end);
+    edit_view->SetUserText(std::wstring());
+
+    // Space should accept keyword even when inline autocomplete is available.
+    const TestHistoryEntry kHistoryFoobar = {
+      "http://www.foobar.com", "Page foobar", kSearchText, 10000, 10000, true
+    };
+
+    // Add a history entry to trigger inline autocomplete when typing "foo".
+    ASSERT_NO_FATAL_FAILURE(
+        AddHistoryEntry(kHistoryFoobar, Time::Now() - TimeDelta::FromHours(1)));
+
+    // Type "foo" to trigger inline autocomplete.
+    ASSERT_NO_FATAL_FAILURE(SendKeySequence(kSearchKeywordKeys));
+    ASSERT_NO_FATAL_FAILURE(WaitForAutocompleteControllerDone());
+    ASSERT_TRUE(edit_view->model()->popup_model()->IsOpen());
+    ASSERT_NE(text, edit_view->GetText());
+
+    // Keyword hint shouldn't be visible.
+    ASSERT_FALSE(edit_view->model()->is_keyword_hint());
+    ASSERT_TRUE(edit_view->model()->keyword().empty());
+
+    // Trigger keyword mode by space.
+    ASSERT_NO_FATAL_FAILURE(SendKey(ui::VKEY_SPACE, false, false, false));
+    ASSERT_FALSE(edit_view->model()->is_keyword_hint());
+    ASSERT_EQ(text, edit_view->model()->keyword());
+    ASSERT_TRUE(edit_view->GetText().empty());
   }
 
 };