[AiS] Use top local result for prefetch.

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 c2064838797c5c59c9e1d11b0a7a95885a3fc23d..f26c4a1c434b51808b58817b50072f350f4f2b77 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -246,7 +246,17 @@ void SearchProvider::Start(const AutocompleteInput& input,
   input_ = input;
 
   DoHistoryQuery(minimal_changes);
-  DoAnswersQuery(input);
+  // Answers needs the scored history results before any suggest query has been

[Mark P, 2014/09/05 22:18:16]

nit: omit "the"

[groby-ooo-7-16, 2014/09/10 23:21:32]

Done.

+  // started.

[Mark P, 2014/09/05 22:18:15]

Please explain why, either here or elsewhere if there's a better place.

[groby-ooo-7-16, 2014/09/10 23:21:37]

Done.

+  if (OmniboxFieldTrial::EnableAnswersInSuggest()) {

[groby-ooo-7-16, 2014/08/28 21:36:08]

We could _always_ do the scoring after DoHistoryQuery - but that might impact
UMA timing. 

Related, let me repost a previous comment: Do we want a histogram just for the
scoring times to see how much impact on AddHistoryResultsTime this has?

[Mark P, 2014/09/05 22:18:16]

I like your approach in this changelist.
I'm not concerned.  If you want to add it, it's fine with me.  I'm not asking
you to add it.

[groby-ooo-7-16, 2014/09/10 23:21:32]

Acknowledged.

+    ScoreHistoryResultsMultiWord(
+        default_history_results_, false, &scored_default_history_results_);
+    ScoreHistoryResultsMultiWord(
+        keyword_history_results_, true, &scored_keyword_history_results_);
+
+    prefetch_data_ = FindAnswersPrefetchData(input.text());
+  }

[Mark P, 2014/09/05 22:18:15]

Consider:
else { clear all relevant data }

[groby-ooo-7-16, 2014/09/10 23:21:33]

Done.

+
   StartOrStopSuggestQuery(minimal_changes);
   UpdateMatches();
 }
@@ -881,40 +891,39 @@ void SearchProvider::AddHistoryResultsToMap(const HistoryResults& results,
     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 any impact on timing measurements.

[Mark P, 2014/09/05 22:18:16]

The reason for this is not "to prevent any impact on timing measurements",
rather it's to prevent performance regressions from moving the scoring code
before the suggest request is sent.

[groby-ooo-7-16, 2014/09/10 23:21:33]

Done.

+  // Answers FieldTrials will instead use scoring results obtained previously.

[Mark P, 2014/09/05 22:18:16]

nit: "Answers FieldTrials" is awkward.  Consider replacing the sentence. 
Something like "For users with Answers enabled, the history results will be
scored earlier."

[groby-ooo-7-16, 2014/09/10 23:21:38]

Done.

+  SearchSuggestionParser::SuggestResults local_scored_results;
+  const SearchSuggestionParser::SuggestResults* scored_results = NULL;
+  if (!OmniboxFieldTrial::EnableAnswersInSuggest()) {

[groby-ooo-7-16, 2014/08/28 21:36:08]

Another repost: Do we want to run unit tests both with and without Answers
enabled?

[Mark P, 2014/09/05 22:18:16]

It's up to you.

My typical strategy when I add a field trial:
* add test(s) if necessary that specifically exercise the new code.
* run all tests in a local build that has the field trial on for everyone
I don't bother duplicating tests for field-trial-on and field-trial-off unless
they should/do behave differently under those cases.

[groby-ooo-7-16, 2014/09/10 23:21:33]

They _shouldn't_ behave differently except for Answers, which is covered. I'll
give the local run a spin.

+    ScoreHistoryResultsMultiWord(results, is_keyword, &local_scored_results);
+    scored_results = &local_scored_results;
+  } else {
+    // TODO(groby): Once answers is on by default, just pass in scored results.
+    scored_results = is_keyword ? &scored_keyword_history_results_
+                                : &scored_default_history_results_;
   }
-  if (scored_results.empty())
-    scored_results = ScoreHistoryResults(results, prevent_inline_autocomplete,
-                                         input_multiple_words, input_text,
-                                         is_keyword);
+  DCHECK(scored_results);
+  AddScoredHistoryResultsToMap(*scored_results, did_not_accept_suggestion, map);
+  UMA_HISTOGRAM_TIMES("Omnibox.SearchProvider.AddHistoryResultsTime",
+                      base::TimeTicks::Now() - start_time);
+}
+
+// TODO(groby): Can this be merged with AddSuggestResultsToMap?

[groby-ooo-7-16, 2014/08/28 21:36:08]

I don't think it can because AddSuggestResults because it sets
|accepted_suggestion| to the index of the result, while AddHistoryResults sets
the index to true. (Which, BTW, is odd. Is that intended?)

[Mark P, 2014/09/05 22:18:15]

You're reading this wrong.  It sets |mark_as_deletable| to true, which is
accurate.

The |accepted_suggestion| variable is set to |did_not_accept_suggestion|, which
seems appropriate for a suggestion that didn't come from the server.  (This
|accepted_suggestion| variable is used to send feedback to the server.)

You can try combining these two functions into one.  I'm not sure it's worth the
time.

[groby-ooo-7-16, 2014/09/10 23:21:37]

Skipping the merging, then.

