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..602956076bc72cae4410b2363c003e1195ce4694
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -0,0 +1,194 @@
+// 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 <algorithm>
+#include <iterator>
+
+#include "base/bind.h"
+#include "ui/gfx/image/image.h"
+
+namespace ntp_snippets {
+
+namespace {
+
+// Helper method get the provider_type from a combined ID.
+ContentSuggestionsProviderType GetProviderTypeFromCombinedID(std::string id) {

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

const std::string&

[tschumann, 2016/06/30 10:55:19]

do we already have move semantics in chrome?
I'd go for const std::string& (same for GetIDFromCombinedID().

[Marc Treib, 2016/06/30 11:34:23]

We do, but there's no reason to pass in ownership here. So yes, const ref
please.

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

Done.

+  auto colon_index = id.find(":");

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

size_t please - if you say "auto", then it kinda looks like this was an iterator
IMO

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

Done.

+  DCHECK_NE(std::string::npos, colon_index);
+  int provider_type = std::stoi(id.substr(0, colon_index));

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

What happens if this fails to parse as an int?
std::stoi isn't allowed in Chromium yet, see https://chromium-cpp.appspot.com/.
The common way to do this in Chrome is base::StringToInt.

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

Done.

+  return ContentSuggestionsProviderType(provider_type);

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

What if the int isn't a valid provider type? (Shouldn't happen, so a DCHECK is
enough, no "real" error handling required.)

[tschumann, 2016/06/30 10:55:19]

use the most restrictive cast, i.e.
static_cast<ContentSuggestionsProviderType>().

[Marc Treib, 2016/06/30 11:34:23]

AFAIK that will actually do the exact same thing. I think you have to manually
check the range to really make sure.

[tschumann, 2016/06/30 11:58:11]

in this particular case, c-style cast does the same thing, but keep in mind that
the c-style cast might even apply a reinterpret cast in general
(http://en.cppreference.com/w/cpp/language/explicit_cast).

Range-checks need to be done independently, as it would be unspecified behavior.
The question is just what we do in out-of-range cases. I guess we don't want to
crash chrome, so we need to go on with something. But yeah, it would be better
to document this explicitly. (might be a bit overkill for now, but this function
could simply indicate an error and the calling side could then make sure to
drop/ignore an offending element).

[Marc Treib, 2016/06/30 13:36:12]

IMO, the out-of-range case is one that should *never* happen - the only case
where it would happen is if there was a bug in the code that generates the
combined IDs. So we should just DCHECK (i.e. assert) that it doesn't happen, and
not handle the failure.

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

I agree with Marc, and equally the case when the string (the part before the
colon) is not numeric should never happen, so also DCHECK that. Also, I don't
see any other sensible option, merely logging the problem is not possible
because something needs to be returned.

Not sure if I should still change it to static_cast? Like this, it's similar to
the other way "int(...)" and similar to other code around.

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

I find "create a ContentSuggestionsProviderType out of this int",
constructor-style, nicer than the static_cast (which IMO mostly makes sense for
pointers, not for conversions between built-in types). But if Tim disagrees, I
don't care enough to argue ;)

+}
+
+// Helper method get the original ID from a combined ID.
+std::string GetIDFromCombinedID(std::string id) {
+  auto colon_index = id.find(":");
+  DCHECK_NE(std::string::npos, colon_index);
+  return id.substr(colon_index + 1);
+}

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

I think it'd be nice if the "combine" and "extract" functions were in the same
place, next to each other. Maybe static methods on ContentSuggestion? Though
that kinda invites others to use them... hrm. I don't know.

[tschumann, 2016/06/30 11:58:11]

+1
They should live together but not on the ContentSuggestion. The
ContentSuggestion should not know anything about the ID encoding/decoding. This
file is IMO the right place to implement both.

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

I had the same dilemma. ContentSuggestion is definetely not the place because
that's where we're trying to ban the provider_type from. So if we offer a
GetProviderType() there, that completely defeats the purpose. Though we could
make it private (only the GetProviderType()) and have ContentSuggestionsService
as a friend class?

The combining conversion pretty much needs to happen in the ContentSuggestion
constructor, otherwise it gets really complicated for a provider to create a new
ContentSuggestion. Could move it into ContentSuggestionsService as public static
members, but make the GetProviderType() a private member?

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

Naaah.
What GetProviderType? ContentSuggestion shouldn't have that at all.

We could create a "content_suggestion_utils.h" or something like that, and make
it private to the component (gn has a way of specifying which headers are
considered "external" and may be used by clients, though I don't remember the
details).

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

Ok. So should I do that? Btw this is also one of that problems that would be
resolved if a category could be provided by at most one provider.

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

How exactly would that solve it? We'll still have to put the "combine" and
"extract" logic somewhere..?

[Philipp Keck, 2016/07/01 13:45:11]

Yes, but it could be public logic because categories are not an implementation
detail.

+
+// Helper method to remove all suggestions of the given |provider_type| from
+// the given |suggestions|. Returns true if anything changed.
+bool RemoveSuggestionsOfProvider(std::vector<ContentSuggestion>* suggestions,
+                                 ContentSuggestionsProviderType provider_type) {
+  auto remove_from = std::remove_if(
+      suggestions->begin(), suggestions->end(),
+      [provider_type](const ContentSuggestion& s) {
+        return GetProviderTypeFromCombinedID(s.id()) == provider_type;
+      });
+  if (remove_from == suggestions->end()) {
+    return false;
+  } else {

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

No "else" after "return"

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

I changed it. Though I personally like the shape of code like:
if (cond) return A;
else return B;

In other projects I tend to use
if (cond) return;
other_statements;
when the condition is rather exceptional and the return is more like "abort" and
I use the above form when the two cases are equally fine.

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

I agree, in the second example, the benefit is clearer. But the code style is
unambiguous: https://www.chromium.org/developers/coding-style

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

Done.

+    suggestions->erase(remove_from, suggestions->end());
+    return true;
+  }
+}
+
+}  // namespace
+
+ContentSuggestionsService::ContentSuggestionsService(bool enabled)
+    : enabled_(enabled) {}
+
+ContentSuggestionsService::~ContentSuggestionsService() {}
+
+void ContentSuggestionsService::Shutdown() {
+  FOR_EACH_OBSERVER(Observer, observers_, ContentSuggestionsServiceShutdown());
+  observers_.Clear();

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

Hm. Since the observers register themselves, they should probably also
unregister themselves. So DCHECK(observers_.empty()) ?

[tschumann, 2016/06/30 11:58:11]

+1

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

Marc's precondition is not perfectly true: Observers don't register themselves,
but they get registered by their factory. They don't have a reference to the
service, so they couldn't call an Unregister(Provider) that is placed next to
the Register(Provider) in the service. Instead, there would need to be a
Unregister(ProviderType) on the Observer interface, which is somewhat similar to
the OnProviderShutdown method there (except that the provider might not want to
shut down, so it would be a separate method?).

It would make the shutdown more inefficient in that the service would need to
remove and filter out all suggestions of every provider that's shutting down
instead of just doing categories_.clear() and suggestions_by_category_.clear(),
which is missing here btw.
So maybe the service would need a state "shutting down"?

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

But the observers need a reference to the service anyway, to get the actual
data, right?

Anyway, the Register and Unregister should be handled from the same place. So
the factory is probably not the right place for that.

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

The Unregistering would need to be triggered from the provider itself, once it
notices that it must shut down. Is that right?
Assuming that's true, the Registering would need to happen there, too (so both
are in one place), and that would require us to pass a reference to the service
into the providers, but we specifically don't want it there because we have the
Observer (former Delegate) interface to reduce the interface of the service that
the providers can use.
So the provider factory would need to pass a
ContentSuggestionsProvider::Observer pointer to the service into the provider
and the Observer would get a Register method? To make this consistent with the
Observer idea, Register and Unregister would need to be called OnStart and
OnShutdown or sth. like that.

This would fit in with the proposed requirement that there is at most one
provider per category. The ContentSuggestionsProvider::Observer interface could
be:
+OnStartCategory(category): bool  <-- "I want to provide data for this category.
Return value says if that's okay or if there is already another provider for
that"
+OnStopCategory(category) <-- "I won't provide data for this category anymore."
equivalent to current provider shutdown, but more finegrained.
+OnSuggestionsChanged(category, data) <-- "For this category, which I have been
allowed to provide, I deliver new data"

When a provider receives the shutdown signal from the service, it would need to
stop all of its categories.

The ContentSuggestionsService::Observer::OnSuggestionInvalidated() for the UI
could also be refined with a category parameter, because once a provider stops a
category, we know that there cannot be any other providers for that category.

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

Hm, I think the categories should be registered "as statically as possible",
since we never expect them to change at runtime, and so we also never expect
conflicts.
If the providers are all KeyedServices, they would just unregister on their own
shutdown. When the service itself is shut down, it would just DHCECK that there
are no registered providers anymore.

[Philipp Keck, 2016/07/01 13:45:11]

I agree about the static categories, when conflicts are detected, something went
wrong. Though I'm not sure if we can already detect such conflicts at compile
time (any suggestions?) or if we simply have to make sure that we don't make any
mistakes and we DCHECK-fail if there is a conflict.

A provider's provided categories are static and a provider needs to tell about
its provided categories even before OnStartCategory is called (because the
service will want to ask why the category is missing if it hasn't been started),
probably through a constructor parameter?
But it also needs to inform about status changes (data is available for the
first time, data has changed, data cannot be available because something is
disabled). For the latter I'm not sure if this should be per provider or per
provider-category. In the latter case, we'd still need OnStartCategory() or
something like that.

For the shutdown I agree, even if the providers are not KeyedServices
themselves, but just owned by one, it would work like that and when the
ContentSuggestionsService shuts down, it just DCHECKs.

[Marc Treib, 2016/07/01 15:08:14]

DCHECK should be fine; I don't actually expect any problems here.
I guess when the provider is registered with the service, it'd also list its
categories?
Yeah, that makes sense. Especially the "clear this category" would handle the
sign-out case.

+  for (auto& provider : providers_) {
+    provider.second->SetObserver(nullptr);
+  }
+  providers_.clear();
+  enabled_ = false;

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

Also clear the cached suggestions, just to be sure?

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

Yes, see previous comment.

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

Hmm?

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

Depending on how we change the shutdown, I would add
"suggestions_by_category_.clear()" as you suggested, or even
"DCHECK(suggestions_by_category_.empty())", if the require the providers to shut
down cleanly.

[Marc Treib, 2016/07/01 15:08:14]

Acknowledged.

+}
+
+const std::vector<ContentSuggestion>&
+ContentSuggestionsService::GetSuggestionsForCategory(
+    ContentSuggestionCategory category) const {
+  return suggestions_by_category_.at(category);

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

Hm, so this assumes that it'll never be called for a category that doesn't have
suggestions. Should it maybe return an empty vector in that case? Otherwise, at
least add a comment in the header saying that this mustn't be called for
categories that aren't in GetCategories.

[tschumann, 2016/06/30 10:55:19]

hmmm... is there anything guaranteeing that we have indeed an entry for every
category enum? this might be hard to enforce, so maybe handle this gracefully?

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

In the obvious, simple Android implementation, this would only be called
directly after calling GetCategories(), and only be called for every entry in
categories_.

An empty list would be different from no list or an error. This also depends on
the way the status cards work (are the suggestions themselves or not).

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

"no list" isn't an option if you return a reference.
Anyway, crashing when this is called with a currently empty category seems like
a bad API.

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

Changed to empty list. 

+}
+
+void ContentSuggestionsService::FetchSuggestionImage(
+    const std::string& suggestion_id,
+    const ImageFetchedCallback& callback) {
+  ContentSuggestionsProviderType provider_type =
+      GetProviderTypeFromCombinedID(suggestion_id);
+  std::string original_id = GetIDFromCombinedID(suggestion_id);

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

nit: move this into the "if"

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

Done.

+  if (providers_.count(provider_type)) {
+    providers_[provider_type]->FetchSuggestionImage(
+        original_id, base::Bind(callback, suggestion_id));
+  } else {
+    LOG(WARNING) << "Requested image for suggestion " << suggestion_id
+                 << " for unavailable provider type " << int(provider_type);
+    callback.Run(suggestion_id, gfx::Image());
+  }
+}
+
+void ContentSuggestionsService::ClearCachedSuggestions() {
+  categories_.clear();
+  suggestions_by_category_.clear();
+  for (auto& provider : providers_) {
+    provider.second->ClearCachedSuggestions();
+  }
+  FOR_EACH_OBSERVER(Observer, observers_, OnSuggestionsChanged());
+}
+
+void ContentSuggestionsService::ClearDiscardedSuggestions() {
+  for (auto& provider : providers_) {
+    provider.second->ClearDiscardedSuggestions();
+  }
+}
+
+void ContentSuggestionsService::DiscardSuggestion(
+    const std::string& suggestion_id) {
+  ContentSuggestionsProviderType provider_type =
+      GetProviderTypeFromCombinedID(suggestion_id);
+  std::string original_id = GetIDFromCombinedID(suggestion_id);
+
+  if (providers_.count(provider_type)) {
+    providers_[provider_type]->DiscardSuggestion(original_id);
+
+    // Remove the suggestion locally.
+    for (auto& pair : suggestions_by_category_) {

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

Can't you get the category from the suggestions, rather than going over all of
them?

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

Then I would need to include the category in the DiscardSuggestion() signature.
Or make it part of the ID.

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

Ah, right, sorry. Carry on :)

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

It might be part of the ID, if we enforce that the category uniquely identifies
the provider :-) So that would even make this code nicer and faster.

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

Yup, SGTM.

+      auto position =
+          std::find_if(pair.second.begin(), pair.second.end(),
+                       [suggestion_id](const ContentSuggestion& suggestion) {

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

You could capture this by reference, to save a string copy

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

Done.

+                         return suggestion_id == suggestion.id();
+                       });
+      if (position != pair.second.end()) {
+        pair.second.erase(position);

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

Add a break? Once we're removed the suggestion, there's no reason to keep
looking.

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

Done.

+      }
+    }
+  } else {

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

nit: Turning the if around might make things a bit easier to read (remove one
level of indentation):
if (no provider)
  LOG
  return
carrying on...

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

Done.

+    LOG(WARNING) << "Discarded suggestion " << suggestion_id
+                 << " for unavailable provider type "
+                 << static_cast<base::underlying_type<
+                        ContentSuggestionsProviderType>::type>(provider_type);

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

int(provider_type)

[tschumann, 2016/06/30 11:58:11]

i'd vote for implicit_cast<int> as it's the least powerful cast to do the job.

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

Wasn't sure where to import the implicit_cast from, so it's int() for now.

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

implicit_cast isn't a standard C++ feature, and doesn't exist in Chromium :)

