Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(4430)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: chrome/browser/android/most_visited_sites_unittest.cc

Issue 1330773002: [Android] Persist ordering of NTP suggestions. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Move things around in a struct Created 5 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« chrome/browser/android/most_visited_sites.cc ('K') | « chrome/browser/android/most_visited_sites.cc ('k') | chrome/browser/prefs/browser_prefs.cc » ('j') | chrome/common/pref_names.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..9ad9844411f977d7c79b37f960458456dc49d729 100644
--- a/chrome/browser/android/most_visited_sites_unittest.cc
+++ b/chrome/browser/android/most_visited_sites_unittest.cc
@@ -12,182 +12,213 @@
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) {}
+ TitleURL(const base::string16& title, const std::string& url)
+ : title(title), url(url) {}
base::string16 title;
std::string url;
- std::string source;
-};
-
-std::vector<base::string16> GetTitles(const std::vector<TitleURL>& data) {
- std::vector<base::string16> titles;
- for (const TitleURL& item : data)
- titles.push_back(item.title);
- return titles;
-}
-std::vector<std::string> GetURLs(const std::vector<TitleURL>& data) {
- std::vector<std::string> urls;
- for (const TitleURL& item : data)
- urls.push_back(item.url);
- return urls;
-}
+ bool operator==(const TitleURL& other) const {
+ return title == other.title && url == other.url;
+ }
-std::vector<std::string> GetSources(const std::vector<TitleURL>& data) {
- std::vector<std::string> sources;
- for (const TitleURL& item : data)
- sources.push_back(item.source);
- return sources;
-}
+ MostVisitedSites::Suggestion* GetSuggestion(bool is_personal) const {
Marc Treib 2015/09/08 11:39:49 nit: Return a scoped_ptr to make ownership clear?
knn 2015/09/08 13:42:00 Done.
+ return new MostVisitedSites::Suggestion(
+ title, url,
+ is_personal ? MostVisitedSites::TOP_SITES : MostVisitedSites::POPULAR);
+ }
+};
-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.
+// - Storing the current suggestion ordering.
+// - Retrieving the previous ordering.
+// - Correct Host extraction from the URL.
+// - Ensuring popular suggestions don't contain personal ones.
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_is_personal,
+ const std::vector<bool>& expected_sites_is_personal,
+ const std::vector<TitleURL>& expected_sites) {
+ ScopedVector<MostVisitedSites::Suggestion> personal_suggestions;
+ personal_suggestions.reserve(personal_sites.size());
+ for (const auto& site : personal_sites)
+ personal_suggestions.push_back(site.GetSuggestion(true));
+ ScopedVector<MostVisitedSites::Suggestion> popular_suggestions;
+ popular_suggestions.reserve(popular_sites.size());
+ for (const auto& site : popular_sites)
+ popular_suggestions.push_back(site.GetSuggestion(false));
+ ScopedVector<MostVisitedSites::Suggestion> result_suggestions;
+ MostVisitedSites::MergeSuggestions(
+ &personal_suggestions, &popular_suggestions, old_sites_url,
+ old_sites_is_personal, &result_suggestions);
+ std::vector<TitleURL> result_sites;
+ std::vector<bool> result_is_personal;
+ result_sites.reserve(result_suggestions.size());
+ result_is_personal.reserve(result_suggestions.size());
+ for (const auto suggestion : result_suggestions) {
+ result_sites.push_back(TitleURL(suggestion->title, suggestion->url));
+ result_is_personal.push_back(suggestion->source !=
+ MostVisitedSites::POPULAR);
+ }
+ EXPECT_EQ(result_is_personal, expected_sites_is_personal);
+ EXPECT_EQ(result_sites, expected_sites);
}
};
-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/"),
};
- // Popular suggestions should keep their positions, with personal suggestions
- // appended at the end.
+ std::string old[] = {
+ "https://www.site4.com/", "https://www.site2.com/",
+ };
+ 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("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 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"),
- };
-
- 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) {
+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/"),
+ };
+ std::string old[] = {
+ "https://www.site2.com/", "https://www.unknownsite.com/",
+ "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::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)));
}
« chrome/browser/android/most_visited_sites.cc ('K') | « chrome/browser/android/most_visited_sites.cc ('k') | chrome/browser/prefs/browser_prefs.cc » ('j') | chrome/common/pref_names.cc » ('J')

Powered by Google App Engine
This is Rietveld 408576698