[AiS] Use top local result for prefetch.

components/omnibox/search_provider.cc

Index: components/omnibox/search_provider.cc
diff --git a/components/omnibox/search_provider.cc b/components/omnibox/search_provider.cc
index 41763048fb52a74899b507382e64b6f902d82fae..e841d3a33238d7e5f3973b3cf033d0576e841896 100644
--- a/components/omnibox/search_provider.cc
+++ b/components/omnibox/search_provider.cc
@@ -265,7 +265,23 @@ void SearchProvider::Start(const AutocompleteInput& input,
   input_ = input;
 
   DoHistoryQuery(minimal_changes);
-  DoAnswersQuery(input);
+  // Answers needs scored history results before any suggest query has been
+  // started, since the query for answer-bearing results needs additional
+  // prefetch information based on the highest-scored local history result.
+  if (OmniboxFieldTrial::EnableAnswersInSuggest()) {
+    ScoreHistoryResultsMultiWord(raw_default_history_results_,
+                                 false,
+                                 &transformed_default_history_results_);
+    ScoreHistoryResultsMultiWord(raw_keyword_history_results_,
+                                 true,
+                                 &transformed_keyword_history_results_);
+
+    prefetch_data_ = FindAnswersPrefetchData(input.text());
+  } else {
+    transformed_default_history_results_.clear();
+    transformed_keyword_history_results_.clear();
+  }
+
   StartOrStopSuggestQuery(minimal_changes);
   UpdateMatches();
 }
@@ -519,8 +535,8 @@ void SearchProvider::DoHistoryQuery(bool minimal_changes) {
   if (minimal_changes)
     return;
 
-  keyword_history_results_.clear();
-  default_history_results_.clear();
+  raw_keyword_history_results_.clear();
+  raw_default_history_results_.clear();
 
   if (OmniboxFieldTrial::SearchHistoryDisable(
       input_.current_page_classification()))
@@ -533,7 +549,7 @@ void SearchProvider::DoHistoryQuery(bool minimal_changes) {
   // Request history for both the keyword and default provider.  We grab many
   // more matches than we'll ultimately clamp to so that if there are several
   // recent multi-word matches who scores are lowered (see
-  // AddHistoryResultsToMap()), they won't crowd out older, higher-scoring
+  // ScoreHistoryResultsMultiWord()), they won't crowd out older, higher-scoring
   // matches.  Note that this doesn't fix the problem entirely, but merely
   // limits it to cases with a very large number of such multi-word matches; for
   // now, this seems OK compared with the complexity of a real fix, which would
@@ -543,8 +559,10 @@ void SearchProvider::DoHistoryQuery(bool minimal_changes) {
   const TemplateURL* default_url = providers_.GetDefaultProviderURL();
   if (default_url) {
     const base::TimeTicks start_time = base::TimeTicks::Now();
-    url_db->GetMostRecentKeywordSearchTerms(default_url->id(), input_.text(),
-        num_matches, &default_history_results_);
+    url_db->GetMostRecentKeywordSearchTerms(default_url->id(),
+                                            input_.text(),
+                                            num_matches,
+                                            &raw_default_history_results_);
     UMA_HISTOGRAM_TIMES(
         "Omnibox.SearchProvider.GetMostRecentKeywordTermsDefaultProviderTime",
         base::TimeTicks::Now() - start_time);
@@ -552,7 +570,9 @@ void SearchProvider::DoHistoryQuery(bool minimal_changes) {
   const TemplateURL* keyword_url = providers_.GetKeywordProviderURL();
   if (keyword_url) {
     url_db->GetMostRecentKeywordSearchTerms(keyword_url->id(),
-        keyword_input_.text(), num_matches, &keyword_history_results_);
+                                            keyword_input_.text(),
+                                            num_matches,
+                                            &raw_keyword_history_results_);
   }
 }
 
@@ -844,10 +864,14 @@ void SearchProvider::ConvertResultsToAutocompleteMatches() {
       }
     }
   }
-  AddHistoryResultsToMap(keyword_history_results_, true,
-                         did_not_accept_keyword_suggestion, &map);
-  AddHistoryResultsToMap(default_history_results_, false,
-                         did_not_accept_default_suggestion, &map);
+  AddRawHistoryResultsToMap(raw_keyword_history_results_,
+                            true,
+                            did_not_accept_keyword_suggestion,
+                            &map);
+  AddRawHistoryResultsToMap(raw_default_history_results_,
+                            false,
+                            did_not_accept_default_suggestion,
+                            &map);
 
   AddSuggestResultsToMap(keyword_results_.suggest_results,
                          keyword_results_.metadata, &map);
@@ -937,48 +961,50 @@ void SearchProvider::AddNavigationResultsToMatches(
   }
 }
 
