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

components/ntp_snippets/remote/remote_suggestions_provider.cc

Index: components/ntp_snippets/remote/remote_suggestions_provider.cc
diff --git a/components/ntp_snippets/remote/remote_suggestions_provider.cc b/components/ntp_snippets/remote/remote_suggestions_provider.cc
index a33d4d9a3144c9acbdda695d636799a4b0d8f193..254ebd69c15ce28e023cd6e2489fb7a9d0b3c593 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
@@ -27,6 +27,7 @@
 #include "components/history/core/browser/history_service.h"
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
+#include "components/ntp_snippets/category_rankers/category_ranker.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
@@ -224,9 +225,9 @@ void CallWithEmptyResults(const FetchDoneCallback& callback,
 
 RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     Observer* observer,
-    CategoryFactory* category_factory,
     PrefService* pref_service,
     const std::string& application_language_code,
+    CategoryRanker* category_ranker,
     const UserClassifier* user_classifier,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
@@ -234,12 +235,13 @@ RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
     std::unique_ptr<RemoteSuggestionsDatabase> database,
     std::unique_ptr<RemoteSuggestionsStatusService> status_service)
-    : ContentSuggestionsProvider(observer, category_factory),
+    : ContentSuggestionsProvider(observer),
       state_(State::NOT_INITED),
       pref_service_(pref_service),
       articles_category_(
-          category_factory->FromKnownCategory(KnownCategories::ARTICLES)),
+          Category::FromKnownCategory(KnownCategories::ARTICLES)),
       application_language_code_(application_language_code),
+      category_ranker_(category_ranker),
       user_classifier_(user_classifier),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
@@ -598,7 +600,10 @@ void RemoteSuggestionsProvider::OnDatabaseLoaded(
   NTPSnippet::PtrVector to_delete;
   for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
     Category snippet_category =
-        category_factory()->FromRemoteCategory(snippet->remote_category_id());
+        Category::FromRemoteCategory(snippet->remote_category_id());
+    // The category is added to the ranker to enforce the order among remote
+    // categories.
+    category_ranker_->AppendCategoryIfNecessary(snippet_category);

[tschumann, 2016/12/15 11:50:26]

in google3, a common idiom is to call such methods MaybeAppendCategory(...)

i'm not too happy with the 'ifNecessary' part, maybe 'ifMissing' or 'ifNew'?

Then again, I think this logic needs more work anyways and probably will involve
further renamings.

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

I do not like "Maybe", because then it is not clear when the category is
actually added.

As for other suggestions, here are occurrence counts in Chrome:
- ifNecessary -> 695;
- ifMissing -> 49;
- ifNew -> 17;

So I would prefer to leave it as it is now.

[tschumann, 2016/12/15 18:23:13]

For now it's fine. I'm not sure I agree with the "count in Chrome" approach in
this case. Method names highly depend on context.
"if necessary" conveys as much semantical info as "maybe" because they don't say
anything about the condition under which it is added. To be honest, I don't even
know if it matters.

I'm a bit afraid that we're exposing too many implementation details here. For
example what does "Append" mean here? But let's refactor this once we have the
new ranker and a better understanding of the functionality and contracts.

[vitaliii, 2016/12/16 08:15:43]

Ack.
"Append" means that the category is added to the end. Both "Append" and
appending to the end were intended. I am happy to refactor this once we are more
certain regarding what we need.

     auto content_it = category_contents_.find(snippet_category);
     // We should already know about the category.
     if (content_it == category_contents_.end()) {
@@ -748,6 +753,7 @@ void RemoteSuggestionsProvider::OnFetchFinished(
             std::min(fetched_category.snippets.size(),
                      static_cast<size_t>(kMaxSnippetCount + 1)));
       }
+      category_ranker_->AppendCategoryIfNecessary(fetched_category.category);
       CategoryContent* content =
           UpdateCategoryInfo(fetched_category.category, fetched_category.info);
       content->included_in_last_server_response = true;
@@ -1292,7 +1298,9 @@ void RemoteSuggestionsProvider::RestoreCategoriesFromPrefs() {
     dict->GetBoolean(kCategoryContentAllowFetchingMore,
                      &allow_fetching_more_results);
 
-    Category category = category_factory()->FromIDValue(id);
+    Category category = Category::FromIDValue(id);
+    // The ranker may not persist the order of remote categories.
+    category_ranker_->AppendCategoryIfNecessary(category);
     // TODO(tschumann): The following has a bad smell that category
     // serialization / deserialization should not be done inside this
     // class. We should move that into a central place that also knows how to
@@ -1313,12 +1321,11 @@ void RemoteSuggestionsProvider::StoreCategoriesToPrefs() {
   for (const auto& entry : category_contents_) {
     to_store.emplace_back(entry.first, &entry.second);
   }
-  // Sort them into the proper category order.
+  // The ranker may not persist the order, thus, it is stored by the provider.
   std::sort(to_store.begin(), to_store.end(),
             [this](const std::pair<Category, const CategoryContent*>& left,
                    const std::pair<Category, const CategoryContent*>& right) {
-              return category_factory()->CompareCategories(left.first,
-                                                           right.first);
+              return category_ranker_->Compare(left.first, right.first);
             });
   // Convert the relevant info into a base::ListValue for storage.
   base::ListValue list;