[tschumann, 2016/07/01 09:55:59]

=) argh... spent too much time in google3 it seems... so static_cast it is :-)

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

It's "int(provider_type)" now? See discussion above (line 22).

+  }
+}
+
+void ContentSuggestionsService::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ContentSuggestionsService::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void ContentSuggestionsService::RegisterProvider(
+    ContentSuggestionsProvider* provider) {
+  DCHECK_EQ(0ul, providers_.count(provider->GetProviderType()));
+  providers_[provider->GetProviderType()] = provider;
+  provider->SetObserver(this);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private methods
+
+void ContentSuggestionsService::OnSuggestionsChanged(
+    ContentSuggestionsProviderType provider_type,
+    ContentSuggestionCategory changed_category,
+    std::vector<ContentSuggestion> new_suggestions) {
+  RemoveSuggestionsOfProvider(&suggestions_by_category_[changed_category],
+                              provider_type);
+
+  std::move(new_suggestions.begin(), new_suggestions.end(),
+            std::back_inserter(suggestions_by_category_[changed_category]));
+
+  // Ensure the category exists exactly if its list of suggestions is nonempty.
+  auto position =
+      std::find(categories_.begin(), categories_.end(), changed_category);
+  if (suggestions_by_category_[changed_category].empty()) {
+    suggestions_by_category_.erase(changed_category);
+    if (position != categories_.end()) {
+      categories_.erase(position);
+    }
+  } else {
+    if (position == categories_.end()) {
+      // TODO(pke) Useful ordering of categories.

[tschumann, 2016/06/30 10:55:19]

can you explain this TODO() in a bit more detail? =)

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

Done.

+      categories_.emplace_back(changed_category);
+    }
+  }
+
+  FOR_EACH_OBSERVER(Observer, observers_, OnSuggestionsChanged());
+}
+
+void ContentSuggestionsService::OnProviderShutdown(
+    ContentSuggestionsProviderType provider_type) {
+  auto iterator = suggestions_by_category_.begin();
+  while (iterator != suggestions_by_category_.end()) {
+    if (RemoveSuggestionsOfProvider(&iterator->second, provider_type) &&
+        iterator->second.empty()) {
+      categories_.erase(
+          std::find(categories_.begin(), categories_.end(), iterator->first));
+      iterator = suggestions_by_category_.erase(iterator);
+    } else {
+      ++iterator;
+    }
+  }
+  FOR_EACH_OBSERVER(Observer, observers_, OnSuggestionsChanged());
+  providers_.erase(provider_type);
+}
+
+}  // namespace ntp_snippets