[Remote suggestions] Refactor the scheduler

[Remote suggestions] Refactor the scheduler

This CL cleans up the relation of the scheduler and the provider of
remote suggestions. The previous wrapper pattern was not useful as there
was too much two-way communication between these two.

BUG=695447
Review-Url: https://codereview.chromium.org/2774663002
Cr-Commit-Position: refs/heads/master@{#460414}
Committed: https://chromium.googlesource.com/chromium/src/+/f996646c9b3b4c8027e693c5a48c7fab3adbfe8c

[jkrcal, 2017-03-23 15:40:11]

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

[jkrcal, 2017-03-23 15:40:12]

Tim, Marc, this is the first version of the main refactoring change. I would
like to get your agreement that this is the way to go before I touch the
unit-tests. Could you PTAL?

[Marc Treib, 2017-03-23 16:41:15]

Overall direction looks good to me; some comments on the details.

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

https://codereview.chromium.org/2774663002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:231:
service->set_remote_suggestions_scheduler(std::move(scheduler));
Hm, the order of initialization is a bit convoluted here, since everyone depends
on everyone else.

Since the service now owns the scheduler, I'd prefer if we could pass it there
in the constructor. That means we'd have to pass the provider pointer to the
scheduler later, but that seems preferable overall: The owning pointer is there
right away, and the weak pointer is passed later. WDYT?

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:328:
!remote_suggestions_scheduler_->AcquireQuotaForInteractiveFetch()) {
Hm. So if there's no scheduler, we don't bother with quota? That seems kinda
dangerous.

[tschumann, 2017-03-23 17:38:27]

overall looks good. some details.

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

https://codereview.chromium.org/2774663002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:231:
service->set_remote_suggestions_scheduler(std::move(scheduler));
On 2017/03/23 16:41:15, Marc Treib wrote:
> Hm, the order of initialization is a bit convoluted here, since everyone
depends
> on everyone else.
> 
> Since the service now owns the scheduler, I'd prefer if we could pass it there
> in the constructor. That means we'd have to pass the provider pointer to the
> scheduler later, but that seems preferable overall: The owning pointer is
there
> right away, and the weak pointer is passed later. WDYT?

Sounds good to me. Ownership is best expressed with constructors.

Related but a different refactoring: It's a bit weird that the provider's
constructor takes an observer. The nice things about observers is usually that
you don't need them at construction time...

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:328:
!remote_suggestions_scheduler_->AcquireQuotaForInteractiveFetch()) {
On 2017/03/23 16:41:15, Marc Treib wrote:
> Hm. So if there's no scheduler, we don't bother with quota? That seems kinda
> dangerous.

+1, let's return early if we don't have a scheduler explicitly.
should we make check part of ready() and simply use that?

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
this should never be nullptr, right? remove the if?
Didn't we think about making this call by wrapping the fetching_callback?
(same below)
That way, we also had a single place to verify the scheduler is not nullptr and
don't have to worry about that state here.

[jkrcal, 2017-03-27 09:23:53]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 09:24:10]

Dry run: 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, 2017-03-27 09:28:01]

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

[commit-bot: I haz the power, 2017-03-27 09:28:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[jkrcal, 2017-03-27 09:57:10]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 09:57:21]

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

[jkrcal, 2017-03-27 10:03:03]

Marc, could you PTAL again?

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

https://codereview.chromium.org/2774663002/diff/20001/chrome/browser/ntp_snip...
chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:231:
service->set_remote_suggestions_scheduler(std::move(scheduler));
On 2017/03/23 17:38:27, tschumann wrote:
> On 2017/03/23 16:41:15, Marc Treib wrote:
> > Hm, the order of initialization is a bit convoluted here, since everyone
> depends
> > on everyone else.
> > 
> > Since the service now owns the scheduler, I'd prefer if we could pass it
there
> > in the constructor. That means we'd have to pass the provider pointer to the
> > scheduler later, but that seems preferable overall: The owning pointer is
> there
> > right away, and the weak pointer is passed later. WDYT?
> 
> Sounds good to me. Ownership is best expressed with constructors.
> 
> Related but a different refactoring: It's a bit weird that the provider's
> constructor takes an observer. The nice things about observers is usually that
> you don't need them at construction time...

