[NTP::SectionOrder] Add a flag to choose category ranker.

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

 // Copyright 2016 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 "chrome/browser/ntp_snippets/content_suggestions_service_factory.h"

[jkrcal, 2016/12/22 10:06:01]

Can you the same changes for the iOS factory please?

[vitaliii, 2016/12/22 13:11:37]

Done.

However, I cannot prevent duplication of the function from enum to the ranker
itself.

[jkrcal, 2016/12/22 13:39:15]

That's okay, thanks!

 
 #include <utility>
 
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/suggestions/image_decoder_impl.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/translate/language_model_factory.h"
 #include "chrome/common/channel_info.h"
 #include "chrome/common/chrome_features.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/image_fetcher/image_fetcher_impl.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/keyed_service/core/service_access_type.h"
 #include "components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h"
+#include "components/ntp_snippets/category_rankers/category_ranker.h"
+#include "components/ntp_snippets/category_rankers/click_based_category_ranker.h"
 #include "components/ntp_snippets/category_rankers/constant_category_ranker.h"
 #include "components/ntp_snippets/content_suggestions_service.h"
 #include "components/ntp_snippets/features.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/remote/ntp_snippets_fetcher.h"
 #include "components/ntp_snippets/remote/persistent_scheduler.h"
 #include "components/ntp_snippets/remote/remote_suggestions_database.h"
 #include "components/ntp_snippets/remote/remote_suggestions_provider_impl.h"
 #include "components/ntp_snippets/remote/remote_suggestions_status_service.h"
 #include "components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h"
 #include "components/ntp_snippets/sessions/foreign_sessions_suggestions_provider.h"
 #include "components/ntp_snippets/sessions/tab_delegate_sync_adapter.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/translate/core/browser/language_model.h"
+#include "components/variations/variations_associated_data.h"
 #include "components/version_info/version_info.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
 #include "google_apis/google_api_keys.h"
 #include "net/url_request/url_request_context_getter.h"
 
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/chrome_feature_list.h"
 #include "chrome/browser/android/ntp/ntp_snippets_launcher.h"
@@ skipping 10 matching lines @@
 using offline_pages::OfflinePageModel;
 using offline_pages::OfflinePageModelFactory;
 using physical_web::PhysicalWebDataSource;
 #endif  // OS_ANDROID
 
 using bookmarks::BookmarkModel;
 using content::BrowserThread;
 using history::HistoryService;
 using image_fetcher::ImageFetcherImpl;
 using ntp_snippets::BookmarkSuggestionsProvider;
+using ntp_snippets::CategoryRanker;
+using ntp_snippets::ClickBasedCategoryRanker;
+using ntp_snippets::ConstantCategoryRanker;
 using ntp_snippets::ContentSuggestionsService;
 using ntp_snippets::ForeignSessionsSuggestionsProvider;
 using ntp_snippets::NTPSnippetsFetcher;
 using ntp_snippets::PersistentScheduler;
 using ntp_snippets::RemoteSuggestionsDatabase;
 using ntp_snippets::RemoteSuggestionsProviderImpl;
 using ntp_snippets::RemoteSuggestionsStatusService;
 using ntp_snippets::SchedulingRemoteSuggestionsProvider;
 using ntp_snippets::TabDelegateSyncAdapter;
 using suggestions::ImageDecoderImpl;
@@ skipping 102 matching lines @@
 void RegisterForeignSessionsProvider(SyncService* sync_service,
                                      ContentSuggestionsService* service,
                                      PrefService* pref_service) {
   std::unique_ptr<TabDelegateSyncAdapter> sync_adapter =
       base::MakeUnique<TabDelegateSyncAdapter>(sync_service);
   auto provider = base::MakeUnique<ForeignSessionsSuggestionsProvider>(
       service, std::move(sync_adapter), pref_service);
   service->RegisterProvider(std::move(provider));
 }
 
+std::unique_ptr<CategoryRanker> BuildCategoryRanker(PrefService* pref_service) {
+  std::string category_ranker_parameter = variations::GetVariationParamValue(

[jkrcal, 2016/12/22 10:06:01]

Can you use variations::GetVariationParamValueByFeatureName?

We try to move reading all variation params to this way as it allows us to split
the variation configs into multiple files (multiple studies).

[vitaliii, 2016/12/22 13:11:37]

Done.

+      ntp_snippets::kStudyName, "category_ranker");
+
+  if (category_ranker_parameter.empty()) {
+    // Default, Enabled or Disabled.
+    return base::MakeUnique<ConstantCategoryRanker>();
+  }
+  if (category_ranker_parameter == "constant") {

[jkrcal, 2016/12/22 10:06:01]

Do we need this option? I would have just 
 - Default    = constant, 
 - Enabled    = constant, 
 - Enabled (click based), 
 - Disabled    = constant, bit unfortunately

[tschumann, 2016/12/22 10:31:26]

i like the idea of using the strings in less places and an enum instead. in that
case, the default semantics would also live in a single place (where the strings
get turned into the enum) and here, we could have a simple switch over the value
and no need to interpret defaults etc.

[vitaliii, 2016/12/22 13:11:37]

I think - yes.

1) I treat all of "Default", "Enabled" and "Disabled" as default ranker, so it
is nice to have an option to choose constant ranker once default is changed to
something else.
2) If one is not sure which ranker is considered default, one can explicitly
force constant ranker if needed.

[vitaliii, 2016/12/22 13:11:37]

Done.

[jkrcal, 2016/12/22 13:39:15]

Makes sense. Ack.

+    return base::MakeUnique<ConstantCategoryRanker>();
+  }
+  if (category_ranker_parameter == "click_based") {
+    return base::MakeUnique<ClickBasedCategoryRanker>(pref_service);
+  }
+
+  NOTREACHED() << "The category_ranker parameter value is "
+               << category_ranker_parameter;
+  return nullptr;
+}
+
 }  // namespace
 
 // static
 ContentSuggestionsServiceFactory*
 ContentSuggestionsServiceFactory::GetInstance() {
   return base::Singleton<ContentSuggestionsServiceFactory>::get();
 }
 
 // static
 ContentSuggestionsService* ContentSuggestionsServiceFactory::GetForProfile(
@@ skipping 34 matching lines @@
   // Create the ContentSuggestionsService.
   State state =
       base::FeatureList::IsEnabled(ntp_snippets::kContentSuggestionsFeature)
           ? State::ENABLED
           : State::DISABLED;
   SigninManagerBase* signin_manager =
       SigninManagerFactory::GetForProfile(profile);
   HistoryService* history_service = HistoryServiceFactory::GetForProfile(
       profile, ServiceAccessType::EXPLICIT_ACCESS);
   PrefService* pref_service = profile->GetPrefs();
-  auto category_ranker =
-      base::MakeUnique<ntp_snippets::ConstantCategoryRanker>();
+  std::unique_ptr<CategoryRanker> category_ranker =
+      BuildCategoryRanker(pref_service);
   auto* service =
       new ContentSuggestionsService(state, signin_manager, history_service,
                                     pref_service, std::move(category_ranker));
   if (state == State::DISABLED) {
     // Since we won't initialise the services, they won't get a chance to
     // unschedule their tasks. We do it explicitly here instead.
     ClearScheduledTasks();
     return service;
   }
 
@@ skipping 51 matching lines @@
                             language_model, pref_service, profile);
   }
 
   if (base::FeatureList::IsEnabled(
           ntp_snippets::kForeignSessionsSuggestionsFeature)) {
     RegisterForeignSessionsProvider(sync_service, service, pref_service);
   }
 
   return service;
 }