Omnibox: In Reorder Mode, SearchProvider Doesn't Need to Remove Stale Results

chrome/browser/autocomplete/search_provider.cc

 // Copyright 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.
 
 #include "chrome/browser/autocomplete/search_provider.h"
 
 #include <algorithm>
 #include <cmath>
 
 #include "base/callback.h"
@@ skipping 851 matching lines @@
   keyword_fetcher_.reset();
   default_fetcher_.reset();
 }
 
 void SearchProvider::ClearAllResults() {
   keyword_results_.Clear();
   default_results_.Clear();
 }
 
 void SearchProvider::RemoveAllStaleResults() {
-  // In theory it would be better to run an algorithm like that in
+  // We only need to remove stale results (which ensures the top-scoring
+  // match is inlineable) if the user is not in reorder mode.  In reorder
+  // mode, the autocomplete system will reorder results to make sure the
+  // top result is inlineable.
+  const bool omnibox_will_reorder_for_legal_default_match =
+      OmniboxFieldTrial::ReorderForLegalDefaultMatch(
+          input_.current_page_classification());
+  // In theory it would be better to run the removal algorithm like that in

[Peter Kasting, 2013/11/09 00:06:20]

Nit: I think changing "an" to "the removal" here actually made this less
grammatically correct and harder to understand.

[Mark P, 2013/11/11 18:46:14]

I thought "an algorithm" was vague (which algorithm?), so I changed it
originally.
Changed it back per your request.

   // RemoveStaleResults(...) below that uses all four results lists
   // and both verbatim scores at once.  However, that will be much
   // more complicated for little obvious gain.  For code simplicity
   // and ease in reasoning about the invariants involved, this code
   // removes stales results from the keyword provider and default
   // provider independently.
-  RemoveStaleResults(input_.text(), GetVerbatimRelevance(NULL),
-                     &default_results_.suggest_results,
-                     &default_results_.navigation_results);
+  if (!omnibox_will_reorder_for_legal_default_match) {
+    RemoveStaleResults(input_.text(), GetVerbatimRelevance(NULL),
+                       &default_results_.suggest_results,
+                       &default_results_.navigation_results);
+  }
   if (!keyword_input_.text().empty()) {
-    RemoveStaleResults(keyword_input_.text(), GetKeywordVerbatimRelevance(NULL),
-                       &keyword_results_.suggest_results,
-                       &keyword_results_.navigation_results);
-  } else {
+    if (!omnibox_will_reorder_for_legal_default_match) {
+      RemoveStaleResults(keyword_input_.text(),
+                         GetKeywordVerbatimRelevance(NULL),
+                         &keyword_results_.suggest_results,
+                         &keyword_results_.navigation_results);
+    }
+  } else  {
     // User is either in keyword mode with a blank input or out of
     // keyword mode entirely.
     keyword_results_.Clear();
   }

[Peter Kasting, 2013/11/09 00:06:20]

Tiny nit: Maybe this would match the comments better:

  if (!omnibox_will_reorder_for_legal_default_match) {
    RemoveStaleResults(...);
    if (!keyword_input_.text().empty()) {
      RemoveStaleResults(...);
    }
  }
  if (keyword_input_.text().empty()) {
    ...
  }

[Mark P, 2013/11/11 18:46:14]

Sure, if you want.  I considered both options and couldn't decide which I liked
better.  I ended up going with this one because it only tested
keyword_input_.text().empty() once and I thought maybe that meant it was better
than one that tested it twice.  I'm switching to your suggestion; it's easier to
read (even though for some reason I picked the other one originally).

 }
 
 void SearchProvider::ApplyCalculatedRelevance() {
   ApplyCalculatedSuggestRelevance(&keyword_results_.suggest_results);
   ApplyCalculatedSuggestRelevance(&default_results_.suggest_results);
   ApplyCalculatedNavigationRelevance(&keyword_results_.navigation_results);
   ApplyCalculatedNavigationRelevance(&default_results_.navigation_results);
   default_results_.verbatim_relevance = -1;
   keyword_results_.verbatim_relevance = -1;
 }
@@ skipping 879 matching lines @@
     it->set_relevance(max_query_relevance);
     it->set_relevance_from_server(relevance_from_server);
   }
 }
 
 void SearchProvider::UpdateDone() {
   // We're done when the timer isn't running, there are no suggest queries
   // pending, and we're not waiting on Instant.
   done_ = !timer_.IsRunning() && (suggest_results_pending_ == 0);
 }