Done.

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:328:
!remote_suggestions_scheduler_->AcquireQuotaForInteractiveFetch()) {
On 2017/03/23 17:38:27, tschumann wrote:
> On 2017/03/23 16:41:15, Marc Treib wrote:
> > Hm. So if there's no scheduler, we don't bother with quota? That seems kinda
> > dangerous.
> 
> +1, let's return early if we don't have a scheduler explicitly.
> should we make check part of ready() and simply use that?

Done. I cannot make it part of ready() because these calls should make it
further when the DB is not ready().

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/23 17:38:27, tschumann wrote:
> this should never be nullptr, right? remove the if?
> Didn't we think about making this call by wrapping the fetching_callback?
> (same below)
> That way, we also had a single place to verify the scheduler is not nullptr
and
> don't have to worry about that state here.

Correct, removed the ifs here and below.

I am not sure about the wrappers. 
 - We use wrapper callbacks for fetches initialized from the scheduler.
 - It is the responsibility of the provider to ask for quota & inform about
status for interactive fetches. I am not sure it is cleaner to set it up all
here compared to the current situation. The two callbacks have different
signatures, for one of them, I would need to introduce a helper function...

Since Tim is OOO, Marc what it your opinion?

[commit-bot: I haz the power, 2017-03-27 10:05:51]

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

[commit-bot: I haz the power, 2017-03-27 10:05:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Marc Treib, 2017-03-27 11:31:01]

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 10:03:02, jkrcal wrote:
> On 2017/03/23 17:38:27, tschumann wrote:
> > this should never be nullptr, right? remove the if?
> > Didn't we think about making this call by wrapping the fetching_callback?
> > (same below)
> > That way, we also had a single place to verify the scheduler is not nullptr
> and
> > don't have to worry about that state here.
> 
> Correct, removed the ifs here and below.
> 
> I am not sure about the wrappers. 
>  - We use wrapper callbacks for fetches initialized from the scheduler.
>  - It is the responsibility of the provider to ask for quota & inform about
> status for interactive fetches. I am not sure it is cleaner to set it up all
> here compared to the current situation. The two callbacks have different
> signatures, for one of them, I would need to introduce a helper function...
> 
> Since Tim is OOO, Marc what it your opinion?

Not sure I understand the proposal... who would wrap the callback?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:89:
ContentSuggestionsService(
Add a comment saying which of the passed-in ptrs may be null?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:228: // influence on
scheduling remote fetches.
Mention that it can be null?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.h:120:
RemoteSuggestionsScheduler* scheduler,
Can the scheduler be null? If so, please add a comment here. If not, remove the
ifs in the .cc :)

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:561:
.WillByDefault(Return(true));
Hm. Could this be an EXPECT_CALL with RetiresOnSaturation? Seems like that would
mess less with the tests' own expectations.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:580:
NiceMock<MockScheduler> scheduler_;
Hm hm. Okay for now I think, but could we add a TODO to make this a StrictMock?
Or do you have good reasons for keeping it Nice?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:9: #include
"components/ntp_snippets/remote/remote_suggestions_provider.h"
I think this can just be forward declared.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:38: virtual void
OnInteractiveFetchFinished(Status fetch_status) = 0;
#include for Status?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:65:
RemoteSuggestionsProvider* provider_;
Maybe move this out of the interface and into the impl, so that this stays an
actual interface?
If you keep it here, then you also have to add a ctor to initialize it. Also I'd
prefer to make it private and add an accessor.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:220: :
RemoteSuggestionsScheduler(),
Not needed

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:417:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) {
"RemoteSuggestionsSchedulerImpl::" not required

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:444:
std::move(callback)));
nit: Inline the Bind() call here? Then the move isn't required.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:448:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) {
"RemoteSuggestionsSchedulerImpl::" not required

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:474:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) const {
Also here

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:15: #include
"base/time/default_clock.h"
s/default_clock/clock/

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:52: //
Scheduler interface. crbug.com/695447
I think most of these comments are obsolete now.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:165:
std::set<RemoteSuggestionsSchedulerImpl::TriggerType> enabled_triggers_;
"RemoteSuggestionsSchedulerImpl::" isn't needed

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:128:
scheduler_(nullptr),
nit: unique_ptrs initialize themselves to nullptr

[jkrcal, 2017-03-27 14:02:45]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 14:02:57]

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

