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

components/ntp_snippets/remote/remote_suggestions_provider.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/remote/remote_suggestions_provider.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/time/default_clock.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #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"
 #include "components/ntp_snippets/switches.h"
 #include "components/ntp_snippets/user_classifier.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "components/variations/variations_associated_data.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
@@ skipping 177 matching lines @@
   if (callback.is_null()) {
     return;
   }
   callback.Run(status, std::vector<ContentSuggestion>());
 }
 
 }  // namespace
 
 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,
     std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
     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)),
       image_fetcher_(std::move(image_fetcher)),
       image_decoder_(std::move(image_decoder)),
       database_(std::move(database)),
       status_service_(std::move(status_service)),
       fetch_when_ready_(false),
       nuke_when_initialized_(false),
       thumbnail_requests_throttler_(
@@ skipping 338 matching lines @@
   DCHECK(base::ContainsKey(category_contents_, articles_category_));
 
   base::TimeDelta database_load_time =
       base::TimeTicks::Now() - database_load_start_;
   UMA_HISTOGRAM_MEDIUM_TIMES("NewTabPage.Snippets.DatabaseLoadTime",
                              database_load_time);
 
   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()) {
       DLOG(WARNING) << "Loaded a suggestion for unknown category "
                     << snippet_category << " from the DB; deleting";
       to_delete.emplace_back(std::move(snippet));
       continue;
     }
     CategoryContent* content = &content_it->second;
     if (snippet->is_dismissed()) {
@@ skipping 129 matching lines @@
     for (NTPSnippetsFetcher::FetchedCategory& fetched_category :
          *fetched_categories) {
       // TODO(tschumann): Remove this histogram once we only talk to the content
       // suggestions cloud backend.
       if (fetched_category.category == articles_category_) {
         UMA_HISTOGRAM_SPARSE_SLOWLY(
             "NewTabPage.Snippets.NumArticlesFetched",
             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;
       SanitizeReceivedSnippets(content->dismissed, &fetched_category.snippets);
       IntegrateSnippets(content, std::move(fetched_category.snippets));
     }
   }
 
   // TODO(tschumann): The snippets fetcher needs to signal errors so that we
   // know why we received no data. If an error occured, none of the following
@@ skipping 524 matching lines @@
                           &included_in_last_server_response)) {
       DLOG(WARNING) << "Invalid category pref value, missing '"
                     << kCategoryContentProvidedByServer << "': " << *entry;
       continue;
     }
     bool allow_fetching_more_results = false;
     // This wasn't always around, so it's okay if it's missing.
     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
     // parse data we received from remote backends.
     CategoryInfo info =
         category == articles_category_
             ? BuildArticleCategoryInfo(title)
             : BuildRemoteCategoryInfo(title, allow_fetching_more_results);
     CategoryContent* content = UpdateCategoryInfo(category, info);
     content->included_in_last_server_response =
         included_in_last_server_response;
   }
 }
 
 void RemoteSuggestionsProvider::StoreCategoriesToPrefs() {
   // Collect all the CategoryContents.
   std::vector<std::pair<Category, const CategoryContent*>> to_store;
   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;
   for (const auto& entry : to_store) {
     const Category& category = entry.first;
     const CategoryContent& content = *entry.second;
     auto dict = base::MakeUnique<base::DictionaryValue>();
     dict->SetInteger(kCategoryContentId, category.id());
     // TODO(tschumann): Persist other properties of the CategoryInfo.
     dict->SetString(kCategoryContentTitle, content.info.title());
@@ skipping 14 matching lines @@
 RemoteSuggestionsProvider::CategoryContent::CategoryContent(CategoryContent&&) =
     default;
 
 RemoteSuggestionsProvider::CategoryContent::~CategoryContent() = default;
 
 RemoteSuggestionsProvider::CategoryContent&
 RemoteSuggestionsProvider::CategoryContent::operator=(CategoryContent&&) =
     default;
 
 }  // namespace ntp_snippets