Enabled pressing TAB to traverse through the Omnibox results, removed…

… moving the caret to the end of the line with TAB, and filtered out redundant URLs.

This adds the ability to move through Omnibox result matches using TAB in addition to the arrow keys. To enable this, pressing TAB to move the caret to the end of the line was removed and the keyword hint/shortcut logic has been modified.

The Omnibox popup now shows keyword markers on the right side of matches that have associated keywords (represented by a right arrow). When this kind of match is selected, and the keyword is accepted, the match changes appearance by animating in the associated keyword match from the right to display the "Search X for <>" message. 

If multiple matches have the same keyword then only the most relevant match will display the keyword marker and hint. Pressing TAB while a keyword hint is shown will enter keyword mode in place; the results will no longer change when keyword mode is entered. Additionally, substituting keyword provider matches will only be shown if a keyword substitution
is available.  

Finally, results with redundant destination URLs (e.g., "foo.com", "www.foo.com") will have the less relevant URLs filtered out.

This also addresses some GTK omnibox browsertest flakiness; see bug 112041.

See original change review at http://codereview.chromium.org/6731036

Contributed by aaron.randolph@gmail.com

BUG=57748,76278,77662,80934,84420
TEST=Press TAB to move the selection down the list of results, SHIFT+TAB to move up.

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

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

AUTHORS

@@ -164,3 +164,4 @@ Erik Hill <erikghill@gmail.com>
 Mao Yujie <maojie0924@gmail.com>
 Aaron Leventhal <aaronlevbugs@gmail.com>
 Peter Collingbourne <peter@pcc.me.uk>
+Aaron Randolph <aaron.randolph@gmail.com>

chrome/browser/autocomplete/autocomplete.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/autocomplete/autocomplete.h"
 
 #include <algorithm>
+#include <set>
 
 #include "base/basictypes.h"
 #include "base/command_line.h"
@@ -24,12 +25,16 @@
 #include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/autocomplete/shortcuts_provider.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/external_protocol/external_protocol_handler.h"
 #include "chrome/browser/instant/instant_field_trial.h"
 #include "chrome/browser/net/url_fixer_upper.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
+#include "chrome/browser/search_engines/template_url.h"
+#include "chrome/browser/search_engines/template_url_service.h"
+#include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/webui/history_ui.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
@@ -683,6 +688,9 @@ void AutocompleteResult::AddMatch(const AutocompleteMatch& match) {
 }
 
 void AutocompleteResult::SortAndCull(const AutocompleteInput& input) {
+  for (ACMatches::iterator i = matches_.begin(); i != matches_.end(); ++i)
+    i->ComputeStrippedDestinationURL();
+
   // Remove duplicates.
   std::sort(matches_.begin(), matches_.end(),
             &AutocompleteMatch::DestinationSortFunc);
@@ -991,26 +999,62 @@ void AutocompleteController::UpdateResult(bool is_synchronous_pass) {
   }
 
   UpdateKeywordDescriptions(&result_);
+  UpdateAssociatedKeywords(&result_);
 
   bool notify_default_match = is_synchronous_pass;
   if (!is_synchronous_pass) {
     const bool last_default_was_valid =
         last_result.default_match() != last_result.end();
     const bool default_is_valid = result_.default_match() != result_.end();
     // We've gotten async results. Send notification that the default match
-    // updated if fill_into_edit differs. We don't check the URL as that may
-    // change for the default match even though the fill into edit hasn't
-    // changed (see SearchProvider for one case of this).
+    // updated if fill_into_edit differs or associated_keyword differ.  (The
+    // latter can change if we've just started Chrome and the keyword database
+    // finishes loading while processing this request.) We don't check the URL
+    // as that may change for the default match even though the fill into edit
+    // hasn't changed (see SearchProvider for one case of this).
     notify_default_match =
         (last_default_was_valid != default_is_valid) ||
         (default_is_valid &&
-         (result_.default_match()->fill_into_edit !=
-          last_result.default_match()->fill_into_edit));
+          ((result_.default_match()->fill_into_edit !=
+            last_result.default_match()->fill_into_edit) ||
+            (result_.default_match()->associated_keyword.get() !=
+              last_result.default_match()->associated_keyword.get())));
   }
 
   NotifyChanged(notify_default_match);
 }
 
