Create IOSChromeContentSuggestionsServiceFactory

ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc

Index: ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
diff --git a/ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc b/ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
index 655623f4a1be0cf67a653cf7ff8204d86b964192..15bb67e7da629282008cbcb610d58975060916e0 100644
--- a/ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
+++ b/ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
@@ -14,6 +14,7 @@
 #include "components/image_fetcher/image_decoder.h"
 #include "components/image_fetcher/image_fetcher.h"
 #include "components/keyed_service/ios/browser_state_dependency_manager.h"
+#include "components/ntp_snippets/content_suggestions_service.h"
 #include "components/ntp_snippets/ntp_snippets_constants.h"
 #include "components/ntp_snippets/ntp_snippets_database.h"
 #include "components/ntp_snippets/ntp_snippets_fetcher.h"
@@ -23,6 +24,7 @@
 #include "components/version_info/version_info.h"
 #include "ios/chrome/browser/application_context.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
 #include "ios/chrome/browser/signin/oauth2_token_service_factory.h"
 #include "ios/chrome/browser/signin/signin_manager_factory.h"
 #include "ios/chrome/browser/suggestions/image_fetcher_impl.h"
@@ -34,6 +36,7 @@
 #include "ios/web/public/web_thread.h"
 #include "net/url_request/url_request_context_getter.h"
 
+using ntp_snippets::ContentSuggestionsService;
 using suggestions::ImageFetcherImpl;
 using suggestions::IOSImageDecoderImpl;
 using suggestions::SuggestionsService;
@@ -91,6 +94,15 @@ IOSChromeNTPSnippetsServiceFactory::BuildServiceInstanceFor(
   ios::ChromeBrowserState* chrome_browser_state =
       ios::ChromeBrowserState::FromBrowserState(browser_state);
   DCHECK(!browser_state->IsOffTheRecord());
+
+  ContentSuggestionsService* content_suggestions_service =
+      IOSChromeContentSuggestionsServiceFactory::GetForBrowserState(
+          chrome_browser_state);

[noyau (Ping after 24h), 2016/07/29 12:35:22]

You pull the Suggestion service here, so this service depends on it, it should
be captured by an additional DependsOn call in the constructor.

But, again, this is reversed and should probably works the other way, with the
SuggestionServiceFactory injecting the right provider we want on the platform.

Going further I'm not even sure the providers need to be keyed services, they
should probably be constructed when the suggestion service starts and do not
need a factory at all.

+  if (content_suggestions_service->state() ==
+      ContentSuggestionsService::State::DISABLED) {
+    return nullptr;
+  }
+
   SigninManager* signin_manager =
       ios::SigninManagerFactory::GetForBrowserState(chrome_browser_state);
   OAuth2TokenService* token_service =
@@ -116,18 +128,23 @@ IOSChromeNTPSnippetsServiceFactory::BuildServiceInstanceFor(
   PrefService* prefs = chrome_browser_state->GetPrefs();
   // TODO(treib,markusheintz): Inject an image_fetcher::ImageDecoder once that's
   // implemented on iOS. crbug.com/609127
-  return base::WrapUnique(new ntp_snippets::NTPSnippetsService(
-      false /* enabled */, prefs, suggestions_service,
-      GetApplicationContext()->GetApplicationLocale(), scheduler,
-      base::WrapUnique(new ntp_snippets::NTPSnippetsFetcher(
-          signin_manager, token_service, request_context, prefs,
-          base::Bind(&ParseJson),
-          GetChannel() == version_info::Channel::STABLE)),
-      base::WrapUnique(new ImageFetcherImpl(request_context.get(),
-                                            web::WebThread::GetBlockingPool())),
-      base::MakeUnique<IOSImageDecoderImpl>(),
-      base::WrapUnique(
-          new ntp_snippets::NTPSnippetsDatabase(database_dir, task_runner)),
-      base::WrapUnique(new ntp_snippets::NTPSnippetsStatusService(
-          signin_manager, sync_service, prefs))));
+  std::unique_ptr<ntp_snippets::NTPSnippetsService> ntp_snippets_service =
+      base::WrapUnique(new ntp_snippets::NTPSnippetsService(
+          false /* enabled */, prefs, suggestions_service,
+          GetApplicationContext()->GetApplicationLocale(), scheduler,
+          base::WrapUnique(new ntp_snippets::NTPSnippetsFetcher(
+              signin_manager, token_service, request_context, prefs,
+              base::Bind(&ParseJson),
+              GetChannel() == version_info::Channel::STABLE)),
+          base::WrapUnique(new ImageFetcherImpl(
+              request_context.get(), web::WebThread::GetBlockingPool())),
+          base::MakeUnique<IOSImageDecoderImpl>(),
+          base::WrapUnique(
+              new ntp_snippets::NTPSnippetsDatabase(database_dir, task_runner)),
+          base::WrapUnique(new ntp_snippets::NTPSnippetsStatusService(
+              signin_manager, sync_service, prefs))));
+
+  content_suggestions_service->RegisterProvider(ntp_snippets_service.get());

[noyau (Ping after 24h), 2016/07/29 12:11:55]

This seems reversed. Shouldn't the factory for the suggestion service do that
instead of it happening here?

[Philipp Keck, 2016/07/29 12:20:28]

No, this is "inverted" on purpose: The ContentSuggestionsService shouldn't have
any (serious) dependencies. Instead, the providers start themselves (or are part
of another service that is started anyway) and inject themselves into the
service. Otherwise the ContentSuggestionsService(Factory) would depend on many
things.

This, for example, allows us to build a "ReadingListProvider" that depends on
both the ContentSuggestionsService and the Reading List module without telling
the ContentSuggestionsService at compile time. So we can flexibly add and remove
such providers and their factories.

[noyau (Ping after 24h), 2016/07/29 12:35:22]

Yes, but you are in a Chrome factory, so this is the place to do such
initialization, what other class is going to do it otherwise? That's the whole
point of factory, building these dependencies and making sure everything is
there.

+
+  return ntp_snippets_service;
 }