[Android] Persist ordering of NTP suggestions.

chrome/browser/android/most_visited_sites_unittest.cc

Index: chrome/browser/android/most_visited_sites_unittest.cc
diff --git a/chrome/browser/android/most_visited_sites_unittest.cc b/chrome/browser/android/most_visited_sites_unittest.cc
index f2dc351c6734bfb7f4599e35b513383f8f44688f..2f17c89ad3e9445382fabf63ef34c32ee04c40b3 100644
--- a/chrome/browser/android/most_visited_sites_unittest.cc
+++ b/chrome/browser/android/most_visited_sites_unittest.cc
@@ -12,14 +12,11 @@
 namespace {
 
 struct TitleURL {
-  TitleURL(const std::string& title,
-           const std::string& url,
-           const std::string& source)
-      : title(base::UTF8ToUTF16(title)), url(url), source(source) {}
+  TitleURL(const std::string& title, const std::string& url)
+      : title(base::UTF8ToUTF16(title)), url(url) {}
 
   base::string16 title;
   std::string url;
-  std::string source;
 };
 
 std::vector<base::string16> GetTitles(const std::vector<TitleURL>& data) {
@@ -36,158 +33,199 @@ std::vector<std::string> GetURLs(const std::vector<TitleURL>& data) {
   return urls;
 }
 
-std::vector<std::string> GetSources(const std::vector<TitleURL>& data) {
-  std::vector<std::string> sources;
+std::vector<std::string> GetHosts(const std::vector<TitleURL>& data) {
+  std::vector<std::string> hosts;
   for (const TitleURL& item : data)
-    sources.push_back(item.source);
-  return sources;
+    hosts.emplace_back(GURL(item.url).host());
+  return hosts;
 }
 
-static const int kNumSites = 4;
+static const size_t kNumSites = 4;
 
 }  // namespace
 
+// This a test for MostVisitedSites::MergeSuggestions(...) method, and thus has
+// the same scope as the method itself. This includes:
+// + Merge popular suggestions with personal suggestions.
+// + Order the suggestions correctly based on the previous ordering.
+// More importantly things out of the scope of testing presently:
+// - Removing blacklisted suggestions.

[Marc Treib, 2015/09/07 11:13:00]

Also, blacklisting suggestions in the first place. I don't think we particularly
care about the blacklisting case though :)

[knn, 2015/09/08 10:48:56]

Acknowledged.

+// - Storing the current suggestion ordering.
+// - Retrieving the previous ordering.
+// - Correct Host extraction from the URL.

[Marc Treib, 2015/09/07 11:13:00]

This is just a feature of GURL and (hopefully) tested elsewhere?
I guess you actually mean the "duplicate" elimination?

[knn, 2015/09/08 10:48:56]

No that is below. Agreed it is tested elsewhere, just specifying scope.

+// - Ensuring popular suggestions don't contain personal ones.

[Marc Treib, 2015/09/07 11:13:00]

What does this mean?

[knn, 2015/09/08 10:48:56]

This is duplicate elimination.

[Marc Treib, 2015/09/08 11:39:49]

Then please say that :)
I find the current comment confusing, since of course there can be overlap
between personal and popular suggestions.

[knn, 2015/09/08 13:42:00]

There shouldn't be overlap between the two right? I have rephrased it.

[Marc Treib, 2015/09/08 13:56:55]

After MergeSuggestions, right. The comment IMO doesn't really make clear if
you're talking about the "before" or "after" state.

[knn, 2015/09/08 14:17:32]

I'm not sure I understand, there is no "after" for personal and popular
suggestions. Can you read my rephrased comment and suggest changes?

[Marc Treib, 2015/09/08 14:24:27]

