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;
 }
@@ skipping 69 matching lines @@
     i->ComputeStrippedDestinationURL(profile);
 
   // Remove duplicates.
   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);
-  matches_.resize(num_matches);
+  std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);

[H Fung, 2013/08/06 22:03:17]

Nit: Since you use begin() and end() below, maybe be consistent here?
(I also don't see how the existing sorting function worked since it only sorted
the first num_matches after being sorted based on destination url)

[Mark P, 2013/08/06 22:18:06]

Switched below to matches_ to be consistent with the rest of this function that
I didn't trust.
Look again at the original sorting function AutocompleteMatch::MoreRelevant.  It
sorts by relevance, breaking ties by destination URL.

[H Fung, 2013/08/06 22:53:00]

I meant that AutocompleteMatch::DestinationSortFunc used to sort |matches_|
based on destination URL.  After that, only the first num_matches matches get
partially sorted using AutocompleteMatch::MoreRelevant.  So I'm not sure how a
high-scoring match with a URL like zillow would get sorted properly in this
function.

[Mark P, 2013/08/07 00:44:31]

I think you don't understand what partial_sort does.
http://www.cplusplus.com/reference/algorithm/partial_sort/

It sorts the array enough so that highest X matches are put at the beginning, in
order.

+  if (!empty() && !begin()->allowed_to_be_default_match &&
+      OmniboxFieldTrial::ReorderForLegalDefaultMatch(
+          input.current_page_classification())) {
+    // Top match is not allowed to be the default match.  Find the most
+    // relevance legal match and shift it to the front.
+    for (AutocompleteResult::iterator it = begin() + 1; it != end(); ++it) {
+      if (it->allowed_to_be_default_match) {
+        std::rotate(matches_.begin(), it, it + 1);
+        break;
+      }
+    }
+  }
+  matches_.resize(std::min(kMaxMatches, matches_.size()));
 
   default_match_ = begin();
+  DCHECK((default_match_ == end()) ||

[H Fung, 2013/08/06 22:03:17]

I'm not sure, so feel free to ignore, but maybe empty() might be clearer than
(default_match_ == end())

[Mark P, 2013/08/06 22:18:06]

I can't do
default_match_.empty()
because default_match_ is an iterator, not the underlying vector.

I could do matches_.empty() but I think using default_match_ makes things
clearer that this is an assortment about, well, default_match_.

+      default_match_->allowed_to_be_default_match);
 
   // 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--;
     }
   }
 }