Optimize shortcuts provider

components/omnibox/browser/shortcuts_provider_unittest.cc

Index: components/omnibox/browser/shortcuts_provider_unittest.cc
diff --git a/components/omnibox/browser/shortcuts_provider_unittest.cc b/components/omnibox/browser/shortcuts_provider_unittest.cc
index 00947f962f62b8efa9230ceb67f07bf0a46f5dcd..7801568ea8a3fad8b757f411ae488d167d2674cc 100644
--- a/components/omnibox/browser/shortcuts_provider_unittest.cc
+++ b/components/omnibox/browser/shortcuts_provider_unittest.cc
@@ -750,3 +750,51 @@ TEST_F(ShortcutsProviderTest, DoesNotProvideOnFocus) {
   provider_->Start(input, false);
   EXPECT_TRUE(provider_->matches().empty());
 }
+
+TEST_F(ShortcutsProviderTest, SortAndDedupMatches) {
+  using ShortcutMatch = ShortcutsProvider::ShortcutMatch;
+  // Create empty ShortcutsDatabase::Shortcut object that will be used to
+  // initialize ShortcutMatch objects.
+  ShortcutsDatabase::Shortcut::MatchCore db_match_core(
+      base::string16(), GURL(), base::string16(), std::string(),
+      base::string16(), std::string(), 0, 0, base::string16());
+  // Object used only for ShortcutMatch initialization.
+  ShortcutsDatabase::Shortcut db_shortcut(std::string(), base::string16(),
+      db_match_core, base::Time(), 0);
+
+  std::vector<ShortcutMatch> matches;
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("123");

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

Nit: Set this value in the constructor above, push all the matches using it onto
the vector first, and eliminate all the explicit sets of ("123").

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

I removed unittest for  SortAndDedupMatches entirely because I believe its
functionality is already tested
by existing ShortcutsProvider tests.

+  matches.push_back(

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

Nit: While normally push_back() is more readable, I'm inclined to suggest using
emplace_back() on these lines just so they can all be fit onto one line each. 
(That would also eliminate the need for the using directive at the top.)  WDYT?

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

Removed test entirely.

+    ShortcutMatch(42, GURL("http://www.abc.com"), &db_shortcut));
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("123");
+  matches.push_back(
+      ShortcutMatch(43, GURL("http://www.abcd.com"), &db_shortcut));
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("123");
+  matches.push_back(
+      ShortcutMatch(42, GURL("http://www.abcde.com"), &db_shortcut));
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("DEADBEEF");
+  matches.push_back(
+      ShortcutMatch(41, GURL("http://www.abcd.com"), &db_shortcut));
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("234");
+  matches.push_back(
+      ShortcutMatch(42, GURL("http://www.abcde.com"), &db_shortcut));
+  db_shortcut.match_core.contents = base::ASCIIToUTF16("123");
+  matches.push_back(
+      ShortcutMatch(42, GURL("http://www.abc.com"), &db_shortcut));
+
+  ShortcutsProvider::SortAndDedupMatches(
+      metrics::OmniboxEventProto::INVALID_SPEC, &matches);
+  // Expect deduplicated matches to be sorted by destination.
+  // The remaining match out of each set of duplicates should be the one with
+  // the highest relevance.
+  EXPECT_EQ(3U, matches.size());
+  EXPECT_EQ(matches[0].stripped_destination_url, GURL("http://www.abc.com"));

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

Nit: (expected, actual) (many places)

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

Removed test entirely.

+  EXPECT_EQ(matches[0].relevance, 42);
+  EXPECT_EQ(matches[0].contents, base::ASCIIToUTF16("123"));
+  EXPECT_EQ(matches[1].stripped_destination_url, GURL("http://www.abcd.com"));
+  EXPECT_EQ(matches[1].relevance, 43);
+  EXPECT_EQ(matches[1].contents, base::ASCIIToUTF16("123"));
+  EXPECT_EQ(matches[2].stripped_destination_url, GURL("http://www.abcde.com"));
+  EXPECT_EQ(matches[2].relevance, 42);
+  EXPECT_EQ(matches[2].contents, base::ASCIIToUTF16("123"));
+}