[jkrcal, 2017-03-27 14:09:32]

Thanks for the detailed review! PTAL, again.

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 11:31:00, Marc Treib wrote:
> On 2017/03/27 10:03:02, jkrcal wrote:
> > On 2017/03/23 17:38:27, tschumann wrote:
> > > this should never be nullptr, right? remove the if?
> > > Didn't we think about making this call by wrapping the fetching_callback?
> > > (same below)
> > > That way, we also had a single place to verify the scheduler is not
nullptr
> > and
> > > don't have to worry about that state here.
> > 
> > Correct, removed the ifs here and below.
> > 
> > I am not sure about the wrappers. 
> >  - We use wrapper callbacks for fetches initialized from the scheduler.
> >  - It is the responsibility of the provider to ask for quota & inform about
> > status for interactive fetches. I am not sure it is cleaner to set it up all
> > here compared to the current situation. The two callbacks have different
> > signatures, for one of them, I would need to introduce a helper function...
> > 
> > Since Tim is OOO, Marc what it your opinion?
> 
> Not sure I understand the proposal... who would wrap the callback?

I am not sure either :D

The only solution I can imagine is that 
 - ReloadSuggestions() actually provides a callback to FetchSuggestions() that
does the work being done here;
 - Fetch() wraps the callback provided to FetchSnippets() to do the work being
done here.

These two callbacks have a different signature, so I see more ugliness than
elegance in this whole idea.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:89:
ContentSuggestionsService(
On 2017/03/27 11:31:00, Marc Treib wrote:
> Add a comment saying which of the passed-in ptrs may be null?

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:228: // influence on
scheduling remote fetches.
On 2017/03/27 11:31:00, Marc Treib wrote:
> Mention that it can be null?

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.h:120:
RemoteSuggestionsScheduler* scheduler,
On 2017/03/27 11:31:00, Marc Treib wrote:
> Can the scheduler be null? If so, please add a comment here. If not, remove
the
> ifs in the .cc :)

It cannot be null. Removed the ifs.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:561:
.WillByDefault(Return(true));
On 2017/03/27 11:31:00, Marc Treib wrote:
> Hm. Could this be an EXPECT_CALL with RetiresOnSaturation? Seems like that
would
> mess less with the tests' own expectations.

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:580:
NiceMock<MockScheduler> scheduler_;
On 2017/03/27 11:31:00, Marc Treib wrote:
> Hm hm. Okay for now I think, but could we add a TODO to make this a
StrictMock?
> Or do you have good reasons for keeping it Nice?

The only reason is not to plague every unit-test with
"EXPECT_CALL(*scheduler(), OnProviderActivated());"

I could add it into WaitForSuggestionsProviderInitialization(). Do you think it
is cleaner than a NiceMock?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:9: #include
"components/ntp_snippets/remote/remote_suggestions_provider.h"
On 2017/03/27 11:31:00, Marc Treib wrote:
> I think this can just be forward declared.

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:38: virtual void
OnInteractiveFetchFinished(Status fetch_status) = 0;
On 2017/03/27 11:31:00, Marc Treib wrote:
> #include for Status?

I forward-declared it.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:65:
RemoteSuggestionsProvider* provider_;
On 2017/03/27 11:31:00, Marc Treib wrote:
> Maybe move this out of the interface and into the impl, so that this stays an
> actual interface?
> If you keep it here, then you also have to add a ctor to initialize it. Also
I'd
> prefer to make it private and add an accessor.