-void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
-                                            bool is_keyword,
-                                            int did_not_accept_suggestion,
-                                            MatchMap* map) {
-  if (results.empty())
+void SearchProvider::AddRawHistoryResultsToMap(
+    const HistoryResults& raw_results,
+    bool is_keyword,
+    int did_not_accept_suggestion,
+    MatchMap* map) {
+  if (raw_results.empty())
     return;
 
   base::TimeTicks start_time(base::TimeTicks::Now());
-  bool prevent_inline_autocomplete = input_.prevent_inline_autocomplete() ||
-      (input_.type() == metrics::OmniboxInputType::URL);
-  const base::string16& input_text =
-      is_keyword ? keyword_input_.text() : input_.text();
-  bool input_multiple_words = HasMultipleWords(input_text);
 
-  SearchSuggestionParser::SuggestResults scored_results;
-  if (!prevent_inline_autocomplete && input_multiple_words) {
-    // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit
-    // queries if the input also has multiple words.  But if we were already
-    // scoring a multi-word, multi-visit query aggressively, and the current
-    // input is still a prefix of it, then changing the suggestion suddenly
-    // feels wrong.  To detect this case, first score as if only one word has
-    // been typed, then check if the best result came from aggressive search
-    // history scoring.  If it did, then just keep that score set.  This
-    // 1200 the lowest possible score in CalculateRelevanceForHistory()'s
-    // aggressive-scoring curve.
-    scored_results = ScoreHistoryResults(results, prevent_inline_autocomplete,
-                                         false, input_text, is_keyword);
-    if ((scored_results.front().relevance() < 1200) ||
-        !HasMultipleWords(scored_results.front().suggestion()))
-      scored_results.clear();  // Didn't detect the case above, score normally.
+  // Until Answers becomes default, scoring of history results will still happen
+  // here for non-Answers Chrome, to prevent scoring regressions from moving the

[Mark P, 2014/09/16 22:24:55]

nit: ^scoring^performance
(I think)

If you are worried about scoring regressions, please tell me why.  (I don't
think there could be any.)

[groby-ooo-7-16, 2014/09/18 23:27:24]

performance regressions, indeed. 

+  // scoring code before the suggest request is sent.
+  // For users with Answers enabled, the history results have already been
+  // scored earlier, right after calling DoHistoryQuery().
+  SearchSuggestionParser::SuggestResults local_transformed_results;
+  const SearchSuggestionParser::SuggestResults* transformed_results = NULL;
+  if (!OmniboxFieldTrial::EnableAnswersInSuggest()) {
+    ScoreHistoryResultsMultiWord(
+        raw_results, is_keyword, &local_transformed_results);
+    transformed_results = &local_transformed_results;
+  } else {
+    // TODO(groby): Once answers is on by default, just pass in scored results.
+    transformed_results = is_keyword ? &transformed_keyword_history_results_

[Mark P, 2014/09/16 22:24:55]

I'd prefer if you could add somewhere (possibly here) that, before using
transformed_history_results, munges them to revise whether suggestions are
available at this instant.  Otherwise when answers is enabled all our history
results will be annotated with "no suggestions available", even if suggestions
are available.

[groby-ooo-7-16, 2014/09/18 23:27:24]

I'm not sure I understand why results would be annotated improperly. Unless I
misunderstand, the annotation should ultimately be stored stored in the
AutoCompleteMatch's |search_term_args.accepted_suggestion|, correct?

Here is what happens when Answers is enabled:

1) Right after DoHistoryQuery, transformed_..._results is populated. As far as I
can tell, "suggestion available" doesn't matter when populating SuggestResults

2) This function - AddRawHistoryResultsToMap() - is called from
ConvertResultsToAutocompleteMatches. |did_not_accept_suggestions| is passed in
from there, and is computed there:
 int did_not_accept_default_suggestion =
      default_results_.suggest_results.empty() ?
      TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
      TemplateURLRef::NO_SUGGESTION_CHOSEN;

3) AddTransformedHistoryResultsToMap() calls AddMatchToMap(), passing through
|did_not_accept_suggestion|.

4) AddMatchToMap converts the SuggestResult into an AutocompleteMatch, once more
passing through |did_not_accept_suggestion|, using it to set
|match.search_terms_args->accepted_suggestion|

