[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 4ff354f6b697750382057030828db4b3d856afab..c35e680ea6a7d4ec3eaaf7d1b5cb8c62fea984bf 100644
--- a/components/ntp_snippets/remote/remote_suggestions_provider.cc
+++ b/components/ntp_snippets/remote/remote_suggestions_provider.cc
@@ -29,6 +29,7 @@
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/pref_names.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
+#include "components/ntp_snippets/section_rankers/section_ranker.h"
 #include "components/ntp_snippets/switches.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -223,9 +224,9 @@ void CallWithEmptyResults(const FetchDoneCallback& callback,
 
 RemoteSuggestionsProvider::RemoteSuggestionsProvider(
     Observer* observer,
-    CategoryFactory* category_factory,
     PrefService* pref_service,
     const std::string& application_language_code,
+    SectionRanker* section_ranker,
     const UserClassifier* user_classifier,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
@@ -233,12 +234,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),
+      section_ranker_(section_ranker),
       user_classifier_(user_classifier),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
@@ -597,7 +599,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.
+    section_ranker_->AppendCategoryIfNecessary(snippet_category);

[Marc Treib, 2016/12/13 12:22:42]

Hm. This is one downside of the new design: Before, the Factory (which did the
sorting) always knew about all categories. Now, it's possible to create a
category but forget to register it. What would the ranker do in that case?

[vitaliii, 2016/12/14 08:59:38]

Currently all non-registered categories are in the end.
Among themselves they are sorted by increasing the category id.

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

Should it be some kind of error if someone tries to compare two categories that
haven't been registered?
I'm a bit worried that now it's super easy to forget to register a category,
which could lead to all sorts of subtle bugs with the ordering...

[tschumann, 2016/12/15 13:05:20]

We need to clearly handle the cases where a category can enter the system: there
are 3 places I know of: reading from the database, getting requests from the
remote server, and reading categories from prefs.
We need to take care of those. IMO, consolidating this in one class might make
sense (categoryregistry?)  which knows about all categories and keeps the state
persisted.

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

There are 2 places actually (the database does not enforce the order).
Currently in both cases the order is added to CategoryRanker. Before storing the
order is obtained from CategoryRanker. Note that dynamic rankers will have to
store the order themselves.

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

Done.

I do not understand why you are concerned about 2 unregistered categories, but
not 1.
I added DCHECKs to |Compare| that both categories have been registered.

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

Well, but when we start-up and load the categories from prefs, we need to tell
the ranker about them, no?
And if we read suggestions from the db, that's also a place where we can
potentially see a new category showing up (it shouldn't happen as we store all
known categories in prefs, but that's another place we need to check).
Maybe we should discuss this in person tomorrow -- that  said, it'a tangent to
this CL.

I might not (yet) convinced that the ranker should be involved in persistence.
Of course, it needs to make it's state available for persistence. But it
certainly should not be responsible for storing categories themselves. And
separating category persistence from persisting ranking data of the categories
seems sub-optimal.

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

Of course. We do this already in
RemoteSuggestionsProvider::RestoreCategoriesFromPrefs.
I agree not to consider this question in this CL.
In my view, StoreCategoriesToPrefs() uses |category_contents_| and if something
is missing there, but present in the database, I would consider this a bug.
For this constant ranker, this is not clear. However, for the new dynamic
ranker, it must store the order by itself.
I am not sure what you mean by storing categories. The ranker cannot return a
category not present in sort request, thus, the choice of categories is up to
clients.

     auto content_it = category_contents_.find(snippet_category);
     // We should already know about the category.
     if (content_it == category_contents_.end()) {
@@ -1291,7 +1296,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.
+    section_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
@@ -1312,12 +1319,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.

[Marc Treib, 2016/12/13 12:22:42]

Hm. Makes sense, but this is quite subtle. Do we have a test for this?

Or maybe, we could make persistence part of the ranker's contract. That'd make
it hard to clean up old categories though.

[vitaliii, 2016/12/14 08:59:38]

Done.

We did not have a test for this (actually, I have got a feeling that previously
it did not work). I added a test.

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

What exactly do you think didn't work before?

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

I thought that remote categories were not registered previously.
However, I understood my mistake. Previously The registration was done in
NTPSnippetsFetcher, which is very non obvious place and I missed it.

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

another reason to consolidate that logic in a more central place.

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

Acknowledged.

   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 section_ranker_->Compare(left.first, right.first);
             });
   // Convert the relevant info into a base::ListValue for storage.
   base::ListValue list;