ContentSuggestionsService providers registration refactoring.

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

Index: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
index a2303ce83f9d5ecff08e298adbb183bf05a82251..f2236781853bc8e9e9f3c55ba6ad66ea32b488fe 100644
--- a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -77,6 +77,87 @@ void ClearScheduledTasks() {
 #endif  // OS_ANDROID
 }
 
+void RegisterOfflinePageProvider(ContentSuggestionsService* service,
+                                 Profile* profile) {
+  bool recent_tabs_enabled = base::FeatureList::IsEnabled(
+      ntp_snippets::kRecentOfflineTabSuggestionsFeature);
+  bool downloads_enabled =
+      base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature);
+  bool download_manager_ui_enabled =
+      base::FeatureList::IsEnabled(chrome::android::kDownloadsUiFeature);
+  OfflinePageModel* offline_page_model =
+      OfflinePageModelFactory::GetForBrowserContext(profile);
+
+  std::unique_ptr<OfflinePageSuggestionsProvider>
+      offline_page_suggestions_provider =
+          base::MakeUnique<OfflinePageSuggestionsProvider>(
+              recent_tabs_enabled, downloads_enabled,
+              download_manager_ui_enabled, service, service->category_factory(),
+              offline_page_model, profile->GetPrefs());
+  service->RegisterProvider(std::move(offline_page_suggestions_provider));
+}
+
+void RegisterBookmarkProvider(ContentSuggestionsService* service,
+                              Profile* profile) {
+  BookmarkModel* bookmark_model =
+      BookmarkModelFactory::GetForBrowserContext(profile);
+  std::unique_ptr<BookmarkSuggestionsProvider> bookmark_suggestions_provider =
+      base::MakeUnique<BookmarkSuggestionsProvider>(
+          service, service->category_factory(), bookmark_model);
+  service->RegisterProvider(std::move(bookmark_suggestions_provider));
+}
+
+void RegisterPhysicalWebPageProvider(ContentSuggestionsService* service) {
+  std::unique_ptr<PhysicalWebPageSuggestionsProvider>
+      physical_web_page_suggestions_provider =
+          base::MakeUnique<PhysicalWebPageSuggestionsProvider>(
+              service, service->category_factory());
+  service->RegisterProvider(std::move(physical_web_page_suggestions_provider));
+}
+
+void RegisterArticleProvider(ContentSuggestionsService* service,
+                             Profile* profile) {
+  SigninManagerBase* signin_manager =
+      SigninManagerFactory::GetForProfile(profile);

[dgn, 2016/08/17 14:07:29]

We need to specifically declare the dependency on other services in the
factory's constructor, so I'd move getting these services out of the helper
functions and back in BuildServiceInstanceFor, it would make it clearer what the
dependencies are for IMO.

[tschumann, 2016/08/18 17:41:29]

So you are concerned about keeping the DependsOn() calls in sync with accessing
the factories?
This is a challenge we're facing anyways. Especially looking at the insanely
growing BuildInstanceFor() function (where we need to keep track of those as
well).

Ideally, issues like these would be caught by unit tests -- and the fact that we
don't have tests here at all is actually really concerning. How hard would it be
to have these things tested?
Lacking tests, anything can happen ;-)

IMO, this CL already improves the situation as I can skim BuildInstanceFor()
quickly and see which providers get registered at a glance. We could better
reflect these dependencies by handing in the service's themselves instead of the
profile and have the function call GetForProfile().

Then at the calling side, you'd see the accessed services.

[dgn, 2016/08/19 09:25:14]

Yes, I agree, this CL already helps a lot. What I was suggesting is just that it
would be nice to have BuildInstanceFor grab all the services, instead of the
registration functions  doing that. So something like

Constructor() {
Dependson(FactoryA)
Dependson(FactoryB)
Dependson(FactoryC)
}

BuildInstanceFor() {
A = FactoryA::GetInstance()
B = FactoryB::GetInstance()
C = FactoryC::GetInstance()

RegisterProviderX()
RegisterProviderY(A, B)
RegisterProviderZ(C)
}