+void AutocompleteController::UpdateAssociatedKeywords(
+    AutocompleteResult* result) {
+  if (!keyword_provider_)
+    return;
+
+  std::set<string16> keywords;
+  for (ACMatches::iterator match(result->begin()); match != result->end();
+       ++match) {
+    if (!match->keyword.empty()) {
+      keywords.insert(match->keyword);
+    } else {
+      string16 keyword = match->associated_keyword.get() ?
+          match->associated_keyword->keyword :
+          keyword_provider_->GetKeywordForText(match->fill_into_edit);
+
+      // Only add the keyword if the match does not have a duplicate keyword
+      // with a more relevant match.
+      if (!keyword.empty() && !keywords.count(keyword)) {
+        keywords.insert(keyword);
+
+        if (!match->associated_keyword.get())
+          match->associated_keyword.reset(new AutocompleteMatch(
+              keyword_provider_->CreateAutocompleteMatch(match->fill_into_edit,
+                  keyword, input_)));
+      } else {
+        match->associated_keyword.reset();
+      }
+    }
+  }
+}
+
 void AutocompleteController::UpdateKeywordDescriptions(
     AutocompleteResult* result) {
   const TemplateURL* last_template_url = NULL;

chrome/browser/autocomplete/autocomplete.h

@@ -10,6 +10,7 @@
 #include <string>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/string16.h"
@@ -678,8 +679,12 @@ class AutocompleteController : public ACProviderListener {
   void set_search_provider(SearchProvider* provider) {
     search_provider_ = provider;
   }
+  void set_keyword_provider(KeywordProvider* provider) {
+    keyword_provider_ = provider;
+  }
 #endif
   SearchProvider* search_provider() const { return search_provider_; }
+  KeywordProvider* keyword_provider() const { return keyword_provider_; }
 
   // Getters
   const AutocompleteInput& input() const { return input_; }
@@ -691,11 +696,20 @@ class AutocompleteController : public ACProviderListener {
   virtual void OnProviderUpdate(bool updated_matches);
 
  private:
+  friend class AutocompleteProviderTest;
+  FRIEND_TEST_ALL_PREFIXES(AutocompleteProviderTest,
+                           RedundantKeywordsIgnoredInResult);
+
   // Updates |result_| to reflect the current provider state.  Resets timers and
   // fires notifications as necessary.  |is_synchronous_pass| is true only when
   // Start() is calling this to get the synchronous result.
   void UpdateResult(bool is_synchronous_pass);
 
+  // Updates |result| to populate each match's |associated_keyword| if that
+  // match can show a keyword hint.  |result| should be sorted by
+  // relevance before this is called.
+  void UpdateAssociatedKeywords(AutocompleteResult* result);
+
   // For each group of contiguous matches from the same TemplateURL, show the
   // provider name as a description on the first match in the group.
   void UpdateKeywordDescriptions(AutocompleteResult* result);

chrome/browser/autocomplete/autocomplete_edit.cc

@@ -173,7 +173,8 @@ void AutocompleteEditModel::FinalizeInstantQuery(
   if (skip_inline_autocomplete) {
     const string16 final_text = input_text + suggest_text;
     view_->OnBeforePossibleChange();
-    view_->SetWindowTextAndCaretPos(final_text, final_text.length());
+    view_->SetWindowTextAndCaretPos(final_text, final_text.length(), false,
+        false);
     view_->OnAfterPossibleChange();
   } else if (popup_->IsOpen()) {
     SearchProvider* search_provider =
@@ -406,7 +407,8 @@ void AutocompleteEditModel::Revert() {
   is_keyword_hint_ = false;
   has_temporary_text_ = false;
   view_->SetWindowTextAndCaretPos(permanent_text_,
-                                  has_focus_ ? permanent_text_.length() : 0);
+                                  has_focus_ ? permanent_text_.length() : 0,
+                                  false, true);
   NetworkActionPredictor* network_action_predictor =
       NetworkActionPredictorFactory::GetForProfile(profile_);
   if (network_action_predictor)
@@ -416,6 +418,8 @@ void AutocompleteEditModel::Revert() {
 void AutocompleteEditModel::StartAutocomplete(
     bool has_selected_text,
     bool prevent_inline_autocomplete) const {
+  ClearPopupKeywordMode();
+
   bool keyword_is_selected = KeywordIsSelected();
   popup_->SetHoveredLine(AutocompletePopupModel::kNoMatch);
   // We don't explicitly clear AutocompletePopupModel::manually_selected_match,
@@ -618,28 +622,49 @@ void AutocompleteEditModel::OpenMatch(const AutocompleteMatch& match,
 bool AutocompleteEditModel::AcceptKeyword() {
   DCHECK(is_keyword_hint_ && !keyword_.empty());
 
-  view_->OnBeforePossibleChange();
-  view_->SetWindowTextAndCaretPos(string16(), 0);
+  autocomplete_controller_->Stop(false);
   is_keyword_hint_ = false;
-  view_->OnAfterPossibleChange();
-  just_deleted_text_ = false;  // OnAfterPossibleChange() erroneously sets this
-                               // since the edit contents have disappeared.  It
-                               // doesn't really matter, but we clear it to be
-                               // consistent.
+
+  if (popup_->IsOpen())
+    popup_->SetSelectedLineState(AutocompletePopupModel::KEYWORD);
+  else
+    StartAutocomplete(false, true);
+
+  // Ensure the current selection is saved before showing keyword mode
+  // so that moving to another line and then reverting the text will restore
+  // the current state properly.
+  view_->OnTemporaryTextMaybeChanged(
+      DisplayTextFromUserText(CurrentMatch().fill_into_edit),
+      !has_temporary_text_);
+  has_temporary_text_ = true;
+
   content::RecordAction(UserMetricsAction("AcceptedKeywordHint"));
   return true;
 }
 
 void AutocompleteEditModel::ClearKeyword(const string16& visible_text) {
-  view_->OnBeforePossibleChange();
+  autocomplete_controller_->Stop(false);
+  ClearPopupKeywordMode();
+
   const string16 window_text(keyword_ + visible_text);
-  view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length());
-  keyword_.clear();
-  is_keyword_hint_ = false;
-  view_->OnAfterPossibleChange();
-  just_deleted_text_ = true;  // OnAfterPossibleChange() fails to clear this
-                              // since the edit contents have actually grown
-                              // longer.
+
+  // Only reset the result if the edit text has changed since the
+  // keyword was accepted, or if the popup is closed.
+  if (just_deleted_text_ || !visible_text.empty() || !popup_->IsOpen()) {
+    view_->OnBeforePossibleChange();
+    view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(),
+        false, false);
+    keyword_.clear();
+    is_keyword_hint_ = false;
+    view_->OnAfterPossibleChange();
+    just_deleted_text_ = true;  // OnAfterPossibleChange() fails to clear this
+                                // since the edit contents have actually grown
+                                // longer.
+  } else {
+    is_keyword_hint_ = true;
+    view_->SetWindowTextAndCaretPos(window_text.c_str(), keyword_.length(),
+        false, true);
+  }
 }
 
 const AutocompleteResult& AutocompleteEditModel::result() const {
@@ -902,8 +927,9 @@ void AutocompleteEditModel::OnResultChanged(bool default_match_changed) {
       // can be many of these as a user types an initial series of characters,
       // the OS DNS cache could suffer eviction problems for minimal gain.
 
-      is_keyword_hint = popup_->GetKeywordForMatch(*match, &keyword);
+      is_keyword_hint = match->GetKeyword(&keyword);
     }
+
     popup_->OnResultChanged();
     OnPopupDataChanged(inline_autocomplete_text, NULL, keyword,
                        is_keyword_hint);
@@ -936,6 +962,12 @@ bool AutocompleteEditModel::KeywordIsSelected() const {
   return !is_keyword_hint_ && !keyword_.empty();
 }
 
+void AutocompleteEditModel::ClearPopupKeywordMode() const {
+  if (popup_->IsOpen() &&
+      popup_->selected_line_state() == AutocompletePopupModel::KEYWORD)
+    popup_->SetSelectedLineState(AutocompletePopupModel::NORMAL);
+}
+
 string16 AutocompleteEditModel::DisplayTextFromUserText(
     const string16& text) const {
   return KeywordIsSelected() ?
@@ -1035,7 +1067,9 @@ bool AutocompleteEditModel::ShouldAllowExactKeywordMatch(
                  TRIM_LEADING, &keyword);
 
   // Only allow exact keyword match if |keyword| represents a keyword hint.
-  return keyword.length() && popup_->GetKeywordForText(keyword, &keyword);
+  return keyword.length() &&
+      !autocomplete_controller_->keyword_provider()->
+          GetKeywordForText(keyword).empty();
 }
 
 bool AutocompleteEditModel::DoInstant(const AutocompleteMatch& match,

chrome/browser/autocomplete/autocomplete_edit.h

@@ -381,6 +381,9 @@ class AutocompleteEditModel : public AutocompleteControllerDelegate {
   // Returns true if a keyword is selected.
   bool KeywordIsSelected() const;
 
+  // Turns off keyword mode for the current match.
+  void ClearPopupKeywordMode() const;
+
   // Conversion between user text and display text. User text is the text the
   // user has input. Display text is the text being shown in the edit. The
   // two are different if a keyword is selected.

chrome/browser/autocomplete/autocomplete_edit_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -35,7 +35,9 @@ class TestingOmniboxView : public OmniboxView {
                            const string16& display_text,
                            bool update_popup) OVERRIDE {}
   virtual void SetWindowTextAndCaretPos(const string16& text,
-                                        size_t caret_pos) OVERRIDE {}
+                                        size_t caret_pos,
+                                        bool update_popup,
+                                        bool notify_text_changed) OVERRIDE {}
   virtual void SetForcedQuery() OVERRIDE {}
   virtual bool IsSelectAll() OVERRIDE { return false; }
   virtual bool DeleteAtEndPressed() OVERRIDE { return false; }
@@ -155,9 +157,9 @@ TEST(AutocompleteEditTest, AdjustTextForCopy) {
   TestingOmniboxView view;
   TestingAutocompleteEditController controller;
   TestingProfile profile;
-  AutocompleteEditModel model(&view, &controller, &profile);
-  profile.CreateAutocompleteClassifier();
   profile.CreateTemplateURLService();
+  profile.CreateAutocompleteClassifier();
+  AutocompleteEditModel model(&view, &controller, &profile);
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(input); ++i) {
     model.UpdatePermanentText(ASCIIToUTF16(input[i].perm_text));