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 a8e45ee4e45f1923676a5e814c7c8b9ca07e00df..13a2d486dd5c03aaf6dbac68e76c5f069ea08125 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -90,6 +90,8 @@ bool SearchProvider::query_suggest_immediately_ = false;
 SearchProvider::SearchProvider(ACProviderListener* listener, Profile* profile)
     : AutocompleteProvider(listener, profile, "Search"),
       providers_(profile),
+      has_verbatim_relevance_(false),
+      verbatim_relevance_(0),
       suggest_results_pending_(0),
       have_suggest_results_(false),
       instant_finalized_(false) {
@@ -147,6 +149,8 @@ void SearchProvider::FinalizeInstantQuery(const string16& input_text,
     }
   }
 
+  // TODO(msw): Update this or ensure it's not removed in
+  //            ConvertResultsToAutocompleteMatches?
   // Add the new suggest result. We give it a rank higher than
   // SEARCH_WHAT_YOU_TYPED so that it gets autocompleted.
   int did_not_accept_default_suggestion = default_suggest_results_.empty() ?
@@ -166,8 +170,6 @@ void SearchProvider::FinalizeInstantQuery(const string16& input_text,
 
 void SearchProvider::Start(const AutocompleteInput& input,
                            bool minimal_changes) {
-  matches_.clear();
-
   instant_finalized_ =
       (input.matches_requested() != AutocompleteInput::ALL_MATCHES);
 
@@ -234,6 +236,25 @@ void SearchProvider::Start(const AutocompleteInput& input,
   ConvertResultsToAutocompleteMatches();
 }
 
+SearchProvider::SuggestResult::SuggestResult(const string16& suggestion,
+                                             bool has_suggested_relevance,
+                                             int relevance)
+    : suggestion(suggestion),
+      has_suggested_relevance(has_suggested_relevance),
+      relevance(relevance) {
+}
+
+SearchProvider::NavigationResult::NavigationResult(
+    const GURL& url,
+    const string16& site_name,
+    bool has_suggested_relevance,
+    int relevance)
+    : url(url),
+      site_name(site_name),
+      has_suggested_relevance(has_suggested_relevance),
+      relevance(relevance) {
+}
+
 class SearchProvider::CompareScoredTerms {
  public:
   bool operator()(const ScoredTerm& a, const ScoredTerm& b) {
@@ -264,6 +285,7 @@ void SearchProvider::Run() {
 
 void SearchProvider::Stop() {
   StopSuggest();
+  ClearResults();
   done_ = true;
   default_provider_suggest_text_.clear();
 }
@@ -356,8 +378,28 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
   // likely to end up throwing away anyway.
   const int kQueryDelayMs = 200;
 
+  // TODO(msw): Synchronously remove irrelevant inline autocomplete matches,
+  //            update the remaining matches for the new input.
+  //            Mark these matches from_previous = true?
+  for (ACMatches::iterator i = matches_.begin(); i != matches_.end();) {
+    if (i->contents.find(input_.text()) == string16::npos &&
+        i->description.find(input_.text()) == string16::npos) {
+      i = matches_.erase(i);
+    } else {
+      if (i->inline_autocomplete_offset != string16::npos) {

[Peter Kasting, 2012/05/04 00:11:33]

I wouldn't try to adjust the offsets directly, I'd just reconstruct the
matches... see below

[msw, 2012/05/04 09:43:40]

Done; but I still may need to update the results synchronously...

+        if (StartsWith(i->fill_into_edit, input_.text(), false))
+          i->inline_autocomplete_offset = input_.text().length();
+        else
+          i->inline_autocomplete_offset = string16::npos;
+      }
+      // TODO(msw): Fixup navigational result highlighting/styling?
+      ++i;
+    }
+  }
+
   if (!IsQuerySuitableForSuggest()) {
     StopSuggest();
+    ClearResults();
     return;
   }
 
@@ -372,6 +414,7 @@ void SearchProvider::StartOrStopSuggestQuery(bool minimal_changes) {
 
   // We can't keep running any previous query, so halt it.
   StopSuggest();
+  ClearResults();
 
   // We can't start a new query if we're only allowed synchronous results.
   if (input_.matches_requested() != AutocompleteInput::ALL_MATCHES)
@@ -449,6 +492,9 @@ void SearchProvider::StopSuggest() {
   // Stop any in-progress URL fetches.
   keyword_fetcher_.reset();
   default_fetcher_.reset();
+}
+
+void SearchProvider::ClearResults() {
   keyword_suggest_results_.clear();
   default_suggest_results_.clear();
   keyword_navigation_results_.clear();
@@ -502,6 +548,9 @@ bool SearchProvider::ParseSuggestResults(Value* root_val,
 
   // Parse optional data in the results from the Suggest server if any.
   ListValue* type_list = NULL;
+  ListValue* relevance_list = NULL;
+  has_verbatim_relevance_ = false;
+  verbatim_relevance_ = 0;
   // 5th argument: Optional key-value pairs.
   // TODO: We may iterate the 5th+ arguments of the root_list if any other
   // optional data are defined.
@@ -515,6 +564,16 @@ bool SearchProvider::ParseSuggestResults(Value* root_val,
       const std::string kGoogleSuggestType("google:suggesttype");
       if (dict_val->HasKey(kGoogleSuggestType))
         dict_val->GetList(kGoogleSuggestType, &type_list);
+
+      // Parse Google Suggest specific relevance extension.
+      const std::string kGoogleSuggestRelevance("google:suggestrelevance");
+      if (dict_val->HasKey(kGoogleSuggestRelevance))
+        dict_val->GetList(kGoogleSuggestRelevance, &relevance_list);
+
+      // Parse Google Suggest specific verbatim relevance extension.
+      const std::string kGoogleVerbatimRelevance("google:verbatimrelevance");
+      if (dict_val->GetInteger(kGoogleVerbatimRelevance, &verbatim_relevance_))
+        has_verbatim_relevance_ = true;
     }
   }
 
@@ -532,6 +591,11 @@ bool SearchProvider::ParseSuggestResults(Value* root_val,
     if (!suggestion_str.length())
       continue;
 
+    bool has_suggested_relevance = false;
+    int relevance = 0;
+    if (relevance_list && relevance_list->GetInteger(i, &relevance))
+      has_suggested_relevance = true;
+
     Value* type_val;
     std::string type_str;
     if (type_list && type_list->Get(i, &type_val) &&
@@ -549,14 +613,19 @@ bool SearchProvider::ParseSuggestResults(Value* root_val,
         GURL result_url(URLFixerUpper::FixupURL(UTF16ToUTF8(suggestion_str),
                                                 std::string()));
         if (result_url.is_valid()) {
-          navigation_results.push_back(NavigationResult(result_url, site_name));
+          navigation_results.push_back(NavigationResult(result_url, site_name,
+                                                        has_suggested_relevance,
+                                                        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)
-        suggest_results->push_back(suggestion_str);
+      if (suggest_results->size() < kMaxMatches) {
+        suggest_results->push_back(SuggestResult(suggestion_str,
+                                                 has_suggested_relevance,
+                                                 relevance));
+      }
     }
   }
 
@@ -577,11 +646,14 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
         TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
         TemplateURLRef::NO_SUGGESTION_CHOSEN;
   if (providers_.valid_default_provider()) {
-    AddMatchToMap(input_.text(), input_.text(),
-                  CalculateRelevanceForWhatYouTyped(),
-                  AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
-                  did_not_accept_default_suggestion, false,
-                  input_.prevent_inline_autocomplete(), &map);
+    int verbatim_relevance = CalculateRelevanceForWhatYouTyped();
+    if (verbatim_relevance > 0) {
+      AddMatchToMap(input_.text(), input_.text(), verbatim_relevance,
+                    AutocompleteMatch::SEARCH_WHAT_YOU_TYPED,
+                    did_not_accept_default_suggestion, false,
+                    input_.prevent_inline_autocomplete(), &map);
+    }
+    // TODO(msw): Should verbatim_relevance affect and/or supress this result?
     if (!default_provider_suggest_text_.empty()) {
       AddMatchToMap(input_.text() + default_provider_suggest_text_,
                     input_.text(), CalculateRelevanceForWhatYouTyped() + 1,
@@ -602,13 +674,20 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
                          did_not_accept_default_suggestion, &map);
 
   // Now add the most relevant matches from the map to |matches_|.
-  matches_.clear();
+  // TODO(msw): Clear the old matches now?? (only if there are new ones to add?)
+  if (!map.empty())
+    matches_.clear();
+  // TODO(msw): Merge matches from_previous = true?

[Peter Kasting, 2012/05/04 00:11:33]

So remember that if you merge previous matches, you need to re-construct them at
least insofar as the inline autocomplete offset goes -- this is maybe why you
have stale autocompletion issues today.  If you're currently storing the full
old AutocompleteMatches, I'd just store the Suggestions (or whatever the data
structures are called) and reconstruct the matches here

[msw, 2012/05/04 09:43:40]

Done.

+  //            (I think removing and re-adding any repeats should be okay...)
   for (MatchMap::const_iterator i(map.begin()); i != map.end(); ++i)
     matches_.push_back(i->second);
 
+  // TODO(msw): Need additional logic to preserve the navigational matches from
+  //            StartOrStopSuggestQuery cleanup through the ClearResults calls?
   AddNavigationResultsToMatches(keyword_navigation_results_, true);
   AddNavigationResultsToMatches(default_navigation_results_, false);
 
+  // TODO(msw): Additional logic to only +1 if we have a what-you-typed?

[Peter Kasting, 2012/05/04 00:11:33]

Yeah

[msw, 2012/05/04 09:43:40]

Done.

   const size_t max_total_matches = kMaxMatches + 1;  // 1 for "what you typed"
   std::partial_sort(matches_.begin(),
       matches_.begin() + std::min(max_total_matches, matches_.size()),
@@ -628,10 +707,8 @@ void SearchProvider::AddNavigationResultsToMatches(
     // TODO(kochi): http://b/1170574  We add only one results for navigational
     // suggestions. If we can get more useful information about the score,
     // consider adding more results.
-    const size_t num_results = is_keyword ?
-        keyword_navigation_results_.size() : default_navigation_results_.size();
     matches_.push_back(NavigationToMatch(navigation_results.front(),
-        CalculateRelevanceForNavigation(num_results, 0, is_keyword),
+        CalculateRelevanceForNavigation(navigation_results, 0, is_keyword),
         is_keyword));
   }
 }
@@ -741,9 +818,9 @@ void SearchProvider::AddSuggestResultsToMap(
     int did_not_accept_suggestion,
     MatchMap* map) {
   for (size_t i = 0; i < suggest_results.size(); ++i) {
-    AddMatchToMap(suggest_results[i],
+    AddMatchToMap(suggest_results[i].suggestion,
                   is_keyword ? keyword_input_text_ : input_.text(),
-                  CalculateRelevanceForSuggestion(suggest_results.size(), i,
+                  CalculateRelevanceForSuggestion(suggest_results, i,
                                                   is_keyword),
                   AutocompleteMatch::SEARCH_SUGGEST,
                   static_cast<int>(i), is_keyword,
@@ -752,6 +829,14 @@ void SearchProvider::AddSuggestResultsToMap(
 }
 
 int SearchProvider::CalculateRelevanceForWhatYouTyped() const {
+  // TODO(msw): Effect just the exact SEARCH_WHAT_YOU_TYPED match (line 624) or
+  //            also inline-autocomplete from FinalizeInstantQuery and
+  //            default_provider_suggest_text_???

[Peter Kasting, 2012/05/04 00:11:33]

It's not at all clear to me how instant is supposed to interact with these
experiments.  You should probably get the suggest team to clarify, maybe for now
they'll just say to turn the experiment off for instant users?

[msw, 2012/05/04 09:43:40]

I have a TODO on the CR & in the CL, will e-mail/ask tomorrow.

+
+  // TODO(msw): Do not demote on just_deleted_text_/prevent_inline_autocomplete?

[Peter Kasting, 2012/05/04 00:11:33]

I think your code does this currently and is correct to do so

[msw, 2012/05/04 09:43:40]

Cool, thanks for the confirmation.

+  if (has_verbatim_relevance_ && !input_.prevent_inline_autocomplete())
+    return verbatim_relevance_;
+
   if (providers_.valid_keyword_provider())
     return 250;
 
@@ -808,27 +893,33 @@ int SearchProvider::CalculateRelevanceForHistory(
   return std::max(0, base_score - score_discount);
 }
 
-int SearchProvider::CalculateRelevanceForSuggestion(size_t num_results,
-                                                    size_t result_number,
-                                                    bool is_keyword) const {
-  DCHECK(result_number < num_results);
+int SearchProvider::CalculateRelevanceForSuggestion(
+    const SuggestResults& results,
+    size_t result_number,
+    bool is_keyword) const {
+  DCHECK(result_number < results.size());
+  if (results[result_number].has_suggested_relevance)
+    return results[result_number].relevance;
+
   int base_score;
   if (!providers_.is_primary_provider(is_keyword))
     base_score = 100;
   else
     base_score = (input_.type() == AutocompleteInput::URL) ? 300 : 600;
   return base_score +
-      static_cast<int>(num_results - 1 - result_number);
+      static_cast<int>(results.size() - 1 - result_number);
 }
 
-int SearchProvider::CalculateRelevanceForNavigation(size_t num_results,
-                                                    size_t result_number,
-                                                    bool is_keyword) const {
-  DCHECK(result_number < num_results);
-  // TODO(kochi): http://b/784900  Use relevance score from the NavSuggest
-  // server if possible.
+int SearchProvider::CalculateRelevanceForNavigation(
+    const NavigationResults& results,
+    size_t result_number,
+    bool is_keyword) const {
+  DCHECK(result_number < results.size());
+  if (results[result_number].has_suggested_relevance)
+    return results[result_number].relevance;
+
   return (providers_.is_primary_provider(is_keyword) ? 800 : 150) +
-      static_cast<int>(num_results - 1 - result_number);
+      static_cast<int>(results.size() - 1 - result_number);
 }
 
 void SearchProvider::AddMatchToMap(const string16& query_string,
@@ -959,7 +1050,7 @@ AutocompleteMatch SearchProvider::NavigationToMatch(
   match.fill_into_edit.append(
       AutocompleteInput::FormattedStringWithEquivalentMeaning(navigation.url,
                                                               match.contents));
-  // TODO(pkasting): http://b/1112879 These should perhaps be
+  // TODO(pkasting|msw): http://b/1112879 These should perhaps be
   // inline-autocompletable?
 
   return match;