Add ContentSuggestionsService

chrome/browser/ntp_snippets/content_suggestions_service_factory.cc

Index: chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
diff --git a/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
new file mode 100644
index 0000000000000000000000000000000000000000..144ff61eda7873285a1019420b3c63a23a659205
--- /dev/null
+++ b/chrome/browser/ntp_snippets/content_suggestions_service_factory.cc
@@ -0,0 +1,52 @@
+// 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 "chrome/browser/ntp_snippets/content_suggestions_service_factory.h"
+
+#include "base/feature_list.h"
+#include "base/memory/singleton.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/ntp_snippets/content_suggestions_service.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_context.h"
+
+#if defined(OS_ANDROID)
+#include "chrome/browser/android/chrome_feature_list.h"
+#endif  // OS_ANDROID
+
+// static
+ContentSuggestionsServiceFactory*
+ContentSuggestionsServiceFactory::GetInstance() {
+  return base::Singleton<ContentSuggestionsServiceFactory>::get();
+}
+
+// static
+ntp_snippets::ContentSuggestionsService*
+ContentSuggestionsServiceFactory::GetForProfile(Profile* profile) {
+  DCHECK(!profile->IsOffTheRecord());
+  return static_cast<ntp_snippets::ContentSuggestionsService*>(
+      GetInstance()->GetServiceForBrowserContext(profile, true));
+}
+
+ContentSuggestionsServiceFactory::ContentSuggestionsServiceFactory()
+    : BrowserContextKeyedServiceFactory(
+          "ContentSuggestionsService",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+ContentSuggestionsServiceFactory::~ContentSuggestionsServiceFactory() {}
+
+KeyedService* ContentSuggestionsServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  // TODO(mvanouwerkerk): Move the enable logic into the service once we start
+  // observing pref changes.
+  bool enabled = true;
+#if defined(OS_ANDROID)
+  enabled = enabled &&
+            base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature);

[Marc Treib, 2016/06/28 11:29:08]

Hm. I guess for now it's okay to reuse the same flag, but please add a TODO to
split off a separate one.

[Philipp Keck, 2016/06/28 12:04:08]

I have a separate "article-suggestions" feature in an upcoming CL. My intention
is to leave the current "snippets feature", which is disabled by default, as the
main toggle for the entire NTP suggestions stuff. Maybe rename it to
"ntp-content-suggestions". The new "article-suggestions" and then
"offline-pages-suggestions" features will be enabled by default and turn off the
sub-providers.

So:
ContentSuggestionsService will be enabled iff "snippets" is true (exactly as
coded here, possibly renamed).
ArticleSuggestionsService (pka NTPSnippetsService) will be enabled iff "snippets
&& article-suggestions" is true (so I will add check for article-suggestions to
the NTPSnippetsServiceFactory).

Is that okay? Can features be renamed without further consequences?

[Marc Treib, 2016/06/28 12:09:50]

Mostly SGTM, see below.
ArticleSuggestionsService probably shouldn't check for the "snippets" feature,
but instead check if the ContentSuggestionsService exists.
No, the actual string can not be renamed easily. We already have live configs
that reference the existing name; we'd need to update them with both the old and
new name, to support old and new Chrome versions. What we can do though is to
rename only the variable (kNTPSnippetsFeature -> kNTPContentSuggestions or
something like that), but keep the string as-is.

[Philipp Keck, 2016/06/28 14:18:55]

Acknowledged. So in that case, it seems especially important to make the old
"snippets" feature become the new top-level switch and introduce the "articles"
as a new switch. I assume that the live configs want to control the top-level
feature, not the underlying provider.

[Marc Treib, 2016/06/28 15:04:55]

Yup!

+#endif  // OS_ANDROID
+
+  return new ntp_snippets::ContentSuggestionsService(enabled);
+}