[Mark P, 2014/09/19 18:45:36]

Thank you for the explanation.  I see my worry was incorrect.  I didn't notice
that we pass through a newly-computed |did_not_accept_suggestion| when we add
the transformed results to the map.  (I was thinking that we used the same value
we used when we computed the transformed results in the first place.)

+                                     : &transformed_default_history_results_;
   }
-  if (scored_results.empty())
-    scored_results = ScoreHistoryResults(results, prevent_inline_autocomplete,
-                                         input_multiple_words, input_text,
-                                         is_keyword);
+  DCHECK(transformed_results);
+  AddTransformedHistoryResultsToMap(
+      *transformed_results, did_not_accept_suggestion, map);
+  UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
+                      base::TimeTicks::Now() - start_time);
+}
+
+void SearchProvider::AddTransformedHistoryResultsToMap(
+    const SearchSuggestionParser::SuggestResults& transformed_results,
+    int did_not_accept_suggestion,
+    MatchMap* map) {
   for (SearchSuggestionParser::SuggestResults::const_iterator i(
-           scored_results.begin()); i != scored_results.end(); ++i) {
+           transformed_results.begin());
+       i != transformed_results.end();
+       ++i) {
     AddMatchToMap(*i, std::string(), did_not_accept_suggestion, true,
                   providers_.GetKeywordProviderURL() != NULL, map);
   }
-  UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
-                      base::TimeTicks::Now() - start_time);
 }
 
 SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
@@ -1092,6 +1118,46 @@ SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
   return scored_results;
 }
 
