Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/autocomplete_result.cc

Index: chrome/browser/autocomplete/autocomplete_result.cc
diff --git a/chrome/browser/autocomplete/autocomplete_result.cc b/chrome/browser/autocomplete/autocomplete_result.cc
index 58a19c98b6bcbb4720141f66a1f0a0242b9b2706..55ff101eaa943dca428c749a3351cdc0ea098197 100644
--- a/chrome/browser/autocomplete/autocomplete_result.cc
+++ b/chrome/browser/autocomplete/autocomplete_result.cc
@@ -10,6 +10,7 @@
 #include "base/logging.h"
 #include "chrome/browser/autocomplete/autocomplete_input.h"
 #include "chrome/browser/autocomplete/autocomplete_match.h"
+#include "chrome/browser/omnibox/omnibox_field_trial.h"
 
 // static
 const size_t AutocompleteResult::kMaxMatches = 6;
@@ -21,7 +22,9 @@ void AutocompleteResult::Selection::Clear() {
   is_history_what_you_typed_match = false;
 }
 
-AutocompleteResult::AutocompleteResult() {
+AutocompleteResult::AutocompleteResult()
+    : reorder_for_legal_default_match_(
+          OmniboxFieldTrial::InReorderForLegalDefaultMatchGroup()) {
   // Reserve space for the max number of matches we'll show.
   matches_.reserve(kMaxMatches);
 
@@ -102,9 +105,27 @@ void AutocompleteResult::SortAndCull(const AutocompleteInput& input,
   const size_t num_matches = std::min(kMaxMatches, matches_.size());
   std::partial_sort(matches_.begin(), matches_.begin() + num_matches,
                     matches_.end(), &AutocompleteMatch::MoreRelevant);
+  if ((begin() != end()) && !begin()->allowed_to_be_default_match &&

[Peter Kasting, 2013/07/17 18:29:48]

Nit: First condition should be "!empty()"

[Mark P, 2013/07/17 20:39:18]

Done.  Note that given the code below, I can remove the whole first line of this
if clause if we want.  It depends on whether you think it's clearer or faster
with these parts of the test or without.

[Peter Kasting, 2013/07/17 22:12:30]

I would leave these here.  I think it's slightly clearer with them in place.

I would use "begin() + 1" as the initial value in the loop in that case, since
these tests guarantee that's both safe and correct.

[Mark P, 2013/07/17 23:50:45]

Done.

+      reorder_for_legal_default_match_) {
+    // Top match is not allowed to be the default match.  Find the most
+    // relevant legal match and shift it to the front.
+    AutocompleteResult::iterator best_legal_default_match = end();
+    for (AutocompleteResult::iterator it = begin(); it != end(); ++it) {
+      if (it->allowed_to_be_default_match &&
+          ((best_legal_default_match == end()) ||
+           AutocompleteMatch::MoreRelevant(*it, *best_legal_default_match))) {
+        best_legal_default_match = it;
+      }
+    }
+    DCHECK(best_legal_default_match != end());

[Peter Kasting, 2013/07/17 18:29:48]

Our existing code probably violates this, so be careful.

For example, if I type a '?' alone, I probably only get back the "placeholder
result" from the search provider -- which isn't currently "allowed to be
default".  (The fix there is presumably to set the "allowed to be default" flag
on that match.)

There may be other places where people weren't setting any inline autocomplete
offset but we might need to allow something to be default.  Auditing every
construction of AutocompleteMatch carefully is probably the only way to find
them :(

[Mark P, 2013/07/17 20:39:18]

Yes, I know.  I added the DCHECK in hope that it would help me iron out all the
kinks as in the placeholder one you found below.
Did that.
I audited every provider and within them searched for "fill_into_edit".  I
thought that would find me every match.  Apparently it made me miss the
placeholder one (no fill_into_edit oddly).  I just did more searches and I think
that's the only one I missed.

+    std::rotate(matches_.begin(), best_legal_default_match,
+                best_legal_default_match + 1);
+  }
   matches_.resize(num_matches);
 
   default_match_ = begin();
+  DCHECK((default_match_ == end()) ||
+         default_match_->allowed_to_be_default_match);

[Peter Kasting, 2013/07/17 18:29:48]

Nit: Indent 4, not even

[Mark P, 2013/07/17 20:39:18]

Done.

 
   // Set the alternate nav URL.
   alternate_nav_url_ = (default_match_ == end()) ?