Moved out.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:220: :
RemoteSuggestionsScheduler(),
On 2017/03/27 11:31:01, Marc Treib wrote:
> Not needed

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:417:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) {
On 2017/03/27 11:31:00, Marc Treib wrote:
> "RemoteSuggestionsSchedulerImpl::" not required

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:444:
std::move(callback)));
On 2017/03/27 11:31:00, Marc Treib wrote:
> nit: Inline the Bind() call here? Then the move isn't required.

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:448:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) {
On 2017/03/27 11:31:01, Marc Treib wrote:
> "RemoteSuggestionsSchedulerImpl::" not required

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.cc:474:
RemoteSuggestionsSchedulerImpl::TriggerType trigger) const {
On 2017/03/27 11:31:01, Marc Treib wrote:
> Also here

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:15: #include
"base/time/default_clock.h"
On 2017/03/27 11:31:01, Marc Treib wrote:
> s/default_clock/clock/

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:52: //
Scheduler interface. crbug.com/695447
On 2017/03/27 11:31:01, Marc Treib wrote:
> I think most of these comments are obsolete now.

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl.h:165:
std::set<RemoteSuggestionsSchedulerImpl::TriggerType> enabled_triggers_;
On 2017/03/27 11:31:01, Marc Treib wrote:
> "RemoteSuggestionsSchedulerImpl::" isn't needed

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:128:
scheduler_(nullptr),
On 2017/03/27 11:31:01, Marc Treib wrote:
> nit: unique_ptrs initialize themselves to nullptr

Done.

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
File
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_scheduler_impl_unittest.cc:207:
StrictMock<MockPersistentScheduler> persistent_scheduler_;
I reordered the members to reflect how they depend on each other.
(android tests were failing because local_state_ was destroyed before
scheduler_)

[Marc Treib, 2017-03-27 14:27:21]

lgtm

Thanks! LGTM with some optional nits/suggestions.

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 14:09:31, jkrcal wrote:
> On 2017/03/27 11:31:00, Marc Treib wrote:
> > On 2017/03/27 10:03:02, jkrcal wrote:
> > > On 2017/03/23 17:38:27, tschumann wrote:
> > > > this should never be nullptr, right? remove the if?
> > > > Didn't we think about making this call by wrapping the
fetching_callback?
> > > > (same below)
> > > > That way, we also had a single place to verify the scheduler is not
> nullptr
> > > and
> > > > don't have to worry about that state here.
> > > 
> > > Correct, removed the ifs here and below.
> > > 
> > > I am not sure about the wrappers. 
> > >  - We use wrapper callbacks for fetches initialized from the scheduler.
> > >  - It is the responsibility of the provider to ask for quota & inform
about
> > > status for interactive fetches. I am not sure it is cleaner to set it up
all
> > > here compared to the current situation. The two callbacks have different
> > > signatures, for one of them, I would need to introduce a helper
function...
> > > 
> > > Since Tim is OOO, Marc what it your opinion?
> > 
> > Not sure I understand the proposal... who would wrap the callback?
> 
> I am not sure either :D
> 
> The only solution I can imagine is that 
>  - ReloadSuggestions() actually provides a callback to FetchSuggestions() that
> does the work being done here;
>  - Fetch() wraps the callback provided to FetchSnippets() to do the work being
> done here.
> 
> These two callbacks have a different signature, so I see more ugliness than
> elegance in this whole idea.

Alright, then let's leave it as is for now, and let's see what Tim says once
he's back.

Seems to me this boils down to cleaning up the whole mess between
OnFetchFinished and OnFetchMoreFinished, which is a Good Thing and we should do
that, but it's orthogonal to this CL.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:153:
pref_service_.get(), base::MakeUnique<base::DefaultClock>());
Can we keep this, and remove the "can be null in tests"?

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:580:
NiceMock<MockScheduler> scheduler_;
On 2017/03/27 14:09:31, jkrcal wrote:
> On 2017/03/27 11:31:00, Marc Treib wrote:
> > Hm hm. Okay for now I think, but could we add a TODO to make this a
> StrictMock?
> > Or do you have good reasons for keeping it Nice?
> 
> The only reason is not to plague every unit-test with
> "EXPECT_CALL(*scheduler(), OnProviderActivated());"
> 
> I could add it into WaitForSuggestionsProviderInitialization(). Do you think
it
> is cleaner than a NiceMock?

