Create IOSChromeContentSuggestionsServiceFactory

Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me

Committed: https://crrev.com/ca76f9c6a37b34de6d70b3e4e1ddb9c6f61c3041
Cr-Commit-Position: refs/heads/master@{#408934}

[Philipp Keck, 2016-07-29 12:01:03]

pke@google.com changed reviewers:
+ markusheintz@chromium.org, noyau@chromium.org

[Philipp Keck, 2016-07-29 12:01:03]

PTAL

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

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:24:
#include
"ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
You included the header but didn't call GetInstance().

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:147:
content_suggestions_service->RegisterProvider(ntp_snippets_service.get());
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]

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:24:
#include
"ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
On 2016/07/29 12:11:55, noyau wrote:
> You included the header but didn't call GetInstance().

Right. I think the ContentSuggestionsService doesn't need to be instantiated
"just because". Whoever needs it first causes it to be instantiated. I was about
to remove it from profile_manager.cc (which seems to be the Android equivalent
of this file?), but then I reset that (because we couldn't move the factory to
components/).

So given that iOS doesn't even use the service at the moment, I think it can
definitely be removed here, so I removed the header.

On a separate CL, I might also remove it from profile_manager.

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:147:
content_suggestions_service->RegisterProvider(ntp_snippets_service.get());
On 2016/07/29 12:11:55, noyau wrote:
> This seems reversed. Shouldn't the factory for the suggestion service do that
> instead of it happening here?

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:23]

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:24:
#include
"ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
On 2016/07/29 12:20:28, Philipp Keck wrote:
> On 2016/07/29 12:11:55, noyau wrote:
> > You included the header but didn't call GetInstance().
> 
> Right. I think the ContentSuggestionsService doesn't need to be instantiated
> "just because". Whoever needs it first causes it to be instantiated. I was
about
> to remove it from profile_manager.cc (which seems to be the Android equivalent
> of this file?), but then I reset that (because we couldn't move the factory to
> components/).
> 
> So given that iOS doesn't even use the service at the moment, I think it can
> definitely be removed here, so I removed the header.
> 
> On a separate CL, I might also remove it from profile_manager.

This loads the factories, not instantiate the services. This is used to build
the dependency graph, so you definitely need it there.

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
File ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:100:
chrome_browser_state);
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.

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/ntp_...
ios/chrome/browser/ntp_snippets/ios_chrome_ntp_snippets_service_factory.cc:147:
content_suggestions_service->RegisterProvider(ntp_snippets_service.get());
On 2016/07/29 12:20:28, Philipp Keck wrote:
> On 2016/07/29 12:11:55, noyau wrote:
> > This seems reversed. Shouldn't the factory for the suggestion service do
that
> > instead of it happening here?
> 
> 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.

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.

[Philipp Keck, 2016-08-01 08:24:43]

Description was changed from

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Hook up the NTPSnippetsService as a provider with the
ContentSuggestionsService.

BUG=632690
==========

to

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me
==========

[Philipp Keck, 2016-08-01 08:27:53]

As discussed with Eric, I removed the IOSChromeNTPSnippetsServiceFactory
entirely, as the providers will be created by the central service factory in the
future. The corresponding changes will follow along with the changes the for the
Android part later this week.

I also modified the description of this issue.

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
File ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
(right):

https://codereview.chromium.org/2194793004/diff/20001/ios/chrome/browser/brow...
ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm:24:
#include
"ios/chrome/browser/ntp_snippets/ios_chrome_content_suggestions_service_factory.h"
On 2016/07/29 12:35:22, noyau wrote:
> On 2016/07/29 12:20:28, Philipp Keck wrote:
> > On 2016/07/29 12:11:55, noyau wrote:
> > > You included the header but didn't call GetInstance().
> > 
> > Right. I think the ContentSuggestionsService doesn't need to be instantiated
> > "just because". Whoever needs it first causes it to be instantiated. I was
> about
> > to remove it from profile_manager.cc (which seems to be the Android
equivalent
> > of this file?), but then I reset that (because we couldn't move the factory
to
> > components/).
> > 
> > So given that iOS doesn't even use the service at the moment, I think it can
> > definitely be removed here, so I removed the header.
> > 
> > On a separate CL, I might also remove it from profile_manager.
> 
> This loads the factories, not instantiate the services. This is used to build
> the dependency graph, so you definitely need it there.

Done.

[noyau (Ping after 24h), 2016-08-01 08:38:51]

lgtm

[Philipp Keck, 2016-08-01 08:42:05]

The CQ bit was checked by pke@google.com

[commit-bot: I haz the power, 2016-08-01 08:42:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 09:41:55]

Description was changed from

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me
==========

to

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me
==========

[commit-bot: I haz the power, 2016-08-01 09:41:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-01 09:43:28]

Description was changed from

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me
==========

to

==========
Create IOSChromeContentSuggestionsServiceFactory

Create the factory so that the new ContentSuggestionsService is
available on iOS. It is currently always disabled, but the factory
ensures that the service doesn't introduce new non-iOS dependencies.

Remove the IOSChromeNTPSnippetsServiceFactory because the
NTPSnippetsService is now a plain provider and will not be
a KeyedService anymore. It will be created by the new
IOSChromeContentSuggestionsServiceFactory in the future.

BUG=632690
Owner: me

Committed: https://crrev.com/ca76f9c6a37b34de6d70b3e4e1ddb9c6f61c3041
Cr-Commit-Position: refs/heads/master@{#408934}
==========

[commit-bot: I haz the power, 2016-08-01 09:43:30]

Patchset 5 (id:??) landed as
https://crrev.com/ca76f9c6a37b34de6d70b3e4e1ddb9c6f61c3041
Cr-Commit-Position: refs/heads/master@{#408934}