That makes tracking why dependencies are needed clearer. I guess that's what
you're describing in your last paragraph. But anyway, that's just a nit, I think
it would be a nice addition but don't feel too strongly about it.

[vitaliii, 2016/08/25 13:44:18]

Done.

+  OAuth2TokenService* token_service =
+      ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
+  scoped_refptr<net::URLRequestContextGetter> request_context =
+      content::BrowserContext::GetDefaultStoragePartition(profile)
+          ->GetURLRequestContext();
+  SuggestionsService* suggestions_service =
+      SuggestionsServiceFactory::GetForProfile(profile);
+  NTPSnippetsScheduler* scheduler = nullptr;
+#if defined(OS_ANDROID)
+  scheduler = NTPSnippetsLauncher::Get();
+#endif  // OS_ANDROID
+  base::FilePath database_dir(
+      profile->GetPath().Append(ntp_snippets::kDatabaseFolder));
+  scoped_refptr<base::SequencedTaskRunner> task_runner =
+      BrowserThread::GetBlockingPool()
+          ->GetSequencedTaskRunnerWithShutdownBehavior(
+              base::SequencedWorkerPool::GetSequenceToken(),
+              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  bool is_stable_channel =
+      chrome::GetChannel() == version_info::Channel::STABLE;
+  std::unique_ptr<NTPSnippetsService> ntp_snippets_service =
+      base::MakeUnique<NTPSnippetsService>(
+          service, service->category_factory(), profile->GetPrefs(),
+          suggestions_service, g_browser_process->GetApplicationLocale(),
+          scheduler,
+          base::MakeUnique<NTPSnippetsFetcher>(
+              signin_manager, token_service, request_context,
+              profile->GetPrefs(), service->category_factory(),
+              base::Bind(&safe_json::SafeJsonParser::Parse), is_stable_channel),
+          base::MakeUnique<ImageFetcherImpl>(
+              base::MakeUnique<ImageDecoderImpl>(), request_context.get()),
+          base::MakeUnique<ImageDecoderImpl>(),
+          base::MakeUnique<NTPSnippetsDatabase>(database_dir, task_runner),
+          base::MakeUnique<NTPSnippetsStatusService>(signin_manager,
+                                                     profile->GetPrefs()));
+  service->set_ntp_snippets_service(ntp_snippets_service.get());
+  service->RegisterProvider(std::move(ntp_snippets_service));
+}
+
 }  // namespace
 
 // static
@@ -127,93 +208,26 @@ KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
   }
 
 #if defined(OS_ANDROID)