+void SearchProvider::ScoreHistoryResultsMultiWord(
+    const HistoryResults& results,
+    bool is_keyword,
+    SearchSuggestionParser::SuggestResults* scored_results) {
+  DCHECK(scored_results);
+  if (results.empty()) {
+    scored_results->clear();
+    return;
+  }
+
+  bool prevent_inline_autocomplete = input_.prevent_inline_autocomplete() ||
+      (input_.type() == metrics::OmniboxInputType::URL);
+  const base::string16& input_text = GetInput(is_keyword).text();
+  bool input_multiple_words = HasMultipleWords(input_text);
+
+  if (!prevent_inline_autocomplete && input_multiple_words) {
+    // ScoreHistoryResults() allows autocompletion of multi-word, 1-visit
+    // queries if the input also has multiple words.  But if we were already
+    // scoring a multi-word, multi-visit query aggressively, and the current
+    // input is still a prefix of it, then changing the suggestion suddenly
+    // feels wrong.  To detect this case, first score as if only one word has
+    // been typed, then check if the best result came from aggressive search
+    // history scoring.  If it did, then just keep that score set.  This
+    // 1200 the lowest possible score in CalculateRelevanceForHistory()'s
+    // aggressive-scoring curve.
+    *scored_results = ScoreHistoryResults(
+        results, prevent_inline_autocomplete, false, input_text, is_keyword);
+    if ((scored_results->front().relevance() < 1200) ||
+        !HasMultipleWords(scored_results->front().suggestion()))
+      scored_results->clear();  // Didn't detect the case above, score normally.
+  }
+  if (scored_results->empty()) {
+    *scored_results = ScoreHistoryResults(results,
+                                          prevent_inline_autocomplete,
+                                          input_multiple_words,
+                                          input_text,
+                                          is_keyword);
+  }
+}
+
 void SearchProvider::AddSuggestResultsToMap(
     const SearchSuggestionParser::SuggestResults& results,
     const std::string& metadata,
@@ -1333,6 +1399,34 @@ void SearchProvider::RegisterDisplayedAnswers(
   answers_cache_.UpdateRecentAnswers(match->fill_into_edit, match->answer_type);
 }
 
-void SearchProvider::DoAnswersQuery(const AutocompleteInput& input) {
-  prefetch_data_ = answers_cache_.GetTopAnswerEntry(input.text());
+AnswersQueryData SearchProvider::FindAnswersPrefetchData(
+    const base::string16& input) {
+  // Retrieve the top entry from scored history results.
+
+  // TODO(groby): This seems a bit of a waste. Figure out if we can either
+  // re-use a partially populated map, or if we should just scan through the
+  // results, duplicates be damned.

[Mark P, 2014/09/16 22:24:55]

Given that you've discarded this idea (fine by me), perhaps you want to remove
the TODO?

[groby-ooo-7-16, 2014/09/18 23:27:24]

Done.

+  MatchMap map;
+  // TODO(groby): Can I just default to marking suggestions as "not accepted"?
+  // This seems to only matter for AQS....

[Mark P, 2014/09/16 22:24:55]

I'm comfortable with this here.  As I understand it, we only use the results of
this AddTransformedHistoryResultsToMap() call to calculate prefetch_data, and we
never look at the suggestions_available field in prefetch_data.  Please correct
me if I'm wrong.

[groby-ooo-7-16, 2014/09/18 23:27:24]

Correct, so removed comment.

+  AddTransformedHistoryResultsToMap(transformed_keyword_history_results_,
+                                    TemplateURLRef::NO_SUGGESTIONS_AVAILABLE,
+                                    &map);
+  AddTransformedHistoryResultsToMap(transformed_default_history_results_,
+                                    TemplateURLRef::NO_SUGGESTIONS_AVAILABLE,
+                                    &map);
+
+  ACMatches matches;
+  for (MatchMap::const_iterator i(map.begin()); i != map.end(); ++i)
+    matches.push_back(i->second);
+  std::sort(matches.begin(), matches.end(), &AutocompleteMatch::MoreRelevant);
+
+  LOG(ERROR) << "SIZE:" << matches.size();

[Mark P, 2014/09/16 22:24:55]

nit: remove unneeded LOG lines

[groby-ooo-7-16, 2014/09/18 23:27:24]

Done.

+
+  // If there is a top scoring entry, find the corresponding answer.
+  if (!matches.empty() &&
+      StartsWith(matches[0].contents, input, false /* case-insensitive */)) {

[Mark P, 2014/09/16 22:24:55]

Minor question: why this is StartsWith necessary? What's the harm if it's
missing?

[groby-ooo-7-16, 2014/09/18 23:27:24]

By missing you mean leaving the test out completely? As long as history results
are always prefix-matching on input, nothing. Probably just paranoia on my part,
so taking it out.

[Mark P, 2014/09/19 18:45:36]

Yes, search history matches should always be prefix matches.

+    return answers_cache_.GetTopAnswerEntry(matches[0].contents);
+  }
+  return AnswersQueryData();
 }