Omnibox: Make the Controller Reorder Matches for Inlining

chrome/browser/autocomplete/autocomplete_result.cc

 // Copyright (c) 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/autocomplete_result.h"
 
 #include <algorithm>
 #include <iterator>
 
 #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;
 const int AutocompleteResult::kLowestDefaultScore = 1200;
 
 void AutocompleteResult::Selection::Clear() {
   destination_url = GURL();
   provider_affinity = NULL;
   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);
 
   // It's probably safe to do this in the initializer list, but there's little
   // penalty to doing it here and it ensures our object is fully constructed
   // before calling member functions.
   default_match_ = end();
 }
 
 AutocompleteResult::~AutocompleteResult() {}
@@ skipping 60 matching lines @@
   std::sort(matches_.begin(), matches_.end(),
             &AutocompleteMatch::DestinationSortFunc);
   matches_.erase(std::unique(matches_.begin(), matches_.end(),
                              &AutocompleteMatch::DestinationsEqual),
                  matches_.end());
 
   // Sort and trim to the most relevant kMaxMatches matches.
   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()) ?
       GURL() : ComputeAlternateNavUrl(input, *default_match_);
 }
 
 bool AutocompleteResult::HasCopiedMatches() const {
   for (ACMatches::const_iterator i(begin()); i != end(); ++i) {
     if (i->from_previous)
       return true;
@@ skipping 130 matching lines @@
        i != old_matches.rend() && delta > 0; ++i) {
     if (!HasMatchByDestination(*i, new_matches)) {
       AutocompleteMatch match = *i;
       match.relevance = std::min(max_relevance, match.relevance);
       match.from_previous = true;
       AddMatch(match);
       delta--;
     }
   }
 }