Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..96983fc781ff1df83615605afb0d38a7f07f0d46
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -0,0 +1,132 @@
+// 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 "components/ntp_snippets/content_suggestions_service.h"
+
+#include "base/observer_list.h"

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

Not needed, since it's already included in the header.

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

Done.

+#include "ui/gfx/image/image.h"
+
+namespace ntp_snippets {
+
+ContentSuggestionsService::ContentSuggestionsService(bool enabled)
+    : enabled_(enabled) {
+  if (enabled_) {

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

Remove empty if.
It looks like the enabled_ flag currently doesn't do anything at all? It
probably should...

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

The reason that there is no code is not that the enabled flag doesn't do
anything. The reason is that the constructor currently doesn't need to do
anything because there is simply no setup to be done.

When the service is disabled, none of the providers' factories will register a
provider, so the service will remain empty. We could additionally prevent calls
to functions like RegisterProvider, GetSuggestionsForCategory and possibly all
others or make them return empty results.

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

Alright, but then remove the empty if.
Hm. If the "enabled" state can change at runtime (e.g. because we expose a
setting), then it might make sense to register the providers anyway, in case the
service later gets enabled. WDYT?

[Philipp Keck, 2016/06/30 09:35:35]

I removed the empty if for now. The question how to properly implement that
depends on how status cards are handled, i.e., where they are produced and, if
that's in the backend, how they are passed to the UI. Are they special
suggestions (to turn some settings on), are they a separate Delegate/Callback
method, or is it just a Service::is_enabled() that returns false and the UI
needs to realise that?

I agree: All (available) providers should always be registered with the service.
If disabled, the service should clear its data stores and somehow tell the UI.
Additionally, the service could make sure not to return any suggestions (except
maybe the appropriate status cards).
If disabled, a provider should stop fetching data and not deliver data to the
service anymore and tell the service that it's disabled (and the service removes
the corresponding data). How does a provider know that the main switch/feature
is turned off? Maybe through a ContentSuggestionsService::is_enabled() getter
and a Delete::ServiceDisabled() notification? Then we'd also need
Delegate::ServiceEnabled() to re-enable.

[Marc Treib, 2016/06/30 10:36:53]

Hm, right, we need "StateChanged" notifications both ways (IMO this should be
one method with a parameter, rather than separate Enabled/Disabled ones). So
yes, I guess we need a method both on the ServiceObserver and the
ProviderObserver/Delegate.

[Philipp Keck, 2016/06/30 17:14:07]

Yes, but I'm not implementing that (now), right?

[Marc Treib, 2016/07/01 09:15:32]

Yeah, another CL is fine, but we'll need at least some of it before we can
switch to the new service (to clear out any suggestions on sign-out).

[Philipp Keck, 2016/07/01 13:00:03]

Yes. I'm thinking about extending the new Enabled enum to make it a status enum
similar to the new state handling in NTPSnippetsService. This would be a lot
easier if we enforced a mapping from categories to providers (i.e. a category
can be provided by at most one provider; if more providers want to, the second
one will be ignored). Then we could have (slightly) category-specific code even
in the UI; or generic code and just a list of possible categories in the UI. And
the UI could ask "what's the status of category X?", whereupon the service could
say "there's not even a provider for that" or it could ask the provider for the
status of that category and forward the answer to the UI.
Without such a mapping, it is unclear why a category is currently empty: Are the
responsible provider(s) still loading? Or have they loaded an empty set? Have
they encountered errors? Are they not registered?

Another advantage of such a mapping is that we could get rid of the
provider_type altogether, though we still would have to encode the category into
the ID for convenience.

[Marc Treib, 2016/07/01 13:29:17]

The category -> provider mapping sounds like a good idea to me. In particular,
it'll make our lives easier with respect to the empty state and "more" cards,
since those can then only come from one provider.

+  }
+}
+
+ContentSuggestionsService::~ContentSuggestionsService() {}
+
+void ContentSuggestionsService::RegisterProvider(
+    ContentSuggestionsProvider* provider) {
+  DCHECK(providers_.count(provider->GetProviderType()) == 0);

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

DCHECK_EQ(0, ...)

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

Done.

+  providers_[provider->GetProviderType()] = provider;
+  provider->SetDelegate(this);
+}
+
+void ContentSuggestionsService::Shutdown() {
+  FOR_EACH_OBSERVER(ContentSuggestionsServiceObserver, observers_,
+                    ContentSuggestionsServiceShutdown());
+  observers_.Clear();
+  for (auto& provider : providers_) {
+    provider.second->SetDelegate(nullptr);
+  }
+  providers_.clear();
+  enabled_ = false;
+}
+
+void ContentSuggestionsService::FetchImage(
+    const ContentSuggestionsProviderType provider_type,
+    const std::string& suggestion_id,
+    const ImageFetchedCallback& callback) {
+  if (providers_.count(provider_type)) {
+    providers_[provider_type]->FetchImage(suggestion_id, callback);
+  } else {
+    LOG(WARNING) << "Requested image for suggestion" << suggestion_id
+                 << " for unavailable provider type "
+                 << static_cast<base::underlying_type<
+                        ContentSuggestionsProviderType>::type>(provider_type);

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

Argh, this is ugly - I guess it doesn't compile without the cast?
IMO just "int(provider_type)" is good enough for a LOG statement. Or maybe it's
worth adding a "string GetProviderTypeName(ProviderType)" function?

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

Done.

+    callback.Run(suggestion_id, gfx::Image());
+  }
+}
+
+void ContentSuggestionsService::ClearSuggestions() {
+  for (auto& provider : providers_) {
+    provider.second->ClearSuggestions();
+  }
+}
+
+void ContentSuggestionsService::ClearDiscardedSuggestions() {
+  for (auto& provider : providers_) {
+    provider.second->ClearDiscardedSuggestions();
+  }
+}
+
+void ContentSuggestionsService::Discard(
+    const ContentSuggestionsProviderType provider_type,
+    const std::string& suggestion_id) {
+  if (providers_.count(provider_type)) {
+    providers_[provider_type]->Discard(suggestion_id);
+  } else {
+    LOG(WARNING) << "Discarded suggestion " << suggestion_id
+                 << " for unavailable provider type "
+                 << static_cast<base::underlying_type<
+                        ContentSuggestionsProviderType>::type>(provider_type);
+  }
+}
+
+void ContentSuggestionsService::AddObserver(
+    ContentSuggestionsServiceObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ContentSuggestionsService::RemoveObserver(
+    ContentSuggestionsServiceObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void ContentSuggestionsService::OnContentChanged(
+    const ContentSuggestionsProvider& source,
+    ContentSuggestionCategory changed_category,
+    std::vector<ContentSuggestion> new_suggestions) {
+  RemoveSuggestionsOfProvider(&suggestions_[changed_category],
+                              source.GetProviderType());
+
+  std::move(new_suggestions.begin(), new_suggestions.end(),

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

#include <algorithm>

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

Done.

+            std::back_inserter(suggestions_[changed_category]));

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

#include <iterator>

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

Done.

+
+  FOR_EACH_OBSERVER(ContentSuggestionsServiceObserver, observers_,
+                    OnSuggestionsChanged(changed_category));
+}
+
+void ContentSuggestionsService::OnProviderShutdown(
+    const ContentSuggestionsProvider& source) {
+  for (auto& pair : suggestions_) {
+    if (RemoveSuggestionsOfProvider(&pair.second, source.GetProviderType())) {
+      FOR_EACH_OBSERVER(ContentSuggestionsServiceObserver, observers_,
+                        OnSuggestionsChanged(pair.first));
+    }
+  }
+  providers_.erase(source.GetProviderType());
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private methods
+
+bool ContentSuggestionsService::RemoveSuggestionsOfProvider(
+    std::vector<ContentSuggestion>* list,

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

Call this "suggestions" maybe? It's not actually a list.

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

Done.

+    ContentSuggestionsProviderType provider_type) {
+  auto provider_matcher = [provider_type](const ContentSuggestion& s) {

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

Inline this into the remove_if?

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

Done.

+    return s.provider() == provider_type;
+  };
+  auto remove_from =
+      std::remove_if(list->begin(), list->end(), provider_matcher);
+  if (remove_from == list->end()) {
+    return false;
+  } else {
+    list->erase(remove_from, list->end());

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

You could also use the common erase(remove_if, end) idiom, and compare the
return value to the old end() to see if anything changed.

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

Done.

[Philipp Keck, 2016/06/30 09:35:35]

Undone. Reverted to old code because checking whether sth changed with the usual
erase(remove_if) idiom doesn't work.

+    return true;
+  }
+}
+
+}  // namespace ntp_snippets