Hm. Maybe? :)
I'm a bit worried that with a NiceMock, we're not properly testing the
integration with the scheduler. But putting scheduler expectations into every
test below clearly isn't the solution. Ideally, we'd have a separate suite of
tests for the scheduler integration, but with the current state of the provider,
that's probably not possible.

One idea: Make this a unique_ptr<MockScheduler>, and instantiate it as a
NiceMock by default. Then add some scheduler-specific tests where it's
instantiated as a StrictMock instead. Would that work?

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:96: user_classifier,  //
Can be nullptr in unittests.
Would "Can be null in tests." make it fit on the previous line? ;)

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right):

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:48: virtual void
SetProvider(RemoteSuggestionsProvider* provider) = 0;
This isn't considered part of the "internal triggers", right? If so, move up
above those?

[jkrcal, 2017-03-27 15:35:29]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org

[jkrcal, 2017-03-27 15:35:29]

Thanks, Marc! Will address your nits in a final patch list.

Bernhard, could you PTAL at browser_prefs.cc?
Éric, could you PTAL at browser_prefs.mm and the iOS factory?

[tschumann, 2017-03-27 15:40:45]

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 14:27:20, Marc Treib wrote:
> On 2017/03/27 14:09:31, jkrcal wrote:
> > On 2017/03/27 11:31:00, Marc Treib wrote:
> > > On 2017/03/27 10:03:02, jkrcal wrote:
> > > > On 2017/03/23 17:38:27, tschumann wrote:
> > > > > this should never be nullptr, right? remove the if?
> > > > > Didn't we think about making this call by wrapping the
> fetching_callback?
> > > > > (same below)
> > > > > That way, we also had a single place to verify the scheduler is not
> > nullptr
> > > > and
> > > > > don't have to worry about that state here.
> > > > 
> > > > Correct, removed the ifs here and below.
> > > > 
> > > > I am not sure about the wrappers. 
> > > >  - We use wrapper callbacks for fetches initialized from the scheduler.
> > > >  - It is the responsibility of the provider to ask for quota & inform
> about
> > > > status for interactive fetches. I am not sure it is cleaner to set it up
> all
> > > > here compared to the current situation. The two callbacks have different
> > > > signatures, for one of them, I would need to introduce a helper
> function...
> > > > 
> > > > Since Tim is OOO, Marc what it your opinion?
> > > 
> > > Not sure I understand the proposal... who would wrap the callback?
> > 
> > I am not sure either :D
> > 
> > The only solution I can imagine is that 
> >  - ReloadSuggestions() actually provides a callback to FetchSuggestions()
that
> > does the work being done here;
> >  - Fetch() wraps the callback provided to FetchSnippets() to do the work
being
> > done here.
> > 
> > These two callbacks have a different signature, so I see more ugliness than
> > elegance in this whole idea.
> 
> Alright, then let's leave it as is for now, and let's see what Tim says once
> he's back.
> 
> Seems to me this boils down to cleaning up the whole mess between
> OnFetchFinished and OnFetchMoreFinished, which is a Good Thing and we should
do
> that, but it's orthogonal to this CL.

OnFetchMoreFinished is already doing a lot of things, and this CL adds more
responsibilities to it, including accessing state which should always be present
in this particular context (the scheduler) -- but that's not easy to see.
Instead, my suggestion is that Fetch() makes sure the passed in
'fetching_callback' takes care of notifying the scheduler.
This might require an additional helper-function or just an inlined lambda.
However, anything we can do to keep complexity out of OnFetchMoreFinished is a
good thing.

