Omnibox SearchProvider Experiment Client Implementation.

chrome/browser/autocomplete/search_provider.cc

Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index 28a5d2b81f3347bd533f37c76415157484164f8e..e10bf8f73d923f8083f64e5ecf8133196217d035 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -63,7 +63,7 @@ bool HasMultipleWords(const string16& text) {
   return false;
 }
 
-};
+}  // namespace
 
 
 // SearchProvider::Providers --------------------------------------------------
@@ -96,6 +96,9 @@ SearchProvider::SearchProvider(ACProviderListener* listener, Profile* profile)
     : AutocompleteProvider(listener, profile, "Search"),
       providers_(TemplateURLServiceFactory::GetForProfile(profile)),
       suggest_results_pending_(0),
+      has_suggested_relevance_(false),
+      has_verbatim_relevance_(false),
+      verbatim_relevance_(0),
       have_suggest_results_(false),
       instant_finalized_(false) {
   // We use GetSuggestNumberOfGroups() as the group ID to mean "not in field
@@ -308,7 +311,6 @@ void SearchProvider::Run() {
 
 void SearchProvider::Stop() {
   StopSuggest();
-  ClearResults();
   done_ = true;
   default_provider_suggest_text_.clear();
 }
@@ -441,7 +443,9 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
 
   // We can't keep running any previous query, so halt it.
   StopSuggest();
-  ClearResults();
+
+  // Remove existing results that cannot inline autocomplete the new input.
+  RemoveStaleResults();
 
   // We can't start a new query if we're only allowed synchronous results.
   if (input_.matches_requested() != AutocompleteInput::ALL_MATCHES)
@@ -528,9 +532,62 @@ void SearchProvider::ClearResults() {
   default_suggest_results_.clear();
   keyword_navigation_results_.clear();
   default_navigation_results_.clear();
+  has_suggested_relevance_ = false;
+  has_verbatim_relevance_ = false;
+  verbatim_relevance_ = 0;
   have_suggest_results_ = false;
 }
 
+void SearchProvider::RemoveStaleResults() {
+  RemoveStaleSuggestResults(&keyword_suggest_results_, true);
+  RemoveStaleSuggestResults(&default_suggest_results_, false);
+  RemoveStaleNavigationResults(&keyword_navigation_results_, true);
+  RemoveStaleNavigationResults(&default_navigation_results_, false);
+}
+
+void SearchProvider::RemoveStaleSuggestResults(SuggestResults* list,
+                                               bool is_keyword) {
+  const string16& input = is_keyword ? keyword_input_text_ : input_.text();
+  for (SuggestResults::iterator i = list->begin(); i < list->end();)
+    i = (i->suggestion().find(input) != 0) ? list->erase(i) : (i + 1);

[Peter Kasting, 2012/06/05 00:47:51]

Nit: Seems clearer:

  i = StartsWith(i->suggestion(), input, false) ? (i + 1) : list->erase(i);

Note that passing "false" here does case-insensitive comparison... I think
that's probably what you want (though you should check with the suggest guys).

BestURLPrefix() should also do case-insensitive comparisons, I think, which
should address the similar problem below.

You may also want to remove the "!" in RemoveStaleNavigationResults() and
reverse the conditional so the orders in the two functions still match.

[msw, 2012/06/05 01:49:41]

Done.

+}
+
+void SearchProvider::RemoveStaleNavigationResults(NavigationResults* list,
+                                                  bool is_keyword) {
+  const string16& input = is_keyword ? keyword_input_text_ : input_.text();
+  for (NavigationResults::iterator i = list->begin(); i < list->end();) {
+    const string16 fill(AutocompleteInput::FormattedStringWithEquivalentMeaning(
+        i->url(), StringForURLDisplay(i->url(), true, false)));
+    i = !URLPrefix::BestURLPrefix(fill, input) ? list->erase(i) : (i + 1);
+  }
+}
+
+void SearchProvider::ApplyCalculatedRelevance() {
+  ApplyCalculatedSuggestRelevance(&keyword_suggest_results_, true);
+  ApplyCalculatedSuggestRelevance(&default_suggest_results_, false);
+  ApplyCalculatedNavigationRelevance(&keyword_navigation_results_, true);
+  ApplyCalculatedNavigationRelevance(&default_navigation_results_, false);
+  has_suggested_relevance_ = false;
+  has_verbatim_relevance_ = false;
+  verbatim_relevance_ = 0;
+}
+
+void SearchProvider::ApplyCalculatedSuggestRelevance(SuggestResults* list,
+                                                     bool is_keyword) {
+  for (size_t i = 0; i < list->size(); ++i) {
+    (*list)[i].set_relevance(CalculateRelevanceForSuggestion(is_keyword) +
+                             (list->size() - i - 1));
+  }
+}
+
+void SearchProvider::ApplyCalculatedNavigationRelevance(NavigationResults* list,
+                                                        bool is_keyword) {
+  for (size_t i = 0; i < list->size(); ++i) {
+    (*list)[i].set_relevance(CalculateRelevanceForNavigation(is_keyword) +
+                             (list->size() - i - 1));
+  }
+}
+
 net::URLFetcher* SearchProvider::CreateSuggestFetcher(
     int id,
     const TemplateURLRef& suggestions_url,
@@ -564,12 +621,30 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
 
   // 4th element: Disregard the query URL list for now.
 
+  // Reset suggested relevance information from the default provider.
+  if (!is_keyword) {
+    has_suggested_relevance_ = false;
+    has_verbatim_relevance_ = false;
+    verbatim_relevance_ = 0;
+  }
+
   // 5th element: Optional key-value pairs from the Suggest server.
   ListValue* types = NULL;
-  DictionaryValue* dict_val = NULL;
-  if (root_list->GetDictionary(4, &dict_val)) {
-    // Parse Google Suggest specific type extension.
-    dict_val->GetList("google:suggesttype", &types);
+  ListValue* relevances = NULL;
+  DictionaryValue* extras = NULL;
+  if (root_list->GetDictionary(4, &extras)) {
+    extras->GetList("google:suggesttype", &types);
+
+    // Only accept relevance suggestions if Instant is disabled.
+    if (!is_keyword && !InstantController::IsEnabled(profile_)) {
+      // Discard this list if its size does not match that of the suggestions.
+      if (extras->GetList("google:suggestrelevance", &relevances) &&
+          relevances->GetSize() != results->GetSize())
+        relevances = NULL;
+
+      if (extras->GetInteger("google:verbatimrelevance", &verbatim_relevance_))
+        has_verbatim_relevance_ = true;
+    }
   }
 
   SuggestResults* suggest_results =
@@ -577,46 +652,46 @@ bool SearchProvider::ParseSuggestResults(Value* root_val, bool is_keyword) {
   NavigationResults* navigation_results =
       is_keyword ? &keyword_navigation_results_ : &default_navigation_results_;
 
-  string16 result;
-  bool results_updated = false;
+  // Clear the previous results now that new results are available.
+  suggest_results->clear();
+  navigation_results->clear();
+
+  string16 result, title;
+  std::string type;
+  int relevance = -1;
   for (size_t index = 0; results->GetString(index, &result); ++index) {
     // Google search may return empty suggestions for weird input characters,
     // they make no sense at all and can cause problems in our code.
     if (result.empty())
       continue;
 
-    std::string type;
+    // Apply valid suggested relevance scores; discard invalid lists.
+    if (relevances != NULL && !relevances->GetInteger(index, &relevance))
+      relevances = NULL;
     if (types && types->GetString(index, &type) && (type == "NAVIGATION")) {
-      if (navigation_results->size() < kMaxMatches) {
-        // Do not blindly trust the URL coming from the server to be valid.
-        GURL url(URLFixerUpper::FixupURL(UTF16ToUTF8(result), std::string()));
-        if (url.is_valid()) {
-          string16 description;
-          if (descriptions != NULL)
-            descriptions->GetString(index, &description);
-          // Decrement the relevance for successive results to preserve order.
-          int relevance = CalculateRelevanceForNavigation(is_keyword) +
-                          (kMaxMatches - navigation_results->size() - 1);
-          navigation_results->push_back(
-              NavigationResult(url, description, relevance));
-          results_updated = true;
-        }
+      // Do not blindly trust the URL coming from the server to be valid.
+      GURL url(URLFixerUpper::FixupURL(UTF16ToUTF8(result), std::string()));
+      if (url.is_valid()) {
+        if (descriptions != NULL)
+          descriptions->GetString(index, &title);
+        navigation_results->push_back(NavigationResult(url, title, relevance));
       }
     } else {
-      // TODO(kochi): Currently we treat a calculator result as a query, but it
-      // is better to have better presentation for caluculator results.
-      if (suggest_results->size() < kMaxMatches) {
-        // Decrement the relevance for successive results to preserve order.
-        int relevance = CalculateRelevanceForSuggestion(is_keyword) +
-                        (kMaxMatches - suggest_results->size() - 1);
-        suggest_results->push_back(SuggestResult(result, relevance));
-        results_updated = true;
-      }
+      // TODO(kochi): Improve calculator result presentation.
+      suggest_results->push_back(SuggestResult(result, relevance));
     }
   }
 
+  // Apply calculated relevance scores if a valid list was not provided.
+  if (relevances == NULL) {
+    ApplyCalculatedSuggestRelevance(suggest_results, is_keyword);
+    ApplyCalculatedNavigationRelevance(navigation_results, is_keyword);
+  } else if (!is_keyword) {
+    has_suggested_relevance_ = true;
+  }
+
   have_suggest_results_ = true;
-  return results_updated;
+  return true;

[Peter Kasting, 2012/06/05 00:47:51]

Nit: Did you want to preserve the more fine-grained tracking you previously had?
 It seems like it'd work OK as long as we also added in a "results_updated |=
(!suggest_results.empty() || !navigation_results.empty());" before clearing
those lists.

[msw, 2012/06/05 01:49:41]

I'll leave this as-is. Getting the exact same results would also trigger a
results_updated 'false positive'. The only downside here seems to be a (minor?)
perf penalty. I think code simplicity is more important for now.

 }
 
 void SearchProvider::ConvertResultsToAutocompleteMatches() {
@@ -633,9 +708,11 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   int did_not_accept_default_suggestion = default_suggest_results_.empty() ?
       TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
       TemplateURLRef::NO_SUGGESTION_CHOSEN;
-  AddMatchToMap(input_.text(), input_.text(), verbatim_relevance,
-                AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
-                did_not_accept_default_suggestion, false, &map);
+  if (verbatim_relevance > 0) {
+    AddMatchToMap(input_.text(), input_.text(), verbatim_relevance,
+                  AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
+                  did_not_accept_default_suggestion, false, &map);
+  }
   const size_t what_you_typed_size = map.size();
   if (!default_provider_suggest_text_.empty()) {
     AddMatchToMap(input_.text() + default_provider_suggest_text_,
@@ -668,6 +745,16 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
   if (matches_.size() > max_total_matches)
     matches_.erase(matches_.begin() + max_total_matches, matches_.end());
 
+  // The top result must be inlinable; apply calculated scores if it is not.
+  if (!matches_.empty() &&
+      (has_suggested_relevance_ || has_verbatim_relevance_) &&
+      (matches_.front().type == AutocompleteMatch::SEARCH_SUGGEST ||
+       matches_.front().type == AutocompleteMatch::NAVSUGGEST) &&
+      matches_.front().inline_autocomplete_offset == string16::npos) {
+    ApplyCalculatedRelevance();
+    ConvertResultsToAutocompleteMatches();
+  }
+
   UpdateStarredStateOfMatches();
   UpdateDone();
 }
@@ -796,6 +883,9 @@ void SearchProvider::AddSuggestResultsToMap(const SuggestResults& results,
 }
 
 int SearchProvider::CalculateRelevanceForWhatYouTyped() const {
+  if (has_verbatim_relevance_ && !input_.prevent_inline_autocomplete())
+    return verbatim_relevance_;
+
   if (!providers_.keyword_provider().empty())
     return 250;