Combine all suggestions factories into ContentSuggestionsServiceFactory

components/ntp_snippets/ntp_snippets_service.cc

 // Copyright 2015 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/ntp_snippets_service.h"
 
 #include <algorithm>
 #include <iterator>
 #include <utility>
 
@@ skipping 167 matching lines @@
   snippets->erase(
       std::remove_if(
           snippets->begin(), snippets->end(),
           [](const std::unique_ptr<NTPSnippet>& snippet) { return !snippet; }),
       snippets->end());
 }
 
 }  // 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,
     std::unique_ptr<ImageFetcher> image_fetcher,
     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)),
       image_decoder_(std::move(image_decoder)),
       database_(std::move(database)),
       snippets_status_service_(std::move(status_service)),
       fetch_after_load_(false),
       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,
                                      base::Unretained(this)));
 }
 
 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
 void NTPSnippetsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
   registry->RegisterListPref(prefs::kSnippetHosts);
 
   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);
   else
     fetch_after_load_ = true;
 }
 
 void NTPSnippetsService::FetchSnippetsFromHosts(
     const std::set<std::string>& hosts,
     bool force_request) {
@@ skipping 86 matching lines @@
   if (!initialized())
     return;
 
   if (dismissed_snippets_.empty())
     return;
 
   database_->DeleteSnippets(dismissed_snippets_);
   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;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 // image_fetcher::ImageFetcherDelegate implementation.
 void NTPSnippetsService::OnImageDataFetched(const std::string& snippet_id,
                                             const std::string& image_data) {
   if (image_data.empty())
     return;
 
   // Only save the image if the corresponding snippet still exists.
   auto it =
       std::find_if(snippets_.begin(), snippets_.end(),
                    [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
                      return snippet->id() == snippet_id;
                    });
   if (it == snippets_.end())
     return;
 
   database_->SaveImage(snippet_id, image_data);
 }
 
 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) {
     if (snippet->is_dismissed())
       dismissed_snippets_.emplace_back(std::move(snippet));
     else
       snippets_.emplace_back(std::move(snippet));
   }
   std::sort(snippets_.begin(), snippets_.end(),
             [](const std::unique_ptr<NTPSnippet>& lhs,
                const std::unique_ptr<NTPSnippet>& rhs) {
               return lhs->score() > rhs->score();
             });
 
   ClearExpiredSnippets();
   FinishInitialization();
 }
 
 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.
 // method name should be changed.
 void NTPSnippetsService::OnSuggestionsChanged(
     const SuggestionsProfile& suggestions) {
   DCHECK(initialized());
 
   std::set<std::string> hosts = GetSuggestionsHostsImpl(suggestions);
   if (hosts == GetSnippetHostsFromPrefs())
@@ skipping 256 matching lines @@
 
 void NTPSnippetsService::EnterStateDisabled() {
   ClearCachedSuggestionsForDebugging();
   ClearDismissedSuggestionsForDebugging();
 
   expiry_timer_.Stop();
   suggestions_service_subscription_.reset();
   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() {
   snippets_fetcher_->SetCallback(
       base::Bind(&NTPSnippetsService::OnFetchFinished, base::Unretained(this)));
 
   // |image_fetcher_| can be null in tests.
   if (image_fetcher_) {
     image_fetcher_->SetImageFetcherDelegate(this);
     image_fetcher_->SetDataUseServiceName(
         data_use_measurement::DataUseUserData::NTP_SNIPPETS);
   }
 
   // Note: Initializing the status service will run the callback right away with
   // the current state.
   snippets_status_service_->Init(base::Bind(
       &NTPSnippetsService::OnDisabledReasonChanged, base::Unretained(this)));
 
   // Always notify here even if we got nothing from the database, because we
   // don't know how long the fetch will take or if it will even complete.
   NotifyNewSuggestions();
 }
 
 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()
       EnterState(State::READY, category_status_);
       break;
 
     case DisabledReason::EXPLICITLY_DISABLED:
       EnterState(State::DISABLED, CategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
       break;
 
@@ skipping 27 matching lines @@
     }
 
     case State::DISABLED:
       DCHECK(state_ == State::NOT_INITED || state_ == State::READY);
 
       DVLOG(1) << "Entering state: DISABLED";
       state_ = State::DISABLED;
       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())
       continue;
     ContentSuggestion suggestion(
         MakeUniqueID(provided_category_, snippet->id()),
         snippet->best_source().url);
     suggestion.set_amp_url(snippet->best_source().amp_url);
     suggestion.set_title(base::UTF8ToUTF16(snippet->title()));
     suggestion.set_snippet_text(base::UTF8ToUTF16(snippet->snippet()));
     suggestion.set_publish_date(snippet->publish_date());
     suggestion.set_publisher_name(
         base::UTF8ToUTF16(snippet->best_source().publisher_name));
     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) {
   if (status == category_status_)
     return;
 
   category_status_ = status;
-  if (observer_) {
-    observer_->OnCategoryStatusChanged(this, provided_category_,
-                                       category_status_);
-  }
+  observer()->OnCategoryStatusChanged(this, provided_category_,
+                                      category_status_);
 }
 
 }  // namespace ntp_snippets