I don't think that's orthogonal to this CL, as the question is where the extra
logic should be implemented.

[jkrcal, 2017-03-27 16:02:22]

Thanks Tim. 

Aren't you on vacation? ;)
If I do not hear anything from you, I interpret it as you feeling strongly (see
below).

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 15:40:45, tschumann wrote:
> On 2017/03/27 14:27:20, Marc Treib wrote:
> > On 2017/03/27 14:09:31, jkrcal wrote:
> > > On 2017/03/27 11:31:00, Marc Treib wrote:
> > > > On 2017/03/27 10:03:02, jkrcal wrote:
> > > > > On 2017/03/23 17:38:27, tschumann wrote:
> > > > > > this should never be nullptr, right? remove the if?
> > > > > > Didn't we think about making this call by wrapping the
> > fetching_callback?
> > > > > > (same below)
> > > > > > That way, we also had a single place to verify the scheduler is not
> > > nullptr
> > > > > and
> > > > > > don't have to worry about that state here.
> > > > > 
> > > > > Correct, removed the ifs here and below.
> > > > > 
> > > > > I am not sure about the wrappers. 
> > > > >  - We use wrapper callbacks for fetches initialized from the
scheduler.
> > > > >  - It is the responsibility of the provider to ask for quota & inform
> > about
> > > > > status for interactive fetches. I am not sure it is cleaner to set it
up
> > all
> > > > > here compared to the current situation. The two callbacks have
different
> > > > > signatures, for one of them, I would need to introduce a helper
> > function...
> > > > > 
> > > > > Since Tim is OOO, Marc what it your opinion?
> > > > 
> > > > Not sure I understand the proposal... who would wrap the callback?
> > > 
> > > I am not sure either :D
> > > 
> > > The only solution I can imagine is that 
> > >  - ReloadSuggestions() actually provides a callback to FetchSuggestions()
> that
> > > does the work being done here;
> > >  - Fetch() wraps the callback provided to FetchSnippets() to do the work
> being
> > > done here.
> > > 
> > > These two callbacks have a different signature, so I see more ugliness
than
> > > elegance in this whole idea.
> > 
> > Alright, then let's leave it as is for now, and let's see what Tim says once
> > he's back.
> > 
> > Seems to me this boils down to cleaning up the whole mess between
> > OnFetchFinished and OnFetchMoreFinished, which is a Good Thing and we should
> do
> > that, but it's orthogonal to this CL.
> 
> OnFetchMoreFinished is already doing a lot of things, and this CL adds more
> responsibilities to it, including accessing state which should always be
present
> in this particular context (the scheduler) -- but that's not easy to see.
> Instead, my suggestion is that Fetch() makes sure the passed in
> 'fetching_callback' takes care of notifying the scheduler.
> This might require an additional helper-function or just an inlined lambda.
> However, anything we can do to keep complexity out of OnFetchMoreFinished is a
> good thing.
> 
> I don't think that's orthogonal to this CL, as the question is where the extra
> logic should be implemented.

Tim, in the current state of the CL, the scheduler is always present as it is
passed in the constructor. Thus, there is not that much of extra logic.

Anyway, if you really feel strongly, I can implement your proposal.

[commit-bot: I haz the power, 2017-03-27 16:17:34]

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

[commit-bot: I haz the power, 2017-03-27 16:17:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[noyau (Ping after 24h), 2017-03-28 15:15:34]

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

[noyau (Ping after 24h), 2017-03-28 15:15:35]

ios lgtm

[Bernhard Bauer, 2017-03-28 15:22:42]

prefs/ LGTM

[jkrcal, 2017-03-29 10:54:05]

The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 10:54:20]

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

[jkrcal, 2017-03-29 10:55:58]

Marc, do you want to take a look before I submit it?
 - remote_suggestions_provider_impl.cc in patch set 7 addresses Tim's comment;
 - patch set 8 addresses your nits.

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl.cc (right):

