Change NTPSnippetsService to implement ContentSuggestionsProvider
The NTPSnippetsService currently delivers snippets to the UI via the
Android bridge. Change it to additionally serve as a
ContentSuggestionsProvider implementation and deliver
ContentSuggestions for the ARTICLES category. These ContentSuggestions
are currently only delivered to the ContentSuggestionsService and not
served to the UI, yet.
Adjust NTPSnippetsServiceTest, SnippetsBridge and SnippetsInternals
because of a few renamed functions.
Change NTPSnippetsServiceFactory to register the NTPSnippetsService
as a provder with the ContentSuggestionsService.
BUG=619560
Review-Url: https://codereview.chromium.org/2131943002
Cr-Commit-Position: refs/heads/master@{#405103}
diff --git a/chrome/browser/android/ntp/ntp_snippets_bridge.cc b/chrome/browser/android/ntp/ntp_snippets_bridge.cc
index e6e9cdd..373de14c8 100644
--- a/chrome/browser/android/ntp/ntp_snippets_bridge.cc
+++ b/chrome/browser/android/ntp/ntp_snippets_bridge.cc
@@ -93,7 +93,7 @@
const JavaParamRef<jstring>& snippet_id,
const JavaParamRef<jobject>& j_callback) {
base::android::ScopedJavaGlobalRef<jobject> callback(j_callback);
- ntp_snippets_service_->FetchSnippetImage(
+ ntp_snippets_service_->FetchSuggestionImage(
ConvertJavaStringToUTF8(env, snippet_id),
base::Bind(&NTPSnippetsBridge::OnImageFetched,
weak_ptr_factory_.GetWeakPtr(), callback));
@@ -102,7 +102,7 @@
void NTPSnippetsBridge::DiscardSnippet(JNIEnv* env,
const JavaParamRef<jobject>& obj,
const JavaParamRef<jstring>& id) {
- ntp_snippets_service_->DiscardSnippet(ConvertJavaStringToUTF8(env, id));
+ ntp_snippets_service_->DiscardSuggestion(ConvertJavaStringToUTF8(env, id));
}
void NTPSnippetsBridge::SnippetVisited(JNIEnv* env,
diff --git a/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc b/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc
index ab7bfc0..4e3a6865 100644
--- a/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc
+++ b/chrome/browser/ntp_snippets/ntp_snippets_service_factory.cc
@@ -9,6 +9,7 @@
#include "base/memory/ptr_util.h"
#include "base/memory/singleton.h"
#include "chrome/browser/browser_process.h"
+#include "chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/search/suggestions/image_decoder_impl.h"
#include "chrome/browser/search/suggestions/suggestions_service_factory.h"
@@ -22,6 +23,7 @@
#include "components/image_fetcher/image_fetcher.h"
#include "components/image_fetcher/image_fetcher_impl.h"
#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/ntp_snippets/content_suggestions_service.h"
#include "components/ntp_snippets/ntp_snippets_constants.h"
#include "components/ntp_snippets/ntp_snippets_database.h"
#include "components/ntp_snippets/ntp_snippets_fetcher.h"
@@ -48,6 +50,7 @@
using suggestions::ImageDecoderImpl;
using suggestions::SuggestionsService;
using suggestions::SuggestionsServiceFactory;
+using ntp_snippets::ContentSuggestionsService;
// static
NTPSnippetsServiceFactory* NTPSnippetsServiceFactory::GetInstance() {
@@ -70,6 +73,7 @@
DependsOn(ProfileSyncServiceFactory::GetInstance());
DependsOn(SigninManagerFactory::GetInstance());
DependsOn(SuggestionsServiceFactory::GetInstance());
+ DependsOn(ContentSuggestionsServiceFactory::GetInstance());
}
NTPSnippetsServiceFactory::~NTPSnippetsServiceFactory() {}
@@ -78,6 +82,13 @@
content::BrowserContext* context) const {
Profile* profile = Profile::FromBrowserContext(context);
+ ContentSuggestionsService* content_suggestions_service =
+ ContentSuggestionsServiceFactory::GetForProfile(profile);
+ // TODO(pke): When the AndroidBridge does not access the NTPSnippetsService
+ // directly anymore (for retrieving content), the NTPSnippetsService does not
+ // need to be created if content_suggestions_service->state() == DISABLED,
+ // just return nullptr then and remove the other "if" below.
+
// TODO(mvanouwerkerk): Move the enable logic into the service once we start
// observing pref changes.
bool enabled = false;
@@ -111,18 +122,25 @@
base::SequencedWorkerPool::GetSequenceToken(),
base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
- return new ntp_snippets::NTPSnippetsService(
- enabled, profile->GetPrefs(), suggestions_service,
- g_browser_process->GetApplicationLocale(), scheduler,
- base::MakeUnique<ntp_snippets::NTPSnippetsFetcher>(
- signin_manager, token_service, request_context,
- base::Bind(&safe_json::SafeJsonParser::Parse),
- chrome::GetChannel() == version_info::Channel::STABLE),
- base::MakeUnique<ImageFetcherImpl>(
- base::MakeUnique<ImageDecoderImpl>(), request_context.get()),
- base::MakeUnique<ImageDecoderImpl>(),
- base::MakeUnique<ntp_snippets::NTPSnippetsDatabase>(database_dir,
- task_runner),
- base::MakeUnique<ntp_snippets::NTPSnippetsStatusService>(signin_manager,
- sync_service));
+ ntp_snippets::NTPSnippetsService* ntp_snippets_service =
+ new ntp_snippets::NTPSnippetsService(
+ enabled, profile->GetPrefs(), suggestions_service,
+ g_browser_process->GetApplicationLocale(), scheduler,
+ base::MakeUnique<ntp_snippets::NTPSnippetsFetcher>(
+ signin_manager, token_service, request_context,
+ base::Bind(&safe_json::SafeJsonParser::Parse),
+ chrome::GetChannel() == version_info::Channel::STABLE),
+ base::MakeUnique<ImageFetcherImpl>(
+ base::MakeUnique<ImageDecoderImpl>(), request_context.get()),
+ base::MakeUnique<ImageDecoderImpl>(),
+ base::MakeUnique<ntp_snippets::NTPSnippetsDatabase>(database_dir,
+ task_runner),
+ base::MakeUnique<ntp_snippets::NTPSnippetsStatusService>(
+ signin_manager, sync_service));
+
+ if (content_suggestions_service->state() ==
+ ContentSuggestionsService::State::ENABLED) {
+ content_suggestions_service->RegisterProvider(ntp_snippets_service);
+ }
+ return ntp_snippets_service;
}
diff --git a/chrome/browser/ui/webui/snippets_internals_message_handler.cc b/chrome/browser/ui/webui/snippets_internals_message_handler.cc
index 89a20ea..f6e7411b 100644
--- a/chrome/browser/ui/webui/snippets_internals_message_handler.cc
+++ b/chrome/browser/ui/webui/snippets_internals_message_handler.cc
@@ -114,14 +114,14 @@
void SnippetsInternalsMessageHandler::HandleClear(const base::ListValue* args) {
DCHECK_EQ(0u, args->GetSize());
- ntp_snippets_service_->ClearSnippets();
+ ntp_snippets_service_->ClearCachedSuggestionsForDebugging();
}
void SnippetsInternalsMessageHandler::HandleClearDiscarded(
const base::ListValue* args) {
DCHECK_EQ(0u, args->GetSize());
- ntp_snippets_service_->ClearDiscardedSnippets();
+ ntp_snippets_service_->ClearDiscardedSuggestionsForDebugging();
SendDiscardedSnippets();
}
diff --git a/components/ntp_snippets/content_suggestions_category_status.h b/components/ntp_snippets/content_suggestions_category_status.h
index d3b5ba4..16b65af0 100644
--- a/components/ntp_snippets/content_suggestions_category_status.h
+++ b/components/ntp_snippets/content_suggestions_category_status.h
@@ -11,6 +11,10 @@
// On Android builds, a Java counterpart will be generated for this enum.
// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets
enum class ContentSuggestionsCategoryStatus {
+ // The provider is still initializing and it is not yet determined whether
+ // content suggestions will be available or not.
+ INITIALIZING,
+
// Content suggestions are available (though the list of available suggestions
// may be empty simply because there are no reasonable suggestions to be made
// at the moment).
@@ -36,9 +40,10 @@
PASSPHRASE_ENCRYPTION_ENABLED,
// Content suggestions are not available because history sync is disabled.
HISTORY_SYNC_DISABLED,
- // Content suggestions are not available because the history sync service is
- // not yet completely initialized and its status is unknown.
- HISTORY_SYNC_STATE_UNKNOWN
+
+ // Content suggestions are not available because an error occured when loading
+ // or updating them.
+ LOADING_ERROR
};
// Determines whether the given status is one of the AVAILABLE statuses.
diff --git a/components/ntp_snippets/content_suggestions_provider.cc b/components/ntp_snippets/content_suggestions_provider.cc
index 696a3813..e4565e5 100644
--- a/components/ntp_snippets/content_suggestions_provider.cc
+++ b/components/ntp_snippets/content_suggestions_provider.cc
@@ -23,7 +23,8 @@
std::string ContentSuggestionsProvider::MakeUniqueID(
ContentSuggestionsCategory category,
const std::string& within_category_id) {
- return base::StringPrintf(kCombinedIDFormat, int(category),
+ return base::StringPrintf(kCombinedIDFormat,
+ static_cast<int>(category),
within_category_id.c_str());
}
diff --git a/components/ntp_snippets/content_suggestions_provider.h b/components/ntp_snippets/content_suggestions_provider.h
index 93872f58..8d0b5dd 100644
--- a/components/ntp_snippets/content_suggestions_provider.h
+++ b/components/ntp_snippets/content_suggestions_provider.h
@@ -62,7 +62,8 @@
};
// Sets an observer which is notified about changes to the available
- // suggestions, or removes it by passing a nullptr.
+ // suggestions, or removes it by passing a nullptr. The provider does not take
+ // ownership of the observer and the observer must outlive this provider.
virtual void SetObserver(Observer* observer) = 0;
// Determines the status of the given |category|, see
@@ -78,6 +79,8 @@
// Fetches the image for the suggestion with the given ID and returns it
// through the callback. This fetch may occur locally or from the internet.
+ // If that suggestion doesn't exist, doesn't have an image or if the fetch
+ // fails, the callback gets a null image.
virtual void FetchSuggestionImage(const std::string& suggestion_id,
const ImageFetchedCallback& callback) = 0;
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index 5b86d95..3ce179f4 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -61,7 +61,7 @@
ContentSuggestionsCategory category = id_category_map_[suggestion_id];
if (!providers_.count(category)) {
LOG(WARNING) << "Requested image for suggestion " << suggestion_id
- << " for unavailable category " << int(category);
+ << " for unavailable category " << static_cast<int>(category);
callback.Run(suggestion_id, gfx::Image());
return;
}
@@ -92,7 +92,7 @@
ContentSuggestionsCategory category = id_category_map_[suggestion_id];
if (!providers_.count(category)) {
LOG(WARNING) << "Discarded suggestion " << suggestion_id
- << " for unavailable category " << int(category);
+ << " for unavailable category " << static_cast<int>(category);
return;
}
providers_[category]->DiscardSuggestion(suggestion_id);
diff --git a/components/ntp_snippets/ntp_snippets_service.cc b/components/ntp_snippets/ntp_snippets_service.cc
index 51efc2a..3ca1876 100644
--- a/components/ntp_snippets/ntp_snippets_service.cc
+++ b/components/ntp_snippets/ntp_snippets_service.cc
@@ -193,10 +193,13 @@
std::unique_ptr<ImageDecoder> image_decoder,
std::unique_ptr<NTPSnippetsDatabase> database,
std::unique_ptr<NTPSnippetsStatusService> status_service)
- : state_(State::NOT_INITED),
+ : ContentSuggestionsProvider({ContentSuggestionsCategory::ARTICLES}),
+ state_(State::NOT_INITED),
+ category_status_(ContentSuggestionsCategoryStatus::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)),
@@ -204,9 +207,15 @@
database_(std::move(database)),
snippets_status_service_(std::move(status_service)),
fetch_after_load_(false) {
- if (!enabled || database_->IsErrorState()) {
- // Don't even bother loading the database.
- EnterState(State::SHUT_DOWN);
+ // In some cases, don't even bother loading the database.
+ if (!enabled) {
+ EnterState(State::SHUT_DOWN,
+ ContentSuggestionsCategoryStatus::CATEGORY_EXPLICITLY_DISABLED);
+ return;
+ }
+ if (database_->IsErrorState()) {
+ EnterState(State::SHUT_DOWN,
+ ContentSuggestionsCategoryStatus::LOADING_ERROR);
return;
}
@@ -230,7 +239,7 @@
// Inherited from KeyedService.
void NTPSnippetsService::Shutdown() {
- EnterState(State::SHUT_DOWN);
+ EnterState(State::SHUT_DOWN, ContentSuggestionsCategoryStatus::NOT_PROVIDED);
}
void NTPSnippetsService::FetchSnippets() {
@@ -263,16 +272,16 @@
}
}
-void NTPSnippetsService::FetchSnippetImage(
- const std::string& snippet_id,
+void NTPSnippetsService::FetchSuggestionImage(
+ const std::string& suggestion_id,
const ImageFetchedCallback& callback) {
database_->LoadImage(
- snippet_id,
+ suggestion_id,
base::Bind(&NTPSnippetsService::OnSnippetImageFetchedFromDatabase,
- base::Unretained(this), snippet_id, callback));
+ base::Unretained(this), suggestion_id, callback));
}
-void NTPSnippetsService::ClearSnippets() {
+void NTPSnippetsService::ClearCachedSuggestionsForDebugging() {
if (!initialized())
return;
@@ -282,8 +291,7 @@
database_->DeleteSnippets(snippets_);
snippets_.clear();
- FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
- NTPSnippetsServiceLoaded());
+ NotifyNewSuggestions();
}
std::set<std::string> NTPSnippetsService::GetSuggestionsHosts() const {
@@ -296,17 +304,17 @@
suggestions_service_->GetSuggestionsDataFromCache());
}
-bool NTPSnippetsService::DiscardSnippet(const std::string& snippet_id) {
+void NTPSnippetsService::DiscardSuggestion(const std::string& suggestion_id) {
if (!ready())
- return false;
+ return;
- auto it =
- std::find_if(snippets_.begin(), snippets_.end(),
- [&snippet_id](const std::unique_ptr<NTPSnippet>& snippet) {
- return snippet->id() == snippet_id;
- });
+ auto it = std::find_if(
+ snippets_.begin(), snippets_.end(),
+ [&suggestion_id](const std::unique_ptr<NTPSnippet>& snippet) {
+ return snippet->id() == suggestion_id;
+ });
if (it == snippets_.end())
- return false;
+ return;
(*it)->set_discarded(true);
@@ -316,12 +324,10 @@
discarded_snippets_.push_back(std::move(*it));
snippets_.erase(it);
- FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
- NTPSnippetsServiceLoaded());
- return true;
+ NotifyNewSuggestions();
}
-void NTPSnippetsService::ClearDiscardedSnippets() {
+void NTPSnippetsService::ClearDiscardedSuggestionsForDebugging() {
if (!initialized())
return;
@@ -332,6 +338,16 @@
discarded_snippets_.clear();
}
+void NTPSnippetsService::SetObserver(Observer* observer) {
+ observer_ = observer;
+}
+
+ContentSuggestionsCategoryStatus NTPSnippetsService::GetCategoryStatus(
+ ContentSuggestionsCategory category) {
+ DCHECK_EQ(ContentSuggestionsCategory::ARTICLES, category);
+ return category_status_;
+}
+
void NTPSnippetsService::AddObserver(NTPSnippetsServiceObserver* observer) {
observers_.AddObserver(observer);
}
@@ -390,7 +406,7 @@
}
void NTPSnippetsService::OnDatabaseError() {
- EnterState(State::SHUT_DOWN);
+ EnterState(State::SHUT_DOWN, ContentSuggestionsCategoryStatus::LOADING_ERROR);
}
void NTPSnippetsService::OnSuggestionsChanged(
@@ -416,8 +432,7 @@
StoreSnippetHostsToPrefs(hosts);
- FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
- NTPSnippetsServiceLoaded());
+ NotifyNewSuggestions();
FetchSnippetsFromHosts(hosts);
}
@@ -454,8 +469,7 @@
discarded_snippets_.size());
}
- FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
- NTPSnippetsServiceLoaded());
+ NotifyNewSuggestions();
}
void NTPSnippetsService::MergeSnippets(NTPSnippet::PtrVector new_snippets) {
@@ -647,8 +661,8 @@
}
void NTPSnippetsService::EnterStateDisabled() {
- ClearSnippets();
- ClearDiscardedSnippets();
+ ClearCachedSuggestionsForDebugging();
+ ClearDiscardedSuggestionsForDebugging();
expiry_timer_.Stop();
suggestions_service_subscription_.reset();
@@ -679,8 +693,7 @@
snippets_status_service_->Init(base::Bind(
&NTPSnippetsService::UpdateStateForStatus, base::Unretained(this)));
- FOR_EACH_OBSERVER(NTPSnippetsServiceObserver, observers_,
- NTPSnippetsServiceLoaded());
+ NotifyNewSuggestions();
}
void NTPSnippetsService::UpdateStateForStatus(DisabledReason disabled_reason) {
@@ -688,9 +701,11 @@
NTPSnippetsServiceDisabledReasonChanged(disabled_reason));
State new_state;
+ ContentSuggestionsCategoryStatus new_status;
switch (disabled_reason) {
case DisabledReason::NONE:
new_state = State::READY;
+ new_status = ContentSuggestionsCategoryStatus::AVAILABLE;
break;
case DisabledReason::HISTORY_SYNC_STATE_UNKNOWN:
@@ -700,27 +715,54 @@
DVLOG(1) << "Sync configuration incomplete, continuing based on the "
"current state.";
new_state = state_;
+ new_status = ContentSuggestionsCategoryStatus::INITIALIZING;
break;
case DisabledReason::EXPLICITLY_DISABLED:
+ new_state = State::DISABLED;
+ new_status =
+ ContentSuggestionsCategoryStatus::CATEGORY_EXPLICITLY_DISABLED;
+ break;
+
case DisabledReason::SIGNED_OUT:
+ new_state = State::DISABLED;
+ new_status = ContentSuggestionsCategoryStatus::SIGNED_OUT;
+ break;
+
case DisabledReason::SYNC_DISABLED:
+ new_state = State::DISABLED;
+ new_status = ContentSuggestionsCategoryStatus::SYNC_DISABLED;
+ break;
+
case DisabledReason::PASSPHRASE_ENCRYPTION_ENABLED:
+ new_state = State::DISABLED;
+ new_status =
+ ContentSuggestionsCategoryStatus::PASSPHRASE_ENCRYPTION_ENABLED;
+ break;
+
case DisabledReason::HISTORY_SYNC_DISABLED:
new_state = State::DISABLED;
+ new_status = ContentSuggestionsCategoryStatus::HISTORY_SYNC_DISABLED;
break;
default:
// All cases should be handled by the above switch
NOTREACHED();
new_state = State::DISABLED;
+ new_status = ContentSuggestionsCategoryStatus::LOADING_ERROR;
break;
}
- EnterState(new_state);
+ EnterState(new_state, new_status);
}
-void NTPSnippetsService::EnterState(State state) {
+void NTPSnippetsService::EnterState(State state,
+ ContentSuggestionsCategoryStatus status) {
+ if (status != category_status_) {
+ category_status_ = status;
+ NotifyCategoryStatusChanged();
+ }
+
if (state == state_)
return;
@@ -757,4 +799,38 @@
}
}
+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(ContentSuggestionsCategory::ARTICLES, snippet->id()),
+ snippet->best_source().url);
+ suggestion.set_amp_url(snippet->best_source().amp_url);
+ suggestion.set_title(snippet->title());
+ suggestion.set_snippet_text(snippet->snippet());
+ suggestion.set_publish_date(snippet->publish_date());
+ suggestion.set_publisher_name(snippet->best_source().publisher_name);
+ suggestion.set_score(snippet->score());
+ result.emplace_back(std::move(suggestion));
+ }
+ observer_->OnNewSuggestions(ContentSuggestionsCategory::ARTICLES,
+ std::move(result));
+}
+
+void NTPSnippetsService::NotifyCategoryStatusChanged() {
+ if (observer_) {
+ observer_->OnCategoryStatusChanged(ContentSuggestionsCategory::ARTICLES,
+ category_status_);
+ }
+}
+
} // namespace ntp_snippets
diff --git a/components/ntp_snippets/ntp_snippets_service.h b/components/ntp_snippets/ntp_snippets_service.h
index 1482fb9..6e13c3c 100644
--- a/components/ntp_snippets/ntp_snippets_service.h
+++ b/components/ntp_snippets/ntp_snippets_service.h
@@ -19,6 +19,10 @@
#include "base/timer/timer.h"
#include "components/image_fetcher/image_fetcher_delegate.h"
#include "components/keyed_service/core/keyed_service.h"
+#include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/content_suggestions_category.h"
+#include "components/ntp_snippets/content_suggestions_category_status.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
#include "components/ntp_snippets/ntp_snippet.h"
#include "components/ntp_snippets/ntp_snippets_fetcher.h"
#include "components/ntp_snippets/ntp_snippets_scheduler.h"
@@ -58,11 +62,14 @@
class NTPSnippetsServiceObserver;
// Stores and vends fresh content data for the NTP.
+// TODO(pke): Rename this service to ArticleSuggestionsService and move to
+// a subdirectory.
class NTPSnippetsService : public KeyedService,
- public image_fetcher::ImageFetcherDelegate {
+ public image_fetcher::ImageFetcherDelegate,
+ public ContentSuggestionsProvider {
public:
using ImageFetchedCallback =
- base::Callback<void(const std::string& snippet_id, const gfx::Image&)>;
+ base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
// |application_language_code| should be a ISO 639-1 compliant string, e.g.
// 'en' or 'en-US'. Note that this code should only specify the language, not
@@ -126,21 +133,17 @@
// the schedule depends on the time of day.
void RescheduleFetching();
- // Fetches the image for the snippet with the given |snippet_id| and runs the
- // |callback|. If that snippet doesn't exist or the fetch fails, the callback
- // gets a null image.
- void FetchSnippetImage(const std::string& snippet_id,
- const ImageFetchedCallback& callback);
-
- // Deletes all currently stored snippets.
- void ClearSnippets();
-
- // Discards the snippet with the given |snippet_id|, if it exists. Returns
- // true iff a snippet was discarded.
- bool DiscardSnippet(const std::string& snippet_id);
-
- // Clears the lists of snippets previously discarded by the user.
- void ClearDiscardedSnippets();
+ // ContentSuggestionsProvider implementation
+ // TODO(pke): At some point reorder the implementations in the .cc file
+ // accordingly.
+ void SetObserver(Observer* observer) override;
+ ContentSuggestionsCategoryStatus GetCategoryStatus(
+ ContentSuggestionsCategory category) override;
+ void DiscardSuggestion(const std::string& suggestion_id) override;
+ void FetchSuggestionImage(const std::string& suggestion_id,
+ const ImageFetchedCallback& callback) override;
+ void ClearCachedSuggestionsForDebugging() override;
+ void ClearDiscardedSuggestionsForDebugging() override;
// Returns the lists of suggestion hosts the snippets are restricted to.
std::set<std::string> GetSuggestionsHosts() const;
@@ -155,6 +158,9 @@
private:
FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest, HistorySyncStateChanges);
+ // TODO(pke): As soon as the DisabledReason is replaced with the new status,
+ // also remove the old State enum and replace it with
+ // ContentSuggestionsCategoryStatus and a similar status diagram.
// Possible state transitions:
// +------- NOT_INITED ------+
// | / \ |
@@ -238,8 +244,9 @@
void UpdateStateForStatus(DisabledReason disabled_reason);
// Verifies state transitions (see |State|'s documentation) and applies them.
- // Does nothing if called with the current state.
- void EnterState(State state);
+ // Also updates the provider status. Does nothing except updating the provider
+ // status if called with the current state.
+ void EnterState(State state, ContentSuggestionsCategoryStatus status);
// Enables the service and triggers a fetch if required. Do not call directly,
// use |EnterState| instead.
@@ -252,8 +259,18 @@
// directly, use |EnterState| instead.
void EnterStateShutdown();
+ // Converts the cached snippets to article content suggestions and notifies
+ // the observers.
+ void NotifyNewSuggestions();
+
+ // Notifies the content suggestions observer about a change in the
+ // |category_status_|.
+ void NotifyCategoryStatusChanged();
+
State state_;
+ ContentSuggestionsCategoryStatus category_status_;
+
PrefService* pref_service_;
suggestions::SuggestionsService* suggestions_service_;
@@ -269,8 +286,12 @@
const std::string application_language_code_;
// The observers.
+ // TODO(pke): This is an old kind of observers that will be removed.
base::ObserverList<NTPSnippetsServiceObserver> observers_;
+ // The observer to deliver data to.
+ Observer* observer_;
+
// Scheduler for fetching snippets. Not owned.
NTPSnippetsScheduler* scheduler_;
@@ -303,6 +324,8 @@
DISALLOW_COPY_AND_ASSIGN(NTPSnippetsService);
};
+// TODO(pke): Remove this when snippets internals don't access this service
+// directly anymore.
class NTPSnippetsServiceObserver {
public:
// Sent every time the service loads a new set of data.
diff --git a/components/ntp_snippets/ntp_snippets_service_unittest.cc b/components/ntp_snippets/ntp_snippets_service_unittest.cc
index 9ba767d..9a7dad0 100644
--- a/components/ntp_snippets/ntp_snippets_service_unittest.cc
+++ b/components/ntp_snippets/ntp_snippets_service_unittest.cc
@@ -414,7 +414,7 @@
LoadFromJSONString(json_str);
EXPECT_THAT(service()->snippets(), SizeIs(1));
- service()->ClearSnippets();
+ service()->ClearCachedSuggestionsForDebugging();
EXPECT_THAT(service()->snippets(), IsEmpty());
}
@@ -499,11 +499,11 @@
ASSERT_THAT(service()->snippets(), SizeIs(1));
// Discarding a non-existent snippet shouldn't do anything.
- EXPECT_FALSE(service()->DiscardSnippet("http://othersite.com"));
+ service()->DiscardSuggestion("http://othersite.com");
EXPECT_THAT(service()->snippets(), SizeIs(1));
// Discard the snippet.
- EXPECT_TRUE(service()->DiscardSnippet(kSnippetUrl));
+ service()->DiscardSuggestion(kSnippetUrl);
EXPECT_THAT(service()->snippets(), IsEmpty());
// Make sure that fetching the same snippet again does not re-add it.
@@ -517,7 +517,7 @@
EXPECT_THAT(service()->snippets(), IsEmpty());
// The snippet can be added again after clearing discarded snippets.
- service()->ClearDiscardedSnippets();
+ service()->ClearDiscardedSuggestionsForDebugging();
EXPECT_THAT(service()->snippets(), IsEmpty());
LoadFromJSONString(json_str);
EXPECT_THAT(service()->snippets(), SizeIs(1));
@@ -526,8 +526,7 @@
TEST_F(NTPSnippetsServiceTest, GetDiscarded) {
LoadFromJSONString(GetTestJson({GetSnippet()}));
- // For the test, we need the snippet to get discarded.
- ASSERT_TRUE(service()->DiscardSnippet(kSnippetUrl));
+ service()->DiscardSuggestion(kSnippetUrl);
const NTPSnippet::PtrVector& snippets = service()->discarded_snippets();
EXPECT_EQ(1u, snippets.size());
for (auto& snippet : snippets) {
@@ -535,7 +534,7 @@
}
// There should be no discarded snippet after clearing the list.
- service()->ClearDiscardedSnippets();
+ service()->ClearDiscardedSuggestionsForDebugging();
EXPECT_EQ(0u, service()->discarded_snippets().size());
}
@@ -648,7 +647,7 @@
EXPECT_EQ(snippet.best_source().amp_url, GURL());
}
- service()->ClearSnippets();
+ service()->ClearCachedSuggestionsForDebugging();
// Set Source 1 to have no AMP url, and Source 2 to have no publisher name
// Source 1 should win in this case since we prefer publisher name to AMP url
source_urls.clear();
@@ -674,7 +673,7 @@
EXPECT_EQ(snippet.best_source().amp_url, GURL());
}
- service()->ClearSnippets();
+ service()->ClearCachedSuggestionsForDebugging();
// Set source 1 to have no AMP url and no source, and source 2 to only have
// amp url. There should be no snippets since we only add sources we consider
// complete
@@ -721,7 +720,7 @@
}
// Test 2 complete sources, we should choose the first complete source
- service()->ClearSnippets();
+ service()->ClearCachedSuggestionsForDebugging();
source_urls.clear();
source_urls.push_back(std::string("http://source1.com"));
source_urls.push_back(std::string("http://source2.com"));
@@ -749,7 +748,7 @@
}
// Test 3 complete sources, we should choose the first complete source
- service()->ClearSnippets();
+ service()->ClearCachedSuggestionsForDebugging();
source_urls.clear();
source_urls.push_back(std::string("http://source1.com"));
source_urls.push_back(std::string("http://source2.com"));
@@ -813,7 +812,7 @@
IsEmpty());
// Discarding a snippet should decrease the list size. This will only be
// logged after the next fetch.
- EXPECT_TRUE(service()->DiscardSnippet(kSnippetUrl));
+ service()->DiscardSuggestion(kSnippetUrl);
LoadFromJSONString(GetTestJson({GetSnippet()}));
EXPECT_THAT(tester.GetAllSamples("NewTabPage.Snippets.NumArticles"),
ElementsAre(base::Bucket(/*min=*/0, /*count=*/3),
@@ -852,7 +851,7 @@
source_urls[0], creation, expiry, source_urls, publishers, amp_urls)}));
ASSERT_THAT(service()->snippets(), SizeIs(1));
// Discard the snippet via the mashable source corpus ID.
- EXPECT_TRUE(service()->DiscardSnippet(source_urls[0]));
+ service()->DiscardSuggestion(source_urls[0]);
EXPECT_THAT(service()->snippets(), IsEmpty());
// The same article from the AOL domain should now be detected as discarded.
