Omnibox: Create DemoteByType Experiment

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..5c670bf766dac59f2ed7fe878b5d8daf8ec18c2c 100644
--- a/chrome/browser/autocomplete/autocomplete_result.cc
+++ b/chrome/browser/autocomplete/autocomplete_result.cc
@@ -10,6 +10,55 @@
 #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"
+
+namespace {
+
+// This class implements a special version of AutocompleteMatch::MoreRelevant
+// that allows match of particular types to be demoted in AutocompleteResult.

[Peter Kasting, 2013/08/06 19:33:50]

Nit: match -> matches?  AutocompleteResult -> AutocompleteResult::SortAndCull()?

[Mark P, 2013/08/06 22:00:15]

Done.
Not done because this comparison operator is used in two places in this file,
only one of which is SortAndCull.  The key is that this MoreRelevant should be
used as the comparison function everywhere here, that's why I don't want to name
function(s).

+class CompareWithDemoteByType {
+ public:
+  CompareWithDemoteByType(
+      AutocompleteInput::PageClassification current_page_classification);
+
+  // Returns the relevance score of |match| demoted appropriately by
+  // |demotions_by_type_|.
+  int GetDemotedRelevance(const AutocompleteMatch& match);
+
+  // Comparison function.
+  bool operator()(const AutocompleteMatch& elem1,
+                  const AutocompleteMatch& elem2);
+
+ private:
+  OmniboxFieldTrial::DemotionMultiplierByType demotions_by_type_;
+};
+
+CompareWithDemoteByType::CompareWithDemoteByType(
+    AutocompleteInput::PageClassification current_page_classification) {
+  OmniboxFieldTrial::GetDemotionsByType(current_page_classification,
+                                        &demotions_by_type_);
+}
+
+int CompareWithDemoteByType::GetDemotedRelevance(
+    const AutocompleteMatch& match) {
+  return (demotions_by_type_.find(match.type) != demotions_by_type_.end()) ?
+      match.relevance * demotions_by_type_[match.type] : match.relevance;

[Peter Kasting, 2013/08/06 19:33:50]

Nit: How about:

  OmniboxFieldTrial::DemotionMultiplierByType::const_iterator demotion =
      demotions_by_type_.find(match.type);
  return (demotion == demotions_by_type_.end()) ?
      match.relevance : (match.relevance * *demotion));

This is longer, but it avoids the double-lookup (probably no meaningful perf
impact, but if we've got the iterator anyway...)

[Mark P, 2013/08/06 22:00:15]

Done, with a bit of cleanup.

+}
+
+bool CompareWithDemoteByType::operator()(const AutocompleteMatch& elem1,
+                                         const AutocompleteMatch& elem2) {
+  // Compute demoted relevance scores for each match.
+  const int demoted_relevance1 = GetDemotedRelevance(elem1);
+  const int demoted_relevance2 = GetDemotedRelevance(elem2);
+  // For equal-relevance matches, we sort alphabetically, so that providers
+  // who return multiple elements at the same priority get a "stable" sort
+  // across multiple updates.
+  return (demoted_relevance1 == demoted_relevance2) ?
+      (elem1.contents < elem2.contents) :
+      (demoted_relevance1 > demoted_relevance2);
+}
+
+};  // namespace
 
 // static
 const size_t AutocompleteResult::kMaxMatches = 6;
