ContentSuggestionsService providers registration refactoring.

ContentSuggestionsService providers registration refactoring.

BUG=633139
Committed: https://crrev.com/496451c110e32151e4ce05dc4e2e0ca83c69a3df
Cr-Commit-Position: refs/heads/master@{#414481}

[vitaliii, 2016-08-12 12:11:16]

vitaliii@chromium.org changed reviewers:
+ treib@chromium.org, tschumann@chromium.org

[Marc Treib, 2016-08-12 12:15:52]

treib@chromium.org changed reviewers:
+ pke@google.com

[Marc Treib, 2016-08-12 12:15:52]

+pke FYI, since this will conflict with
https://codereview.chromium.org/2245583002/
One of you will have to rebase :)

[Marc Treib, 2016-08-12 12:18:45]

Thanks! Mostly looks good, just some small comments.

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:80: void
RegisterOfflinePageSuggestionsProvider(ContentSuggestionsService* service,
optional naming proposal: Should we remove "Suggestions" from all these function
names? It'd make them a bit shorter, and it's IMO clear from context.

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:84: 
nit: I'd remove the empty line; the other Register* functions don't have it
either.

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:113:
content::BrowserContext* context) {
Profile and BrowserContext are actually the same, it's enough to pass just the
Profile and use that instead of context

[vitaliii, 2016-08-12 12:20:46]

Description was changed from

==========
ContentSuggestionsService providers registration refactoring.

BUG=
==========

to

==========
ContentSuggestionsService providers registration refactoring.

BUG=637248
==========

[Marc Treib, 2016-08-12 12:22:52]

Ah, also: You can either add bug 633139 to the description, since that seems
fitting, or just say "BUG=none".

[vitaliii, 2016-08-12 12:25:07]

Description was changed from

==========
ContentSuggestionsService providers registration refactoring.

BUG=637248
==========

to

==========
ContentSuggestionsService providers registration refactoring.

BUG=633139
==========

[vitaliii, 2016-08-12 12:33:29]

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:80: void
RegisterOfflinePageSuggestionsProvider(ContentSuggestionsService* service,
On 2016/08/12 12:18:45, Marc Treib wrote:
> optional naming proposal: Should we remove "Suggestions" from all these
function
> names? It'd make them a bit shorter, and it's IMO clear from context.

Done.

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:84: 
On 2016/08/12 12:18:45, Marc Treib wrote:
> nit: I'd remove the empty line; the other Register* functions don't have it
> either.

Done.

https://codereview.chromium.org/2245633002/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:113:
content::BrowserContext* context) {
On 2016/08/12 12:18:45, Marc Treib wrote:
> Profile and BrowserContext are actually the same, it's enough to pass just the
> Profile and use that instead of context

Done.

[Marc Treib, 2016-08-12 12:35:16]

LGTM, thanks! (but please coordinate with Philipp about landing)

[tschumann, 2016-08-12 12:38:47]

lgtm

[Marc Treib, 2016-08-17 12:28:41]

Still LGTM after the rebase

[vitaliii, 2016-08-17 12:34:42]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-08-17 12:34:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2245633002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-17 12:34:59]

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-17 12:45:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 12:45:05]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgn, 2016-08-17 14:07:28]

dgn@chromium.org changed reviewers:
+ dgn@chromium.org

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

drive by nit

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:121:
SigninManagerFactory::GetForProfile(profile);
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]

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:121:
SigninManagerFactory::GetForProfile(profile);
On 2016/08/17 14:07:29, dgn wrote:
> 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.

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]

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:121:
SigninManagerFactory::GetForProfile(profile);
On 2016/08/18 17:41:29, tschumann wrote:
> On 2016/08/17 14:07:29, dgn wrote:
> > 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.
> 
> 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.
> 
> 

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.

[Philipp Keck, 2016-08-19 09:30:16]

Btw, there's also the IOSChromeContentSuggestionsServiceFactory:
https://codereview.chromium.org/2232473002/

[vitaliii, 2016-08-25 13:42:31]

Patchset #4 (id:60001) has been deleted

[vitaliii, 2016-08-25 13:44:17]

vitaliii@chromium.org changed reviewers:
- dgn@chromium.org, pke@google.com, tschumann@chromium.org

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

Hi Marc,

would you mind to have a look one more time?

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/40001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:121:
SigninManagerFactory::GetForProfile(profile);
On 2016/08/19 09:25:14, dgn wrote:
> On 2016/08/18 17:41:29, tschumann wrote:
> > On 2016/08/17 14:07:29, dgn wrote:
> > > 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.
> > 
> > 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.
> > 
> > 
> 
> 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.

Done.

[Marc Treib, 2016-08-25 13:51:54]

https://codereview.chromium.org/2245633002/diff/80001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/80001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:95: if
(recent_tabs_enabled || downloads_enabled) {
Hm, so this method checks the features itself, but all the others don't. Can we
make that consistent?

[vitaliii, 2016-08-25 14:24:24]

Hi Marc,

please have a look.

https://codereview.chromium.org/2245633002/diff/80001/chrome/browser/ntp_snip...
File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right):

https://codereview.chromium.org/2245633002/diff/80001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:95: if
(recent_tabs_enabled || downloads_enabled) {
On 2016/08/25 13:51:54, Marc Treib wrote:
> Hm, so this method checks the features itself, but all the others don't. Can
we
> make that consistent?

Done.

[Marc Treib, 2016-08-25 14:26:15]

LGTM, thanks!

[vitaliii, 2016-08-25 14:26:36]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-08-25 14:26:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2245633002/#ps100001
(title: "Marc's comment.")

[commit-bot: I haz the power, 2016-08-25 14:27:05]

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-25 14:43:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 14:43:34]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[vitaliii, 2016-08-25 17:00:27]

The CQ bit was checked by vitaliii@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:01:18]

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-25 17:14:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:14:38]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vitaliii, 2016-08-25 17:25:38]

The CQ bit was checked by vitaliii@chromium.org

[vitaliii, 2016-08-25 17:25:39]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2245633002/#ps120001
(title: "Marked Android specific code.")

[commit-bot: I haz the power, 2016-08-25 17:26:26]

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-25 18:22:59]

Description was changed from

==========
ContentSuggestionsService providers registration refactoring.

BUG=633139
==========

to

==========
ContentSuggestionsService providers registration refactoring.

BUG=633139
==========

[commit-bot: I haz the power, 2016-08-25 18:23:00]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-08-25 18:25:38]

Description was changed from

==========
ContentSuggestionsService providers registration refactoring.

BUG=633139
==========

to

==========
ContentSuggestionsService providers registration refactoring.

BUG=633139

Committed: https://crrev.com/496451c110e32151e4ce05dc4e2e0ca83c69a3df
Cr-Commit-Position: refs/heads/master@{#414481}
==========

[commit-bot: I haz the power, 2016-08-25 18:25:39]

Patchset 6 (id:??) landed as
https://crrev.com/496451c110e32151e4ce05dc4e2e0ca83c69a3df
Cr-Commit-Position: refs/heads/master@{#414481}