https://codereview.chromium.org/2774663002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl.cc:656: if
(remote_suggestions_scheduler_) {
On 2017/03/27 16:02:22, jkrcal wrote:
> On 2017/03/27 15:40:45, tschumann wrote:
> > On 2017/03/27 14:27:20, Marc Treib wrote:
> > > On 2017/03/27 14:09:31, jkrcal wrote:
> > > > On 2017/03/27 11:31:00, Marc Treib wrote:
> > > > > On 2017/03/27 10:03:02, jkrcal wrote:
> > > > > > On 2017/03/23 17:38:27, tschumann wrote:
> > > > > > > this should never be nullptr, right? remove the if?
> > > > > > > Didn't we think about making this call by wrapping the
> > > fetching_callback?
> > > > > > > (same below)
> > > > > > > That way, we also had a single place to verify the scheduler is
not
> > > > nullptr
> > > > > > and
> > > > > > > don't have to worry about that state here.
> > > > > > 
> > > > > > Correct, removed the ifs here and below.
> > > > > > 
> > > > > > I am not sure about the wrappers. 
> > > > > >  - We use wrapper callbacks for fetches initialized from the
> scheduler.
> > > > > >  - It is the responsibility of the provider to ask for quota &
inform
> > > about
> > > > > > status for interactive fetches. I am not sure it is cleaner to set
it
> up
> > > all
> > > > > > here compared to the current situation. The two callbacks have
> different
> > > > > > signatures, for one of them, I would need to introduce a helper
> > > function...
> > > > > > 
> > > > > > Since Tim is OOO, Marc what it your opinion?
> > > > > 
> > > > > Not sure I understand the proposal... who would wrap the callback?
> > > > 
> > > > I am not sure either :D
> > > > 
> > > > The only solution I can imagine is that 
> > > >  - ReloadSuggestions() actually provides a callback to
FetchSuggestions()
> > that
> > > > does the work being done here;
> > > >  - Fetch() wraps the callback provided to FetchSnippets() to do the work
> > being
> > > > done here.
> > > > 
> > > > These two callbacks have a different signature, so I see more ugliness
> than
> > > > elegance in this whole idea.
> > > 
> > > Alright, then let's leave it as is for now, and let's see what Tim says
once
> > > he's back.
> > > 
> > > Seems to me this boils down to cleaning up the whole mess between
> > > OnFetchFinished and OnFetchMoreFinished, which is a Good Thing and we
should
> > do
> > > that, but it's orthogonal to this CL.
> > 
> > OnFetchMoreFinished is already doing a lot of things, and this CL adds more
> > responsibilities to it, including accessing state which should always be
> present
> > in this particular context (the scheduler) -- but that's not easy to see.
> > Instead, my suggestion is that Fetch() makes sure the passed in
> > 'fetching_callback' takes care of notifying the scheduler.
> > This might require an additional helper-function or just an inlined lambda.
> > However, anything we can do to keep complexity out of OnFetchMoreFinished is
a
> > good thing.
> > 
> > I don't think that's orthogonal to this CL, as the question is where the
extra
> > logic should be implemented.
> 
> Tim, in the current state of the CL, the scheduler is always present as it is
> passed in the constructor. Thus, there is not that much of extra logic.
> 
> Anyway, if you really feel strongly, I can implement your proposal.

Okay, done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service_unittest.cc (right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service_unittest.cc:153:
pref_service_.get(), base::MakeUnique<base::DefaultClock>());
On 2017/03/27 14:27:20, Marc Treib wrote:
> Can we keep this, and remove the "can be null in tests"?

Done.

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/80001/components/ntp_snippets...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:580:
NiceMock<MockScheduler> scheduler_;
On 2017/03/27 14:27:20, Marc Treib wrote:
> On 2017/03/27 14:09:31, jkrcal wrote:
> > On 2017/03/27 11:31:00, Marc Treib wrote:
> > > Hm hm. Okay for now I think, but could we add a TODO to make this a
> > StrictMock?
> > > Or do you have good reasons for keeping it Nice?
> > 
> > The only reason is not to plague every unit-test with
> > "EXPECT_CALL(*scheduler(), OnProviderActivated());"
> > 
> > I could add it into WaitForSuggestionsProviderInitialization(). Do you think
> it
> > is cleaner than a NiceMock?
> 
> Hm. Maybe? :)
> I'm a bit worried that with a NiceMock, we're not properly testing the
> integration with the scheduler. But putting scheduler expectations into every
> test below clearly isn't the solution. Ideally, we'd have a separate suite of
> tests for the scheduler integration, but with the current state of the
provider,
> that's probably not possible.
> 
> One idea: Make this a unique_ptr<MockScheduler>, and instantiate it as a
> NiceMock by default. Then add some scheduler-specific tests where it's
> instantiated as a StrictMock instead. Would that work?

