ContentSuggestionsService providers registration refactoring.

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"
 
 #include "base/feature_list.h"
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/singleton.h"
@@ skipping 59 matching lines @@
 
 namespace {
 
 // Clear the tasks that can be scheduled by running services.
 void ClearScheduledTasks() {
 #if defined(OS_ANDROID)
   NTPSnippetsLauncher::Get()->Unschedule();
 #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
 ContentSuggestionsServiceFactory*
 ContentSuggestionsServiceFactory::GetInstance() {
   return base::Singleton<ContentSuggestionsServiceFactory>::get();
 }
 
 // static
 ContentSuggestionsService* ContentSuggestionsServiceFactory::GetForProfile(
@@ skipping 30 matching lines @@
           : State::DISABLED;
   ContentSuggestionsService* service = new ContentSuggestionsService(state);
   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;
   }
 
 #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;
 }