@@ -67,7 +116,7 @@ void AutocompleteResult::CopyOldMatches(const AutocompleteInput& input,
   old_matches.BuildProviderToMatches(&old_matches_per_provider);
   for (ProviderToMatches::const_iterator i(old_matches_per_provider.begin());
        i != old_matches_per_provider.end(); ++i) {
-    MergeMatchesByProvider(i->second, matches_per_provider[i->first]);
+    MergeMatchesByProvider(input, i->second, matches_per_provider[i->first]);
   }
 
   SortAndCull(input, profile);
@@ -99,9 +148,16 @@ void AutocompleteResult::SortAndCull(const AutocompleteInput& input,
                  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);
+  size_t max_num_matches = std::min(kMaxMatches, matches_.size());
+  CompareWithDemoteByType comparing_object(input.current_page_classification());
+  std::partial_sort(matches_.begin(), matches_.begin() + max_num_matches,
+                    matches_.end(), comparing_object);
+  // In the process of trimming, drop all matches with a demoted relevance
+  // score of 0.
+  size_t num_matches;
+  for (num_matches = 0u; (num_matches < max_num_matches) &&

[Peter Kasting, 2013/08/06 19:33:50]

Nit: Simpler and (to me) clearer:

  matches_.resize(std::count_if(matches_.begin(),
                                matches_.begin() + max_num_matches,
                                comparing_object.GetDemotedRelevance));

(You might have to fiddle with some adapters if the compiler complains)

[Mark P, 2013/08/06 22:00:15]

I took your suggestion in the simplest way I could get it to work.  I'm not sure
if you'll like it.

Rather than the operator() which is in this patchset, I could instead pass in
something like 
std::bind(&CompareWithDemoteByType::GetDemotedRelevance, &comparing_object,
std::placeholders::_1)
as the third parameter to the count_if.

I'm not sure if either of these are better than what I already had.

[Peter Kasting, 2013/08/06 22:19:40]

It's clever, but I think semantically confusing.  Reading "count if (comparing
object)" doesn't really make any sense.
I think std::bind() is C++11, no?  We don't support that (yet) in Chromium.

The only way I can think of to do this is to write another functor that takes a
CompareWithDemoteByType object and provides an operator().  This is probably
uglier than just doing what you had originally, so I'd probably give up.

[Mark P, 2013/08/06 22:44:49]

Okay, reverted back to original code.

+       (comparing_object.GetDemotedRelevance(*match_at(num_matches)) > 0);
+       ++num_matches) {}
   matches_.resize(num_matches);
 
   default_match_ = begin();
@@ -200,13 +256,15 @@ void AutocompleteResult::CopyFrom(const AutocompleteResult& rhs) {
   alternate_nav_url_ = rhs.alternate_nav_url_;
 }
 
-void AutocompleteResult::AddMatch(const AutocompleteMatch& match) {
+void AutocompleteResult::AddMatch(const AutocompleteInput& input,
+                                  const AutocompleteMatch& match) {
   DCHECK(default_match_ != end());
   DCHECK_EQ(AutocompleteMatch::SanitizeString(match.contents), match.contents);
   DCHECK_EQ(AutocompleteMatch::SanitizeString(match.description),
             match.description);
+  CompareWithDemoteByType comparing_object(input.current_page_classification());
   ACMatches::iterator insertion_point =
-      std::upper_bound(begin(), end(), match, &AutocompleteMatch::MoreRelevant);
+      std::upper_bound(begin(), end(), match, comparing_object);
   matches_difference_type default_offset = default_match_ - begin();
   if ((insertion_point - begin()) <= default_offset)
     ++default_offset;
@@ -230,7 +288,8 @@ bool AutocompleteResult::HasMatchByDestination(const AutocompleteMatch& match,
   return false;
 }
 
-void AutocompleteResult::MergeMatchesByProvider(const ACMatches& old_matches,
+void AutocompleteResult::MergeMatchesByProvider(const AutocompleteInput& input,
+                                                const ACMatches& old_matches,
                                                 const ACMatches& new_matches) {
   if (new_matches.size() >= old_matches.size())
     return;
@@ -250,7 +309,7 @@ void AutocompleteResult::MergeMatchesByProvider(const ACMatches& old_matches,
       AutocompleteMatch match = *i;
       match.relevance = std::min(max_relevance, match.relevance);
       match.from_previous = true;
-      AddMatch(match);
+      AddMatch(input, match);
       delta--;
     }
   }