[NTP::SectionOrder] Replace CategoryFactory with a category ranker.

components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc b/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
index 378ff2fef7602655ad364333e7bfe08198afc4c1..1a916336ce11a6ac4fdbe73101b29fb16b9e2630 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc
@@ -26,8 +26,10 @@
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/image_fetcher/image_fetcher_delegate.h"
-#include "components/ntp_snippets/category_factory.h"
+#include "components/ntp_snippets/category.h"
 #include "components/ntp_snippets/category_info.h"
+#include "components/ntp_snippets/category_rankers/category_ranker.h"
+#include "components/ntp_snippets/category_rankers/constant_category_ranker.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/ntp_snippet.h"
@@ -116,44 +118,64 @@ base::Time GetDefaultExpirationTime() {
   return base::Time::Now() + base::TimeDelta::FromHours(1);
 }
 
-std::string GetTestJson(const std::vector<std::string>& snippets,
-                        const std::string& category_title) {
+std::string GetCategoryJson(const std::vector<std::string>& snippets,
+                            int remote_category_id,
+                            const std::string& category_title) {
   return base::StringPrintf(
-      "{\n"
-      "  \"categories\": [{\n"
-      "    \"id\": 1,\n"
+      "  {\n"
+      "    \"id\": %d,\n"
       "    \"localizedTitle\": \"%s\",\n"
       "    \"suggestions\": [%s]\n"
-      "  }]\n"
-      "}\n",
-      category_title.c_str(), base::JoinString(snippets, ", ").c_str());
+      "  }\n",
+      remote_category_id, category_title.c_str(),
+      base::JoinString(snippets, ", ").c_str());
 }
 
