[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index 5cd19fd2c70fd03eee9b33759ee80ec3a1c7c29f..b6f64376e6cd95a87c3f59d532af48559964b254 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -29,7 +29,8 @@ ContentSuggestionsService::ContentSuggestionsService(
     : state_(state),
       signin_observer_(this),
       history_service_observer_(this),
-      ntp_snippets_service_(nullptr),
+      remote_suggestions_provider_(nullptr),
+      remote_suggestions_scheduler_(nullptr),

[tschumann, 2016/12/19 11:07:19]

why do we need both?  Shouldn't the SchedulingRemoteSuggestionsProvider be
sufficient?

[Marc Treib, 2016/12/19 12:59:23]

But then that'd have to expose both members. And since they're exposed for
different reasons (the provider only for the internals page, the scheduler only
because of plumbing reasons, to make it accessible from the scheduled wake-ups),
I kinda like them being separate. Not feeling very strongly though.

[tschumann, 2016/12/19 14:17:08]

hehe... so my question is more: why do we still need to expose the wrapped
remote-suggestions-provider. Nothing we need to fix now, but it would be good to
understand and clean-up later.

[jkrcal, 2016/12/20 16:39:46]

Thanks, agreed :)

[jkrcal, 2016/12/20 16:39:46]

These are different interfaces, one is used for debugging and testing, the other
from the bridge. I would like to at least keep the getters separate. We can
still unify the setters and the pointers. However, I am not convinced if the
service should necessarily know who is implementing what.

(At the moment, SchedulingRemoteSuggestionsProvider still does not implement
RemoteSuggestionsProvider. This should be fixed anyway, maybe in another CL?)

[jkrcal, 2016/12/20 16:39:46]

As I say above, the only reason for the moment is that the scheduling wrapper
does not implement RemoteSuggestionsProvider). I think we can just expose the
scheduling wrapper, using two getters. I am not sure whether to have just one
setter or two setters (so that content_suggestions_service does not need to know
about implementation details).

[tschumann, 2016/12/21 08:21:18]

Oh yeah, totally. The scheduling remote suggestions provider should under no
circumstances expose it's underlying provider -- that's the whole idea of the
abstraction. 
I was just wondering which pieces of the remotesuggestionsprovider are still
needed by the ContentSuggestionsService -- in an ideal world there would be no
need to make that provider anything special.

Thanks for clarifying!

[jkrcal, 2016/12/21 08:40:49]

Done, in the end. Scheduling... now implements RemoteSuggestionsProvider and
only this guy is exposed. Via two setters and getters.

       pref_service_(pref_service),
       user_classifier_(pref_service) {
   // Can be null in tests.
@@ -47,7 +48,8 @@ ContentSuggestionsService::ContentSuggestionsService(
 ContentSuggestionsService::~ContentSuggestionsService() = default;
 
 void ContentSuggestionsService::Shutdown() {
-  ntp_snippets_service_ = nullptr;
+  remote_suggestions_provider_ = nullptr;
+  remote_suggestions_scheduler_ = nullptr;
   suggestions_by_category_.clear();
   providers_by_category_.clear();
   categories_.clear();
@@ -223,6 +225,12 @@ void ContentSuggestionsService::Fetch(
   providers_it->second->Fetch(category, known_suggestion_ids, callback);
 }
 
+void ContentSuggestionsService::ReloadSuggestions() {
+  for (const auto& category_provider_pair : providers_by_category_) {
+    category_provider_pair.second->ReloadSuggestions();

[Marc Treib, 2016/12/19 12:59:23]

If a provider provides multiple categories, then it'll get called multiple times
here.
I think this should just iterate over providers_ (and maybe skip those that
haven't registered any categories?)

[jkrcal, 2016/12/20 16:39:46]

Indeed, thanks!

I do not see how to implement skipping those without any categories without
introducing another data or without heavy iteration. I do not see the added
value worth the effort.

[tschumann, 2016/12/21 08:21:18]

+1 for such providers that would be a no-op anyways, right?

[jkrcal, 2016/12/21 08:40:49]

Acknowledged.

+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods