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}

chrome/browser/android/ntp/ntp_snippets_bridge.cc

@@ -93,7 +93,7 @@ void NTPSnippetsBridge::FetchImage(JNIEnv* env,
                                    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::FetchImage(JNIEnv* env,
 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,

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 image_fetcher::ImageFetcherImpl;
 using suggestions::ImageDecoderImpl;
 using suggestions::SuggestionsService;
 using suggestions::SuggestionsServiceFactory;
+using ntp_snippets::ContentSuggestionsService;
 
 // static
 NTPSnippetsServiceFactory* NTPSnippetsServiceFactory::GetInstance() {
@@ -70,6 +73,7 @@ NTPSnippetsServiceFactory::NTPSnippetsServiceFactory()
   DependsOn(ProfileSyncServiceFactory::GetInstance());
   DependsOn(SigninManagerFactory::GetInstance());
   DependsOn(SuggestionsServiceFactory::GetInstance());
+  DependsOn(ContentSuggestionsServiceFactory::GetInstance());
 }
 
 NTPSnippetsServiceFactory::~NTPSnippetsServiceFactory() {}
@@ -78,6 +82,13 @@ KeyedService* NTPSnippetsServiceFactory::BuildServiceInstanceFor(
     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 @@ KeyedService* NTPSnippetsServiceFactory::BuildServiceInstanceFor(
               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;
 }

chrome/browser/ui/webui/snippets_internals_message_handler.cc

@@ -114,14 +114,14 @@ void SnippetsInternalsMessageHandler::HandleLoaded(
 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();
 }
 

components/ntp_snippets/content_suggestions_category_status.h

@@ -11,6 +11,10 @@ namespace ntp_snippets {
 // 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 @@ enum class ContentSuggestionsCategoryStatus {
   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.

components/ntp_snippets/content_suggestions_provider.cc

@@ -23,7 +23,8 @@ ContentSuggestionsProvider::~ContentSuggestionsProvider() {}
 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());
 }
 

components/ntp_snippets/content_suggestions_provider.h

@@ -62,7 +62,8 @@ class ContentSuggestionsProvider {
   };
 
   // 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 @@ class ContentSuggestionsProvider {
 
   // 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;
 

components/ntp_snippets/content_suggestions_service.cc

@@ -61,7 +61,7 @@ void ContentSuggestionsService::FetchSuggestionImage(
   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 @@ void ContentSuggestionsService::DiscardSuggestion(
   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);