Optimize shortcuts provider

components/omnibox/browser/match_compare.h

Index: components/omnibox/browser/match_compare.h
diff --git a/components/omnibox/browser/match_compare.h b/components/omnibox/browser/match_compare.h
new file mode 100644
index 0000000000000000000000000000000000000000..435874ebc16e0fe2ac1a21c2b9b98a4913de0421
--- /dev/null
+++ b/components/omnibox/browser/match_compare.h
@@ -0,0 +1,74 @@
+// Copyright 2016 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.
+
+#ifndef COMPONENTS_OMNIBOX_BROWSER_MATCH_COMPARE_H_
+#define COMPONENTS_OMNIBOX_BROWSER_MATCH_COMPARE_H_
+
+#include "components/omnibox/browser/omnibox_field_trial.h"
+
+using metrics::OmniboxEventProto;

[Peter Kasting, 2016/04/14 23:52:33]

Nit: Avoid this using directive, just explicitly qualify below.

[Alexander Yashkin, 2016/04/15 09:14:14]

Done.
Yet, IMHO,  metrics::OmniboxEventProto::PageClassification looks too long in
arguments list.

+
+// This class implements a special version of AutocompleteMatch::MoreRelevant
+// that allows matches of particular types to be demoted in AutocompleteResult.
+template <class Match> class CompareWithDemoteByType {
+ public:
+  CompareWithDemoteByType(
+      OmniboxEventProto::PageClassification current_page_classification) {
+    OmniboxFieldTrial::GetDemotionsByType(current_page_classification,
+                                          &demotions_);
+  }
+
+  // Returns the relevance score of |match| demoted appropriately by
+  // |demotions_by_type_|.
+  int GetDemotedRelevance(const Match& match) const {
+      OmniboxFieldTrial::DemotionMultipliers::const_iterator demotion_it =

[Peter Kasting, 2016/04/14 23:52:33]

Nit: Wrong indentation (this line is in too far, next isn't far enough)

[Alexander Yashkin, 2016/04/15 09:14:14]

Done.

+      demotions_.find(match.type);
+    return (demotion_it == demotions_.end()) ?
+        match.relevance : (match.relevance * demotion_it->second);
+  }
+
+  // Comparison function.
+  bool operator()(const Match& elem1, const Match& 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);
+  }
+
+ private:
+  OmniboxFieldTrial::DemotionMultipliers demotions_;
+};
+
+template<class Match> class DestinationSort {
+ public:
+  DestinationSort(
+      OmniboxEventProto::PageClassification current_page_classification) :

[Peter Kasting, 2016/04/14 23:52:33]

Nit: ':' goes at the beginning of the next line, not the end of this one

[Alexander Yashkin, 2016/04/15 09:14:14]

Done.

+      demote_by_type_(current_page_classification) {}
+  bool operator()(const Match& elem1,

[Peter Kasting, 2016/04/14 23:52:33]

Nit: Unnecessary linebreak

[Alexander Yashkin, 2016/04/15 09:14:14]

Done.

+                  const Match& elem2) {
+    // Sort identical destination_urls together.
+    // Place the most relevant matches first, so that when we call
+    // std::unique(), these are the ones that get preserved.
+    bool matches_destinations_equal =
+        !elem1.stripped_destination_url.is_empty() &&
+        (elem1.stripped_destination_url == elem2.stripped_destination_url);
+    bool matches_destinations_empty =
+        elem1.stripped_destination_url.is_empty() &&
+        elem2.stripped_destination_url.is_empty();
+    if (matches_destinations_equal || matches_destinations_empty) {

[Peter Kasting, 2016/04/14 23:52:33]

This is all more complicated than need be.  The logic just boils down to:

    return (elem1.stripped_destination_url == elem2.stripped_destination_url) ?
        demote_by_type_(elem1, elem2) :
        (elem1.stripped_destination_url < elem2.stripped_destination_url);

[Alexander Yashkin, 2016/04/15 09:14:14]

Good point. Done.

+      return demote_by_type_(elem1, elem2);
+    }
+    return elem1.stripped_destination_url < elem2.stripped_destination_url;
+  }
+
+ private:
+  CompareWithDemoteByType<Match> demote_by_type_;
+};
+
+#endif  // COMPONENTS_OMNIBOX_BROWSER_MATCH_COMPARE_H_