+void SearchProvider::AddScoredHistoryResultsToMap(
+    const SearchSuggestionParser::SuggestResults& results,
+    int did_not_accept_suggestion,
+    MatchMap* map) {
+  if (results.empty())

[Mark P, 2014/09/05 22:18:16]

I'm not sure the results.empty() short-circuit is worth anything.  Please remove
it.

[groby-ooo-7-16, 2014/09/10 23:21:37]

Done.

+    return;
+
   for (SearchSuggestionParser::SuggestResults::const_iterator i(
-           scored_results.begin()); i != scored_results.end(); ++i) {
+           results.begin()); i != 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(
@@ -1029,6 +1038,42 @@ SearchSuggestionParser::SuggestResults SearchProvider::ScoreHistoryResults(
   return scored_results;
 }
 
+void SearchProvider::ScoreHistoryResultsMultiWord(
+    const HistoryResults& results,
+    bool is_keyword,
+    SearchSuggestionParser::SuggestResults* scored_results) {
+  if (results.empty())

[Mark P, 2014/09/05 22:18:16]

Shouldn't we clear scored_results in this case?

[groby-ooo-7-16, 2014/09/10 23:21:33]

Done.

+    return;
+
+  bool prevent_inline_autocomplete =
+      input_.prevent_inline_autocomplete() ||

[Mark P, 2014/09/05 22:18:16]

nit: can't this go on the previous line?

[groby-ooo-7-16, 2014/09/10 23:21:37]

It can. I blame git cl format.

+      (input_.type() == metrics::OmniboxInputType::URL);
+  const base::string16& input_text =
+      is_keyword ? keyword_input_.text() : input_.text();

[Mark P, 2014/09/05 22:18:15]

Consider GetInput(is_keyword).text()

[groby-ooo-7-16, 2014/09/10 23:21:38]

Done.

+  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())

[Mark P, 2014/09/05 22:18:15]

nit: { }

[groby-ooo-7-16, 2014/09/10 23:21:33]

Done.

+    *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,
@@ -1267,6 +1312,27 @@ 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.

[groby-ooo-7-16, 2014/08/28 21:36:08]

Re-use of the partial map extends the state the SearchProvider carries around,
but it seems possible - except that it slightly messes with "prefer items added
first". This only affects verbatim matches, AFAICT, which should already be
scored sufficiently high.

[Mark P, 2014/09/05 22:18:16]

I'm not sure I follow your proposal here.

Note that verbatim matches may vary in score nowadays.  They can score as low as
851.  Lots of things will beat them.

[groby-ooo-7-16, 2014/09/10 23:21:35]

We're building the MatchMap twice - once here, once in
ConvertResultsToAutocompleteMatches. The latter adds more than the history
results, though. After looking at this a bit more, sharing the map carries more
complexity than I'd like to add, so discarding that.

The alternate proposal - "scan through results" would just sort the scored
results, without building a MatchMap. That relies on AddMatchToMap never
modifying the score, and would require re-implementing the logic in
AutocompleteMatch::MoreRelevant - so I'm discarding that too, for now.

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

Acknowledged.

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

[groby-ooo-7-16, 2014/08/28 21:36:08]

See question above.

If I share state, I'll obviously need to pass in did_not_accept_suggestion.

[groby-ooo-7-16, 2014/08/28 21:36:08]

See question above. 

This might also be relevant when I share state as discussed above. The question
is, is did_not_accept_suggestion available before all results are int?

[Mark P, 2014/09/05 22:18:15]

No.  In fact, the value of did_not_accept_suggestion depends on whether results
are in.
  int did_not_accept_default_suggestion =
      default_results_.suggest_results.empty() ?
      TemplateURLRef::NO_SUGGESTIONS_AVAILABLE :
      TemplateURLRef::NO_SUGGESTION_CHOSEN;
https://code.google.com/p/chromium/codesearch#chromium/src/components/omnibox...

[Mark P, 2014/09/05 22:18:16]

ditto: I'm not sure I follow your proposal here.

[groby-ooo-7-16, 2014/09/10 23:21:33]

Shorter: Does it matter that AddTransformed...() is passed
TemplateURLRef::NO_SUGGESTIONS_AVAILABLE for |did_not_accept_suggestions|?

As far as I can tell, it does not, and it correctly reflects that we don't have
suggestions at this point, so I'll go with that.

[groby-ooo-7-16, 2014/09/10 23:21:33]

Acknowledged.

+  AddScoredHistoryResultsToMap(scored_keyword_history_results_, false, &map);

[Mark P, 2014/09/05 22:18:16]

Please do not pass false for an int without an explicit cast.

Also, more importantly, false is likely the wrong value, regardless.  Please use
the right value.  Note that given my other comment, you might not be able to
have the right value at this point.  I should figure out if it's important to
have the right value in this variable.  If so, you need to correct its value
later.  If not, you should comment here about how it doesn't matter.  (And
perhaps comment in the .h by this function and possibly in the .h by the
internal variable that gets assigned this as a result that this particular field
is set incorrectly.)

[groby-ooo-7-16, 2014/09/10 23:21:33]

It is indeed - thanks for catching this. I read the prototype wrong. 
I can't correct it later - the MatchMap is scoped to this function.
This function discards everything in MatchMap, so nothing is assigned an
incorrect result. I've modified AddTransformedHistoryResultsToMap so that it
does not affect any members, but takes a pointer to modified objects.

+  AddScoredHistoryResultsToMap(scored_default_history_results_, false, &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);
+
+  // If there is a top scoring entry, find the corresponding answer.
+  if (!matches.empty())
+    return answers_cache_.GetTopAnswerEntry(matches[0].contents);
+
+  return AnswersQueryData();
 }