-  // Create the OfflinePageSuggestionsProvider.
-  bool recent_tabs_enabled = base::FeatureList::IsEnabled(
-      ntp_snippets::kRecentOfflineTabSuggestionsFeature);
-  bool downloads_enabled =
-      base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature);
-  bool download_manager_ui_enabled =
-      base::FeatureList::IsEnabled(chrome::android::kDownloadsUiFeature);
-  if (recent_tabs_enabled || downloads_enabled) {
-    OfflinePageModel* offline_page_model =
-        OfflinePageModelFactory::GetForBrowserContext(profile);
-
-    std::unique_ptr<OfflinePageSuggestionsProvider>
-        offline_page_suggestions_provider =
-            base::MakeUnique<OfflinePageSuggestionsProvider>(
-                recent_tabs_enabled, downloads_enabled,
-                download_manager_ui_enabled, service,
-                service->category_factory(), offline_page_model,
-                profile->GetPrefs());
-    service->RegisterProvider(std::move(offline_page_suggestions_provider));
+  if (base::FeatureList::IsEnabled(
+          ntp_snippets::kRecentOfflineTabSuggestionsFeature) ||
+      base::FeatureList::IsEnabled(ntp_snippets::kDownloadSuggestionsFeature)) {
+    RegisterOfflinePageProvider(service, profile);
   }
 #endif  // OS_ANDROID
 
-  // Create the BookmarkSuggestionsProvider.
-  if (base::FeatureList::IsEnabled(
-          ntp_snippets::kBookmarkSuggestionsFeature)) {
-    BookmarkModel* bookmark_model =
-        BookmarkModelFactory::GetForBrowserContext(profile);
-    std::unique_ptr<BookmarkSuggestionsProvider> bookmark_suggestions_provider =
-        base::MakeUnique<BookmarkSuggestionsProvider>(
-            service, service->category_factory(), bookmark_model);
-    service->RegisterProvider(std::move(bookmark_suggestions_provider));
+  if (base::FeatureList::IsEnabled(ntp_snippets::kBookmarkSuggestionsFeature)) {
+    RegisterBookmarkProvider(service, profile);
   }
 
 #if defined(OS_ANDROID)
-  // Create the PhysicalWebPageSuggestionsProvider.
   if (base::FeatureList::IsEnabled(
           ntp_snippets::kPhysicalWebPageSuggestionsFeature)) {
-    std::unique_ptr<PhysicalWebPageSuggestionsProvider>
-        physical_web_page_suggestions_provider =
-            base::MakeUnique<PhysicalWebPageSuggestionsProvider>(
-                service, service->category_factory());
-    service->RegisterProvider(
-        std::move(physical_web_page_suggestions_provider));
+    RegisterPhysicalWebPageProvider(service);
   }
 #endif  // OS_ANDROID
 
   if (base::FeatureList::IsEnabled(ntp_snippets::kArticleSuggestionsFeature)) {
-    // Create the NTPSnippetsService (articles provider).
-    SigninManagerBase* signin_manager =
-        SigninManagerFactory::GetForProfile(profile);
-    OAuth2TokenService* token_service =
-        ProfileOAuth2TokenServiceFactory::GetForProfile(profile);
-    scoped_refptr<net::URLRequestContextGetter> request_context =
-        content::BrowserContext::GetDefaultStoragePartition(context)
-            ->GetURLRequestContext();
-    SuggestionsService* suggestions_service =
-        SuggestionsServiceFactory::GetForProfile(profile);
-    NTPSnippetsScheduler* scheduler = nullptr;
-#if defined(OS_ANDROID)
-    scheduler = NTPSnippetsLauncher::Get();
-#endif  // OS_ANDROID
-    base::FilePath database_dir(
-        profile->GetPath().Append(ntp_snippets::kDatabaseFolder));
-    scoped_refptr<base::SequencedTaskRunner> task_runner =
-        BrowserThread::GetBlockingPool()
-            ->GetSequencedTaskRunnerWithShutdownBehavior(
-                base::SequencedWorkerPool::GetSequenceToken(),
-                base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
-    bool is_stable_channel =
-        chrome::GetChannel() == version_info::Channel::STABLE;
-    std::unique_ptr<NTPSnippetsService> ntp_snippets_service =
-        base::MakeUnique<NTPSnippetsService>(
-            service, service->category_factory(), profile->GetPrefs(),
-            suggestions_service, g_browser_process->GetApplicationLocale(),
-            scheduler, base::MakeUnique<NTPSnippetsFetcher>(
-                           signin_manager, token_service, request_context,
-                           profile->GetPrefs(), service->category_factory(),
-                           base::Bind(&safe_json::SafeJsonParser::Parse),
-                           is_stable_channel),
-            base::MakeUnique<ImageFetcherImpl>(
-                base::MakeUnique<ImageDecoderImpl>(), request_context.get()),
-            base::MakeUnique<ImageDecoderImpl>(),
-            base::MakeUnique<NTPSnippetsDatabase>(database_dir, task_runner),
-            base::MakeUnique<NTPSnippetsStatusService>(signin_manager,
-                                                       profile->GetPrefs()));
-    service->set_ntp_snippets_service(ntp_snippets_service.get());
-    service->RegisterProvider(std::move(ntp_snippets_service));
+    RegisterArticleProvider(service, profile);
   }
 
   return service;