[NTP Snippets] Refactor background scheduling for remote suggestions

components/ntp_snippets/content_suggestions_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/content_suggestions_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <set>
 #include <utility>
@@ skipping 11 matching lines @@
 namespace ntp_snippets {
 
 ContentSuggestionsService::ContentSuggestionsService(
     State state,
     SigninManagerBase* signin_manager,
     history::HistoryService* history_service,
     PrefService* pref_service)
     : 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.
   if (signin_manager) {
     signin_observer_.Add(signin_manager);
   }
 
   if (history_service) {
     history_service_observer_.Add(history_service);
   }
 
   RestoreDismissedCategoriesFromPrefs();
 }
 
 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();
   providers_.clear();
   state_ = State::DISABLED;
   for (Observer& observer : observers_) {
     observer.ContentSuggestionsServiceShutdown();
   }
 }
 
@@ skipping 155 matching lines @@
     const std::set<std::string>& known_suggestion_ids,
     const FetchDoneCallback& callback) {
   auto providers_it = providers_by_category_.find(category);
   if (providers_it == providers_by_category_.end()) {
     return;
   }
 
   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
 
 void ContentSuggestionsService::OnNewSuggestions(
     ContentSuggestionsProvider* provider,
     Category category,
     std::vector<ContentSuggestion> suggestions) {
   // Providers shouldn't call this when they're in a non-available state.
   DCHECK(
       IsCategoryStatusInitOrAvailable(provider->GetCategoryStatus(category)));
@@ skipping 258 matching lines @@
 void ContentSuggestionsService::StoreDismissedCategoriesToPrefs() {
   base::ListValue list;
   for (const auto& category_provider_pair : dismissed_providers_by_category_) {
     list.AppendInteger(category_provider_pair.first.id());
   }
 
   pref_service_->Set(prefs::kDismissedCategories, list);
 }
 
 }  // namespace ntp_snippets