-std::string GetTestJson(const std::vector<std::string>& snippets) {
-  return GetTestJson(snippets, kTestJsonDefaultCategoryTitle);
+class MutliCategoryJsonBuilder {

[Marc Treib, 2016/12/14 10:24:31]

s/Mutli/Multi/g

[vitaliii, 2016/12/15 15:30:12]

Done.

+ public:
+  MutliCategoryJsonBuilder() {}
+
+  MutliCategoryJsonBuilder& AddCategoryWithCustomTitle(
+      const std::vector<std::string>& snippets,
+      int remote_category_id,
+      const std::string& category_title) {
+    category_json.push_back(
+        GetCategoryJson(snippets, remote_category_id, category_title));
+    return *this;
+  }
+
+  MutliCategoryJsonBuilder& AddCategory(
+      const std::vector<std::string>& snippets,
+      int remote_category_id) {
+    return AddCategoryWithCustomTitle(
+        snippets, remote_category_id,
+        "Title" + base::IntToString(remote_category_id));
+  }
+
+  std::string Build() {
+    return base::StringPrintf(
+        "{\n"
+        "  \"categories\": [\n"
+        "%s\n"
+        "  ]\n"
+        "}\n",
+        base::JoinString(category_json, "  ,\n").c_str());
+  }
+
+ private:
+  std::vector<std::string> category_json;
+};
+
+std::string GetTestJson(const std::vector<std::string>& snippets,
+                        const std::string& category_title) {
+  return MutliCategoryJsonBuilder()
+      .AddCategoryWithCustomTitle(snippets, /*remote_category_id=*/1,
+                                  category_title)
+      .Build();
 }
 
-// TODO(tschumann): Remove the default parameter other_id. It makes the tests
-// less explicit and hard to read. Also get rid of the convenience
-// other_category() and unknown_category() helpers -- tests can just define
-// their own.
-std::string GetMultiCategoryJson(const std::vector<std::string>& articles,
-                                 const std::vector<std::string>& others,
-                                 int other_id = 2) {
-  return base::StringPrintf(
-      "{\n"
-      "  \"categories\": [{\n"
-      "    \"id\": 1,\n"
-      "    \"localizedTitle\": \"Articles for You\",\n"
-      "    \"suggestions\": [%s]\n"
-      "  }, {\n"
-      "    \"id\": %i,\n"
-      "    \"localizedTitle\": \"Other Things\",\n"
-      "    \"suggestions\": [%s]\n"
-      "  }]\n"
-      "}\n",
-      base::JoinString(articles, ", ").c_str(), other_id,
-      base::JoinString(others, ", ").c_str());
+std::string GetTestJson(const std::vector<std::string>& snippets) {
+  return GetTestJson(snippets, kTestJsonDefaultCategoryTitle);
 }
 
 std::string FormatTime(const base::Time& t) {
@@ -401,6 +423,7 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
         fake_url_fetcher_factory_(
             /*default_factory=*/&failing_url_fetcher_factory_),
         test_url_(kTestContentSuggestionsServerWithAPIKey),
+        category_ranker_(base::MakeUnique<ConstantCategoryRanker>()),
         user_classifier_(/*pref_service=*/nullptr),
         image_fetcher_(nullptr),
         image_decoder_(nullptr) {
@@ -436,9 +459,8 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher =
         base::MakeUnique<NTPSnippetsFetcher>(
             utils_.fake_signin_manager(), fake_token_service_.get(),
-            std::move(request_context_getter), utils_.pref_service(),
-            &category_factory_, nullptr, base::Bind(&ParseJson), kAPIKey,
-            &user_classifier_);
+            std::move(request_context_getter), utils_.pref_service(), nullptr,
+            base::Bind(&ParseJson), kAPIKey, &user_classifier_);
 
     utils_.fake_signin_manager()->SignIn("foo@bar.com");
 
@@ -451,7 +473,7 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
     EXPECT_FALSE(observer_);
     observer_ = base::MakeUnique<FakeContentSuggestionsProviderObserver>();
     return base::MakeUnique<RemoteSuggestionsProvider>(
-        observer_.get(), &category_factory_, utils_.pref_service(), "fr",
+        observer_.get(), utils_.pref_service(), "fr", category_ranker_.get(),
         &user_classifier_, &scheduler_, std::move(snippets_fetcher),
         std::move(image_fetcher), std::move(image_decoder),
         base::MakeUnique<RemoteSuggestionsDatabase>(database_dir_.GetPath(),
@@ -478,26 +500,37 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
   void ResetSnippetsService(
       std::unique_ptr<RemoteSuggestionsProvider>* service) {
     service->reset();
+    category_ranker_ = base::MakeUnique<ConstantCategoryRanker>();

[Marc Treib, 2016/12/14 10:24:31]

If this needs to be reset here, then it should also be created along with the
provider in MakeSnippetsService...
(but maybe it doesn't actually need to be reset?)

[vitaliii, 2016/12/15 15:30:12]

Done. 

It does, ResetSnippetsService must simulate Chrome restart.

     observer_.reset();
     *service = MakeSnippetsService();
   }
 
+  std::vector<Category> SortCategories(std::vector<Category> categories) {

[Marc Treib, 2016/12/14 10:24:31]

Either pass by const ref, or pass in a pointer and sort in-place.

[vitaliii, 2016/12/15 15:30:12]

Done.

The function was removed completely.

+    std::sort(categories.begin(), categories.end(),
+              [this](Category left, Category right) {
+                return category_ranker_->Compare(left, right);
+              });
+    return categories;
+  }
+
   ContentSuggestion::ID MakeArticleID(const std::string& id_within_category) {
     return ContentSuggestion::ID(articles_category(), id_within_category);
   }
 
   Category articles_category() {
-    return category_factory_.FromKnownCategory(KnownCategories::ARTICLES);
+    return Category::FromKnownCategory(KnownCategories::ARTICLES);
   }
 
   ContentSuggestion::ID MakeOtherID(const std::string& id_within_category) {
     return ContentSuggestion::ID(other_category(), id_within_category);
   }
 
-  Category other_category() { return category_factory_.FromRemoteCategory(2); }
+  // TODO(tschumann): Get rid of the convenience other_category() and
+  // unknown_category() helpers -- tests can just define their own.
+  Category other_category() { return Category::FromRemoteCategory(2); }
 
   Category unknown_category() {
-    return category_factory_.FromRemoteCategory(kUnknownRemoteCategoryId);
+    return Category::FromRemoteCategory(kUnknownRemoteCategoryId);
   }
 
  protected:
@@ -542,10 +575,10 @@ class RemoteSuggestionsProviderTest : public ::testing::Test {
   net::FakeURLFetcherFactory fake_url_fetcher_factory_;
   const GURL test_url_;
   std::unique_ptr<OAuth2TokenService> fake_token_service_;
+  std::unique_ptr<CategoryRanker> category_ranker_;
   UserClassifier user_classifier_;
   NiceMock<MockScheduler> scheduler_;
   std::unique_ptr<FakeContentSuggestionsProviderObserver> observer_;
-  CategoryFactory category_factory_;
   NiceMock<MockImageFetcher>* image_fetcher_;
   FakeImageDecoder* image_decoder_;
 
@@ -730,11 +763,12 @@ TEST_F(RemoteSuggestionsProviderTest, CategoryTitle) {
 }
 
 TEST_F(RemoteSuggestionsProviderTest, MultipleCategories) {
-  std::string json_str(
-      GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
-
   auto service = MakeSnippetsService();
-
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategory({GetSnippetN(0)}, /*remote_category_id=*/1)
+          .AddCategory({GetSnippetN(1)}, /*remote_category_id=*/2)
+          .Build();
   LoadFromJSONString(service.get(), json_str);
 
   ASSERT_THAT(observer().statuses(),
@@ -787,12 +821,15 @@ TEST_F(RemoteSuggestionsProviderTest, ArticleCategoryInfo) {
 
 TEST_F(RemoteSuggestionsProviderTest, ExperimentalCategoryInfo) {
   auto service = MakeSnippetsService();
-
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategory({GetSnippetN(0)}, /*remote_category_id=*/1)
+          .AddCategory({GetSnippetN(1)}, kUnknownRemoteCategoryId)
+          .Build();
   // Load data with multiple categories so that a new experimental category gets
   // registered.
-  LoadFromJSONString(service.get(),
-                     GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)},
-                                          kUnknownRemoteCategoryId));
+  LoadFromJSONString(service.get(), json_str);
+
   CategoryInfo info = service->GetCategoryInfo(unknown_category());
   EXPECT_THAT(info.has_more_action(), Eq(false));
   EXPECT_THAT(info.has_reload_action(), Eq(false));
@@ -802,10 +839,15 @@ TEST_F(RemoteSuggestionsProviderTest, ExperimentalCategoryInfo) {
 
 TEST_F(RemoteSuggestionsProviderTest, PersistCategoryInfos) {
   auto service = MakeSnippetsService();
-
-  LoadFromJSONString(service.get(),
-                     GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)},
-                                          kUnknownRemoteCategoryId));
+  // TODO(vitaliii): Use |articles_category()| instead of constant ID below.
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategoryWithCustomTitle(
+              {GetSnippetN(0)}, /*remote_category_id=*/1, "Articles for You")
+          .AddCategoryWithCustomTitle({GetSnippetN(1)},
+                                      kUnknownRemoteCategoryId, "Other Things")
+          .Build();
+  LoadFromJSONString(service.get(), json_str);
 
   ASSERT_EQ(observer().StatusForCategory(articles_category()),
             CategoryStatus::AVAILABLE);
@@ -840,11 +882,40 @@ TEST_F(RemoteSuggestionsProviderTest, PersistCategoryInfos) {
   EXPECT_EQ(info_unknown_before.title(), info_unknown_after.title());
 }
 
-TEST_F(RemoteSuggestionsProviderTest, PersistSuggestions) {
+TEST_F(RemoteSuggestionsProviderTest, PersistRemoteCategoryOrder) {
   auto service = MakeSnippetsService();
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategory({GetSnippetN(0)}, /*remote_category_id=*/1)
+          .AddCategory({GetSnippetN(1)}, /*remote_category_id=*/3)
+          .AddCategory({GetSnippetN(2)}, /*remote_category_id=*/2)
+          .Build();
+  LoadFromJSONString(service.get(), json_str);
 
-  LoadFromJSONString(service.get(),
-                     GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
+  std::vector<Category> unordered_categories = {
+      Category::FromRemoteCategory(1), Category::FromRemoteCategory(2),
+      Category::FromRemoteCategory(3)};
+  std::vector<Category> ordered_categories = {Category::FromRemoteCategory(1),
+                                              Category::FromRemoteCategory(3),
+                                              Category::FromRemoteCategory(2)};
+  ASSERT_EQ(ordered_categories, SortCategories(unordered_categories));
+
+  // Recreate the service to simulate a Chrome restart. The ranker does not
+  // persist the order.
+  ResetSnippetsService(&service);
+
+  // The categories should have been restored in the same order.
+  EXPECT_EQ(ordered_categories, SortCategories(unordered_categories));

[Marc Treib, 2016/12/14 10:24:31]

Hm, I find this hard to follow - it's not easy to see IMO how the SortCategories
helper depends on the provider.
This also kinda tests the ranker more than the provider. I think what we should
really test is that the AppendCategory methods on the ranker are called in the
correct order. Since the ranker is already injectable, that should be fairly
straightforward: Instead of an actual ranker, pass in a Mock.
WDYT?

[vitaliii, 2016/12/15 15:30:12]

Done.

Rewrote with the mock, but I am not sure whether this became any better (there
are a lot of "surprises" inside RemoteSuggestionsProvider). 

+}
+
+TEST_F(RemoteSuggestionsProviderTest, PersistSuggestions) {
+  auto service = MakeSnippetsService();
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategory({GetSnippetN(0)}, /*remote_category_id=*/1)
+          .AddCategory({GetSnippetN(2)}, /*remote_category_id=*/2)
+          .Build();
+  LoadFromJSONString(service.get(), json_str);
 
   ASSERT_THAT(observer().SuggestionsForCategory(articles_category()),
               SizeIs(1));
@@ -862,8 +933,14 @@ TEST_F(RemoteSuggestionsProviderTest, PersistSuggestions) {
 TEST_F(RemoteSuggestionsProviderTest, DontNotifyIfNotAvailable) {
   // Get some suggestions into the database.
   auto service = MakeSnippetsService();
-  LoadFromJSONString(service.get(),
-                     GetMultiCategoryJson({GetSnippetN(0)}, {GetSnippetN(1)}));
+  std::string json_str =
+      MutliCategoryJsonBuilder()
+          .AddCategory({GetSnippetN(0)},
+                       /*remote_category_id=*/1)
+          .AddCategory({GetSnippetN(1)}, /*remote_category_id=*/2)
+          .Build();
+  LoadFromJSONString(service.get(), json_str);
+
   ASSERT_THAT(observer().SuggestionsForCategory(articles_category()),
               SizeIs(1));
   ASSERT_THAT(observer().SuggestionsForCategory(other_category()), SizeIs(1));