Combine all suggestions factories into ContentSuggestionsServiceFactory

components/ntp_snippets/ntp_snippets_service.cc

Index: components/ntp_snippets/ntp_snippets_service.cc
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index def66cb058631176787cd94ea42163b3495cbdee..177c84f9f9325bb78913272737f5327409983270 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -185,10 +185,10 @@ void Compact(NTPSnippet::PtrVector* snippets) {
 }  // namespace
 
 NTPSnippetsService::NTPSnippetsService(
-    bool enabled,
+    Observer* observer,
+    CategoryFactory* category_factory,
     PrefService* pref_service,
     SuggestionsService* suggestions_service,
-    CategoryFactory* category_factory,
     const std::string& application_language_code,
     NTPSnippetsScheduler* scheduler,
     std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher,
@@ -196,13 +196,12 @@ NTPSnippetsService::NTPSnippetsService(
     std::unique_ptr<ImageDecoder> image_decoder,
     std::unique_ptr<NTPSnippetsDatabase> database,
     std::unique_ptr<NTPSnippetsStatusService> status_service)
-    : ContentSuggestionsProvider(category_factory),
+    : ContentSuggestionsProvider(observer, category_factory),
       state_(State::NOT_INITED),
       category_status_(CategoryStatus::INITIALIZING),
       pref_service_(pref_service),
       suggestions_service_(suggestions_service),
       application_language_code_(application_language_code),
-      observer_(nullptr),
       scheduler_(scheduler),
       snippets_fetcher_(std::move(snippets_fetcher)),
       image_fetcher_(std::move(image_fetcher)),
@@ -213,20 +212,14 @@ NTPSnippetsService::NTPSnippetsService(
       provided_category_(
           category_factory->FromKnownCategory(KnownCategories::ARTICLES)) {
   // In some cases, don't even bother loading the database.

[Marc Treib, 2016/08/03 11:53:23]

Not really appropriate anymore

[Philipp Keck, 2016/08/03 12:28:24]

Done.

-  if (!enabled) {
-    EnterState(State::SHUT_DOWN, CategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
-    return;
-  }
   if (database_->IsErrorState()) {
-    EnterState(State::SHUT_DOWN, CategoryStatus::LOADING_ERROR);
+    EnterState(State::ERROR, CategoryStatus::LOADING_ERROR);
     return;
   }
 
   database_->SetErrorCallback(base::Bind(&NTPSnippetsService::OnDatabaseError,
                                          base::Unretained(this)));
 
-  // TODO(pke): Move this to SetObserver as soon as the UI reads from the
-  // ContentSuggestionsService directly.
   // We transition to other states while finalizing the initialization, when the
   // database is done loading.
   database_->LoadSnippets(base::Bind(&NTPSnippetsService::OnDatabaseLoaded,
@@ -234,7 +227,8 @@ NTPSnippetsService::NTPSnippetsService(
 }
 
 NTPSnippetsService::~NTPSnippetsService() {
-  DCHECK(state_ == State::SHUT_DOWN);
+  expiry_timer_.Stop();

[Marc Treib, 2016/08/03 11:53:23]

I think this isn't necessary, the timer will be destroyed anyway.

[Philipp Keck, 2016/08/03 12:28:24]

Indeed. Done.

+  RescheduleFetching();

[Marc Treib, 2016/08/03 11:53:23]

Why is this here? Note that we do NOT want to un-schedule just because the
service is being destroyed.

[Philipp Keck, 2016/08/03 12:28:24]

Both of these lines are here because they previously were in
EnterStateShutdown().
So previously, when the KeyedService (which is now the
ContentSuggestionsService) got the Shutdown() signal, it would reschedule the
fetching. If I remove this from here, it probably changes the observable
behavior of the application, right? It might still be the right thing to do.

[Marc Treib, 2016/08/03 12:37:56]

Huh, indeed. That line was added by dgn in
https://codereview.chromium.org/2061803002, and I'm guessing he just wrongly
assumed that we should unschedule on shutdown. Probably background fetching is
completely broken right now. :-/
Anyway, probably best not to change it in this CL. So leave it there for now,
but please add a TODO to verify it really should be there, because I strongly
suspect it shouldn't.

[Philipp Keck, 2016/08/03 12:57:23]

Ok. I just noticed that I did change the observable behavior, because
RescheduleFetching now doesn't unschedule anymore: there is no more SHUT_DOWN
state. So to achieve the same behavior, we would have to use "Unschedule()"
here.

Let's discuss this offline and then implement the right solution right away.

[Marc Treib, 2016/08/03 13:01:18]

New plan: I'll land a fix for this first, then maybe that will still merge
cleanly to M53.

 }
 
 // static
@@ -244,11 +238,6 @@ void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   NTPSnippetsStatusService::RegisterProfilePrefs(registry);
 }
 
-// Inherited from KeyedService.
-void NTPSnippetsService::Shutdown() {
-  EnterState(State::SHUT_DOWN, CategoryStatus::NOT_PROVIDED);
-}
-
 void NTPSnippetsService::FetchSnippets(bool force_request) {
   if (ready())
     FetchSnippetsFromHosts(GetSuggestionsHosts(), force_request);
@@ -355,23 +344,11 @@ void NTPSnippetsService::ClearDismissedSuggestionsForDebugging() {
   dismissed_snippets_.clear();
 }
 
-void NTPSnippetsService::SetObserver(Observer* observer) {
-  observer_ = observer;
-}
-
 CategoryStatus NTPSnippetsService::GetCategoryStatus(Category category) {
   DCHECK(category.IsKnownCategory(KnownCategories::ARTICLES));
   return category_status_;
 }
 
-void NTPSnippetsService::AddObserver(NTPSnippetsServiceObserver* observer) {
-  observers_.AddObserver(observer);
-}
-
-void NTPSnippetsService::RemoveObserver(NTPSnippetsServiceObserver* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 // static
 int NTPSnippetsService::GetMaxSnippetCountForTesting() {
   return kMaxSnippetCount;
@@ -399,10 +376,9 @@ void NTPSnippetsService::OnImageDataFetched(const std::string& snippet_id,
 }
 
 void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
-  DCHECK(state_ == State::NOT_INITED || state_ == State::SHUT_DOWN);
-  if (state_ == State::SHUT_DOWN)
+  if (state_ == State::ERROR)
     return;
-
+  DCHECK(state_ == State::NOT_INITED);
   DCHECK(snippets_.empty());
   DCHECK(dismissed_snippets_.empty());
   for (std::unique_ptr<NTPSnippet>& snippet : snippets) {
@@ -422,7 +398,7 @@ void NTPSnippetsService::OnDatabaseLoaded(NTPSnippet::PtrVector snippets) {
 }
 
 void NTPSnippetsService::OnDatabaseError() {
-  EnterState(State::SHUT_DOWN, CategoryStatus::LOADING_ERROR);
+  EnterState(State::ERROR, CategoryStatus::LOADING_ERROR);
 }
 
 // TODO(dgn): name clash between content suggestions and suggestions hosts.
@@ -699,15 +675,9 @@ void NTPSnippetsService::EnterStateDisabled() {
   RescheduleFetching();
 }
 
-void NTPSnippetsService::EnterStateShutdown() {
-  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                    NTPSnippetsServiceShutdown());
-
+void NTPSnippetsService::EnterStateError() {
   expiry_timer_.Stop();
-  suggestions_service_subscription_.reset();
   RescheduleFetching();
-
-  snippets_status_service_.reset();

[Marc Treib, 2016/08/03 11:53:23]

Why did you remove these two lines?

[Philipp Keck, 2016/08/03 12:28:24]

That was unintentional, because I had removed the SHUT_DOWN state entirely,
along with this entire method, and moved stuff to the destructor, where the
resets are not necessary. Then I introduced the ERROR state and took the code
from the destructor.

Now they're back.

 }
 
 void NTPSnippetsService::FinishInitialization() {
@@ -733,9 +703,6 @@ void NTPSnippetsService::FinishInitialization() {
 
 void NTPSnippetsService::OnDisabledReasonChanged(
     DisabledReason disabled_reason) {
-  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                    NTPSnippetsServiceDisabledReasonChanged(disabled_reason));
-
   switch (disabled_reason) {
     case DisabledReason::NONE:
       // Do not change the status. That will be done in EnterStateEnabled()
@@ -783,22 +750,15 @@ void NTPSnippetsService::EnterState(State state, CategoryStatus status) {
       EnterStateDisabled();
       return;
 
-    case State::SHUT_DOWN:
-      DVLOG(1) << "Entering state: SHUT_DOWN";
-      state_ = State::SHUT_DOWN;
-      EnterStateShutdown();
+    case State::ERROR:
+      DVLOG(1) << "Entering state: ERROR";
+      state_ = State::ERROR;
+      EnterStateError();
       return;
   }
 }
 
 void NTPSnippetsService::NotifyNewSuggestions() {
-  // TODO(pke): Remove this as soon as this becomes a pure provider.
-  FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
-                    NTPSnippetsServiceLoaded());
-
-  if (!observer_)
-    return;
-
   std::vector<ContentSuggestion> result;
   for (const std::unique_ptr<NTPSnippet>& snippet : snippets_) {
     if (!snippet->is_complete())
@@ -815,7 +775,7 @@ void NTPSnippetsService::NotifyNewSuggestions() {
     suggestion.set_score(snippet->score());
     result.emplace_back(std::move(suggestion));
   }
-  observer_->OnNewSuggestions(this, provided_category_, std::move(result));
+  observer()->OnNewSuggestions(this, provided_category_, std::move(result));
 }
 
 void NTPSnippetsService::UpdateCategoryStatus(CategoryStatus status) {
@@ -823,10 +783,8 @@ void NTPSnippetsService::UpdateCategoryStatus(CategoryStatus status) {
     return;
 
   category_status_ = status;
-  if (observer_) {
-    observer_->OnCategoryStatusChanged(this, provided_category_,
-                                       category_status_);
-  }
+  observer()->OnCategoryStatusChanged(this, provided_category_,
+                                      category_status_);
 }
 
 }  // namespace ntp_snippets