Done.

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
components/ntp_snippets/content_suggestions_service.h:96: user_classifier,  //
Can be nullptr in unittests.
On 2017/03/27 14:27:21, Marc Treib wrote:
> Would "Can be null in tests." make it fit on the previous line? ;)

Removed, anyway.

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_scheduler.h (right):

https://codereview.chromium.org/2774663002/diff/100001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_scheduler.h:48: virtual void
SetProvider(RemoteSuggestionsProvider* provider) = 0;
On 2017/03/27 14:27:21, Marc Treib wrote:
> This isn't considered part of the "internal triggers", right? If so, move up
> above those?

Done.

[Marc Treib, 2017-03-29 11:11:57]

Thanks! Still lgtm with one more comment in the test.

https://codereview.chromium.org/2774663002/diff/140001/components/ntp_snippet...
File components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc
(right):

https://codereview.chromium.org/2774663002/diff/140001/components/ntp_snippet...
components/ntp_snippets/remote/remote_suggestions_provider_impl_unittest.cc:499:
void SetStrictScheduler() {
Hm, by itself this doesn't really make sense, as the provider will still have
the old scheduler - in fact, a dangling pointer to it. Can we wrap this in a
MakeSuggestionsProviderWithStrictScheduler() or something like that?

Or alternatively, somehow check that this is not called after the previous
scheduler_ has been passed around?

[tschumann, 2017-03-29 12:22:58]

lgtm

thank you!

[jkrcal, 2017-03-29 12:58:24]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-03-29 12:58:25]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
noyau@chromium.org, treib@chromium.org, tschumann@chromium.org
Link to the patchset: https://codereview.chromium.org/2774663002/#ps160001
(title: "Marc's nits #2")

[commit-bot: I haz the power, 2017-03-29 12:58:43]

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, 2017-03-29 15:21:05]

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

[commit-bot: I haz the power, 2017-03-29 15:21:06]

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

[jkrcal, 2017-03-29 15:23:46]

The CQ bit was checked by jkrcal@chromium.org

[commit-bot: I haz the power, 2017-03-29 15:24:45]

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, 2017-03-29 16:25:26]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490801026279290,
"parent_rev": "7a7a48cfbf8854d71104c5830403b1dab6830807", "commit_rev":
"f996646c9b3b4c8027e693c5a48c7fab3adbfe8c"}

[commit-bot: I haz the power, 2017-03-29 16:26:16]

Description was changed from

==========
[Remote suggestions] Refactor the scheduler

This CL cleans up the relation of the scheduler and the provider of
remote suggestions. The previous wrapper pattern was not useful as there
was too much two-way communication between these two.

BUG=695447
==========

to

==========
[Remote suggestions] Refactor the scheduler

This CL cleans up the relation of the scheduler and the provider of
remote suggestions. The previous wrapper pattern was not useful as there
was too much two-way communication between these two.

BUG=695447

Review-Url: https://codereview.chromium.org/2774663002
Cr-Commit-Position: refs/heads/master@{#460414}
Committed:
https://chromium.googlesource.com/chromium/src/+/f996646c9b3b4c8027e693c5a48c...
==========

[commit-bot: I haz the power, 2017-03-29 16:26:18]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/f996646c9b3b4c8027e693c5a48c...