Or maybe I'm missing something? What I mean is: When you call
MergeSuggestions(personal_suggestions, popular_suggestions, ...)
below, then of course there can be duplicates between personal_suggestions and
popular_suggestions, and it's MergeSuggestions' job to filter them out, and not
put any duplicates into the result. (Right?)
So maybe: "Ensure that MostVisitedSites::MergeSuggestions filters out duplicated
between personal and popular suggestions"?

 class MostVisitedSitesTest : public testing::Test {
  protected:
-  void Check(const std::vector<TitleURL>& popular,
-             const std::vector<TitleURL>& personal,
-             const std::vector<TitleURL>& expected) {
-    std::vector<base::string16> titles(GetTitles(personal));
-    std::vector<std::string> urls(GetURLs(personal));
-    std::vector<std::string> sources(GetSources(personal));
-
-    std::vector<base::string16> popular_titles(GetTitles(popular));
-    std::vector<std::string> popular_urls(GetURLs(popular));
-
-    MostVisitedSites::AddPopularSitesImpl(
-        kNumSites, popular_titles, popular_urls, &titles, &urls, &sources);
-
-    EXPECT_EQ(GetTitles(expected), titles);
-    EXPECT_EQ(GetURLs(expected), urls);
-    EXPECT_EQ(GetSources(expected), sources);
+  void Check(const std::vector<TitleURL>& popular_sites,
+             const std::vector<TitleURL>& personal_sites,
+             const std::vector<std::string>& old_sites_url,
+             const std::vector<bool>& old_sites_source,
+             const std::vector<bool>& expected_sites_source,
+             const std::vector<TitleURL>& expected_sites) {
+    std::vector<base::string16> personal_titles(GetTitles(personal_sites));
+    std::vector<std::string> personal_urls(GetURLs(personal_sites));
+    std::vector<std::string> personal_hosts(GetHosts(personal_sites));
+
+    std::vector<base::string16> popular_titles(GetTitles(popular_sites));
+    std::vector<std::string> popular_urls(GetURLs(popular_sites));
+    std::vector<std::string> popular_hosts(GetHosts(popular_sites));
+
+    std::vector<bool> result_sources;
+    std::vector<std::string> result_urls;
+    std::vector<base::string16> result_titles;
+
+    MostVisitedSites::MergeSuggestions(
+        &result_sources, &result_urls, &result_titles, kNumSites,
+        personal_sites.size(), popular_sites.size(), &personal_urls,
+        &personal_titles, &popular_urls, &popular_titles, personal_hosts,
+        popular_hosts, old_sites_url, old_sites_source);
+
+    EXPECT_EQ(GetTitles(expected_sites), result_titles);
+    EXPECT_EQ(GetURLs(expected_sites), result_urls);
+    EXPECT_EQ(expected_sites_source, result_sources);
   }
 };
 
-TEST_F(MostVisitedSitesTest, PopularSitesAppend) {
-  TitleURL popular[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
+TEST_F(MostVisitedSitesTest, PersonalSitesDefaultOrder) {
+  TitleURL personal[] = {
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
   };
+  std::vector<TitleURL> personal_sites(personal,
+                                       personal + arraysize(personal));
+  std::vector<std::string> old_sites_url;
+  std::vector<bool> old_sites_source;
+  // Without any previous ordering or popular suggestions, the result after
+  // merge should be the personal suggestions themselves.
+  std::vector<bool> expected_sites_source(kNumSites, true /*personal source*/);
+  Check(std::vector<TitleURL>(), personal_sites, old_sites_url,
+        old_sites_source, expected_sites_source, personal_sites);
+}
+
+TEST_F(MostVisitedSitesTest, PersonalSitesDefinedOrder) {
   TitleURL personal[] = {
-    TitleURL("Site 3", "https://www.site3.com/", "server8"),
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
+  };
+  std::string old[] = {
+      "https://www.site4.com/", "https://www.site2.com/",
   };
-  // Popular suggestions should keep their positions, with personal suggestions
-  // appended at the end.
+  std::vector<bool> old_sites_source(arraysize(old), true /*personal source*/);
   TitleURL expected[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 3", "https://www.site3.com/", "server8"),
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
+      TitleURL("Site 4", "https://www.site4.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
   };
-
-  Check(std::vector<TitleURL>(popular, popular + arraysize(popular)),
+  std::vector<bool> expected_sites_source(kNumSites, true /*personal source*/);
+  Check(std::vector<TitleURL>(),
         std::vector<TitleURL>(personal, personal + arraysize(personal)),
+        std::vector<std::string>(old, old + arraysize(old)), old_sites_source,
+        expected_sites_source,
         std::vector<TitleURL>(expected, expected + arraysize(expected)));
 }
 
-TEST_F(MostVisitedSitesTest, PopularSitesOverflow) {
+TEST_F(MostVisitedSitesTest, PopularSitesDefaultOrder) {
   TitleURL popular[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 3", "https://www.site3.com/", "popular"),
-  };
-  TitleURL personal[] = {
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
-    TitleURL("Site 5", "https://www.site5.com/", "server8"),
-  };
-  // When there are more total suggestions than slots, the personal suggestions
-  // should win, with the remaining popular suggestions still retaining their
-  // positions.
-  TitleURL expected[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
-    TitleURL("Site 5", "https://www.site5.com/", "server8"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
   };
-
-  Check(std::vector<TitleURL>(popular, popular + arraysize(popular)),
-        std::vector<TitleURL>(personal, personal + arraysize(personal)),
-        std::vector<TitleURL>(expected, expected + arraysize(expected)));
+  std::vector<TitleURL> popular_sites(popular, popular + arraysize(popular));
+  std::vector<std::string> old_sites_url;
+  std::vector<bool> old_sites_source;
+  // Without any previous ordering or personal suggestions, the result after
+  // merge should be the popular suggestions themselves.
+  std::vector<bool> expected_sites_source(kNumSites, false /*popular source*/);
+  Check(popular_sites, std::vector<TitleURL>(), old_sites_url, old_sites_source,
+        expected_sites_source, popular_sites);
 }
 
-TEST_F(MostVisitedSitesTest, PopularSitesOverwrite) {
+TEST_F(MostVisitedSitesTest, PopularSitesDefinedOrder) {
   TitleURL popular[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 3", "https://www.site3.com/", "popular"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
   };
-  TitleURL personal[] = {
-    TitleURL("Site 2 subpage", "https://www.site2.com/path", "server8"),
+  std::string old[] = {
+      "https://www.site4.com/", "https://www.site2.com/",
   };
-  // When a personal suggestions matches the host of a popular one, it should
-  // overwrite that suggestion (in its original position).
+  std::vector<bool> old_sites_source(arraysize(old), false /*popular source*/);
   TitleURL expected[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2 subpage", "https://www.site2.com/path", "server8"),
-    TitleURL("Site 3", "https://www.site3.com/", "popular"),
+      TitleURL("Site 4", "https://www.site4.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
   };
-
+  std::vector<bool> expected_sites_source(kNumSites, false /*popular source*/);
   Check(std::vector<TitleURL>(popular, popular + arraysize(popular)),
-        std::vector<TitleURL>(personal, personal + arraysize(personal)),
+        std::vector<TitleURL>(),
+        std::vector<std::string>(old, old + arraysize(old)), old_sites_source,
+        expected_sites_source,
         std::vector<TitleURL>(expected, expected + arraysize(expected)));
 }
 
-TEST_F(MostVisitedSitesTest, PopularSitesOrdering) {
+TEST_F(MostVisitedSitesTest, PopularAndPersonalDefaultOrder) {
   TitleURL popular[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 3", "https://www.site3.com/", "popular"),
-    TitleURL("Site 4", "https://www.site4.com/", "popular"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
   };
   TitleURL personal[] = {
-    TitleURL("Site 3", "https://www.site3.com/", "server8"),
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
-    TitleURL("Site 1", "https://www.site1.com/", "server8"),
-    TitleURL("Site 2", "https://www.site2.com/", "server8"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
   };
-  // The personal sites should replace the popular ones, but the order of the
-  // popular sites should win (since presumably they were there first).
+  // Without an explicit ordering, personal suggestions precede popular
+  // suggestions.
   TitleURL expected[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "server8"),
-    TitleURL("Site 2", "https://www.site2.com/", "server8"),
-    TitleURL("Site 3", "https://www.site3.com/", "server8"),
-    TitleURL("Site 4", "https://www.site4.com/", "server8"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
   };
-
+  bool expected_source_is_personal[] = {true, true, false, false};
   Check(std::vector<TitleURL>(popular, popular + arraysize(popular)),
         std::vector<TitleURL>(personal, personal + arraysize(personal)),
+        std::vector<std::string>(), std::vector<bool>(),
+        std::vector<bool>(expected_source_is_personal,
+                          expected_source_is_personal +
+                              arraysize(expected_source_is_personal)),
         std::vector<TitleURL>(expected, expected + arraysize(expected)));
 }
 
-TEST_F(MostVisitedSitesTest, PopularSitesComplex) {

[Marc Treib, 2015/09/07 11:13:00]

Why did you remove this test? I don't think your current tests cover the
overflow case in particular (i.e. more total suggestions than we have tiles).

[Marc Treib, 2015/09/09 12:15:01]

I think this hasn't been addressed yet - do we have a test that covers the
overflow case, i.e. where we have more total suggestions than will fit on the
page?

[knn, 2015/09/09 13:54:30]

Oops missed this.
I did not remove any particular test, I rewrote all the cases that are within
the scope of the changed function. We presently make sure that we only take the
required number of suggestions so this will never happen.
This along with restoring the previous suggestions from preferences will have to
be tested elsewhere.

[Marc Treib, 2015/09/09 14:00:47]

Ah, right. Okay then, I guess.

+TEST_F(MostVisitedSitesTest, PopularAndPersonalDefinedOrder) {
   TitleURL popular[] = {
-    TitleURL("Site 1", "https://www.site1.com/", "popular"),
-    TitleURL("Site 2", "https://www.site2.com/", "popular"),
-    TitleURL("Site 3", "https://www.site3.com/", "popular"),
+      TitleURL("Site 1", "https://www.site1.com/"),
+      TitleURL("Site 2", "https://www.site2.com/"),
   };
   TitleURL personal[] = {
-    TitleURL("Site 3 subpage", "https://www.site3.com/path", "server8"),
-    TitleURL("Site 1 subpage", "https://www.site1.com/path", "server8"),
-    TitleURL("Site 5", "https://www.site5.com/", "server8"),
-    TitleURL("Site 6", "https://www.site6.com/", "server8"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
   };
-  // Combination of behaviors: Personal suggestions replace matching popular
-  // ones while keeping the position of the popular suggestion. Remaining
-  // personal suggestions evict popular ones and retain their relative order.
+  std::string old[] = {
+      "https://www.site2.com/", "https://www.unknownsite.com/",
+      "https://www.site4.com/",
+  };
+  std::vector<bool> old_sites_source(arraysize(old), false /*popular source*/);
+  // Keep the order constant for previous suggestions, else personal suggestions
+  // precede popular suggestions.
   TitleURL expected[] = {
-    TitleURL("Site 1 subpage", "https://www.site1.com/path", "server8"),
-    TitleURL("Site 5", "https://www.site5.com/", "server8"),
-    TitleURL("Site 3 subpage", "https://www.site3.com/path", "server8"),
-    TitleURL("Site 6", "https://www.site6.com/", "server8"),
+      TitleURL("Site 2", "https://www.site2.com/"),
+      TitleURL("Site 3", "https://www.site3.com/"),
+      TitleURL("Site 4", "https://www.site4.com/"),
+      TitleURL("Site 1", "https://www.site1.com/"),
   };
-
+  bool expected_source_is_personal[] = {false, true, true, false};
   Check(std::vector<TitleURL>(popular, popular + arraysize(popular)),
         std::vector<TitleURL>(personal, personal + arraysize(personal)),
+        std::vector<std::string>(old, old + arraysize(old)), old_sites_source,
+        std::vector<bool>(expected_source_is_personal,
+                          expected_source_is_personal +
+                              arraysize(expected_source_is_personal)),
         std::vector<TitleURL>(expected, expected + arraysize(expected)));
 }