Optimize shortcuts provider

components/omnibox/browser/shortcuts_provider.h

Index: components/omnibox/browser/shortcuts_provider.h
diff --git a/components/omnibox/browser/shortcuts_provider.h b/components/omnibox/browser/shortcuts_provider.h
index 3344fe36d28d6a49bef689798136151e55a417dc..cda7067aa74a1a2bcecd26823fa29c464292b423 100644
--- a/components/omnibox/browser/shortcuts_provider.h
+++ b/components/omnibox/browser/shortcuts_provider.h
@@ -31,33 +31,34 @@ class ShortcutsProvider : public AutocompleteProvider,
 
   void DeleteMatch(const AutocompleteMatch& match) override;
 
+  // ShortcutMatch holds sufficient information about a single match from the
+  // shortcut database to allow for destination deduping and relevance sorting.
+  // After those stages the top matches are converted to the more heavyweight
+  // AutocompleteMatch struct.  Avoiding constructing the larger struct for
+  // every such match can save significant time when there are many shortcut
+  // matches to process.
+  struct ShortcutMatch {

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

This struct doesn't need to access any members of ShortcutsProvider, so I would
not make it a member struct.  I would keep it as a toplevel struct, probably
with its own (very short) header and .cc file since you want to be able to sue
it in the unittest.  (The other option would be to change the unittests to test
a larger piece of functionality than just SortAndDedupMatches(), e.g.
GetMatches(), which would not take ShortcutMatches directly as input, and then
make this struct local to the .cc file here.)

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

I moved ShortcutMatch structure and SortAndDedupMatches function to
shortcuts_provider.cc anonimous namespace.
I removed unittest for  SortAndDedupMatches entirely because I believe its
functionality is already tested
by existing ShortcutsProvider tests.

+    ShortcutMatch(int relevance,
+                  const GURL& stripped_destination_url,
+                  const ShortcutsDatabase::Shortcut* shortcut);
+    int relevance;
+    GURL stripped_destination_url;
+    const ShortcutsDatabase::Shortcut* shortcut;
+    base::string16 contents;
+    AutocompleteMatch::Type type;
+  };
+
  private:
   friend class ClassifyTest;
   friend class ShortcutsProviderExtensionTest;
   friend class ShortcutsProviderTest;
   FRIEND_TEST_ALL_PREFIXES(ShortcutsProviderTest, CalculateScore);
+  FRIEND_TEST_ALL_PREFIXES(ShortcutsProviderTest, SortAndDedupMatches);

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

Nit: If this is only needed because of the call to SortAndDedupMatches(), I'd
rather make that function public and eliminate this.

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

Removed.

 
   typedef std::multimap<base::char16, base::string16> WordMap;
 
   ~ShortcutsProvider() override;
 
-  // ShortcutsBackendObserver:
-  void OnShortcutsLoaded() override;
-
-  // Performs the autocomplete matching and scoring.
-  void GetMatches(const AutocompleteInput& input);
-
-  // Returns an AutocompleteMatch corresponding to |shortcut|. Assigns it
-  // |relevance| score in the process, and highlights the description and
-  // contents against |input|, which should be the lower-cased version of
-  // the user's input. |input| and |fixed_up_input_text| are used to decide
-  // what can be inlined.
-  AutocompleteMatch ShortcutToACMatch(
-      const ShortcutsDatabase::Shortcut& shortcut,
-      int relevance,
-      const AutocompleteInput& input,
-      const base::string16& fixed_up_input_text);
-
   // Returns a map mapping characters to groups of words from |text| that start
   // with those characters, ordered lexicographically descending so that longer
   // words appear before their prefixes (if any) within a particular
@@ -90,6 +91,31 @@ class ShortcutsProvider : public AutocompleteProvider,
       const base::string16& text,
       const ACMatchClassifications& original_class);
 
+  // Sorts |matches| by destination, taking into account demotions based on
+  // |page_classification| when resolving ties about which of several
+  // duplicates to keep.  The matches are also deduplicated.
+  static void SortAndDedupMatches(
+      metrics::OmniboxEventProto::PageClassification page_classification,
+      std::vector<ShortcutMatch>* matches);
+
+  // ShortcutsBackendObserver:
+  void OnShortcutsLoaded() override;
+
+  // Performs the autocomplete matching and scoring.
+  void GetMatches(const AutocompleteInput& input);
+
+  // Returns an AutocompleteMatch corresponding to |shortcut_match|. Assigns it
+  // |relevance| score in the process, and highlights the description and
+  // contents against |input|, which should be the lower-cased version of
+  // the user's input. |input| and |fixed_up_input_text| are used to decide
+  // what can be inlined.
+  AutocompleteMatch ShortcutMatchToACMatch(
+      const ShortcutMatch& shortcut_match,
+      const AutocompleteInput& input,
+      const base::string16& fixed_up_input_text,
+      const base::string16 term_string,
+      const WordMap& terms_map);
+
   // Returns iterator to first item in |shortcuts_map_| matching |keyword|.
   // Returns shortcuts_map_.end() if there are no matches.
   ShortcutsBackend::ShortcutMap::const_iterator FindFirstMatch(