Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_provider.h

+// 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.
+
+#ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_
+
+#include <set>

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

not needed

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

Done.

+#include <string>
+#include <vector>
+
+#include "base/callback.h"
+#include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/content_suggestions_provider_type.h"
+
+namespace gfx {
+class Image;
+}
+
+namespace ntp_snippets {
+
+// Provides content suggestions from one particular source. The provided
+// suggestions can belong to different ContentSuggestionCategories. A provider
+// can be a keyed service, in which case it should notify the
+// ContentSuggestionsService through the delegate before it shuts down.
+class ContentSuggestionsProvider {
+ public:
+  using ImageFetchedCallback =
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
+
+  // The delegate of a provider is notified when new data is available.
+  class Delegate {
+   public:
+    // Called when the available content changed.
+    // If a provider provides suggestions of multiple categories, it must call
+    // this callback for every category individually. The |suggestions|
+    // parameter should always contain the full list of currently available
+    // suggestions of that category, i.e., passing an empty list will remove
+    // all suggestions of the given category for the provider. Note that to
+    // clear them from the UI immediately, the provider needs to shut down. The
+    // list must not contain ContentSuggestions assigned to a category other
+    // than the passed |changed_category|.
+    virtual void OnContentChanged(
+        const ContentSuggestionsProvider& source,

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

Should this just pass the provider type? Seems like that is all the service
needs anyway.

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

That would indeed be enough, the service has the map<type, instance> anyway.
The reason I just chose this variant without really thinking about it is because
normally with these callback/observer patterns you just pass the instance. So to
me, it's part of the design pattern.

I'm happy to change it though, they seem about equal to me in terms of
performance and readability.

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

Oh, I'm not worried about performance. But the service really only wants the ID,
it doesn't do anything else with the provider. So I'd prefer to make that
obvious.

[tschumann, 2016/06/28 16:48:35]

I don't want to sidetrack the whole discussion, but it feels a bit weird to me
that we need to expose a ContentSuggestionsProviderType at all. Sure, for a
registry we might need something like this, but this should only be some
internal part. That aside, I'd hope that we can simply handle the different
types by passing references/pointers of objects around. If you don't mind, a
quick design discussion might be nice to collect some feedback here.
Don't get me wrong, I don't want to nitpick, but I believe it would be a good
point in time to gather feedback from the group and make sure people know what's
going on. That said, we should keep the initial audience small to make sure the
discussion stays productive and then simply present the design we're happy with
to the larger team.

On a side note, Markus told me he put together a document with current supported
user interactions -- would be nice to play them through and see how they would
be supported by this API.

What do you think?

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

Thank you. The code here now passes the provider_type (as its the interface
between the provider and the service), but the interface towards the UI merges
the provider_type with the ID.

+        ContentSuggestionCategory changed_category,
+        std::vector<ContentSuggestion> suggestions) = 0;
+    virtual void OnProviderShutdown(const ContentSuggestionsProvider& source){};

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

nit: space before "{" and no ";" (though really, this probably shouldn't have a
default implementation)

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

Done. Replaced with "= 0". You are right, default implementation only makes
sense for observers (where different observers might be interested in different
things).

+  };
+
+  // Returns the type of the provider.
+  virtual ContentSuggestionsProviderType GetProviderType() const = 0;
+
+  // Accessor for delegate, which is notified about new available suggestions.
+  virtual void SetDelegate(Delegate* delegate) = 0;
+
+  // Used only for debugging purposes. Clears all caches so that the next
+  // fetch starts from scratch.
+  virtual void ClearSuggestions() {}
+
+  // Used only for debugging purposes. Clears the cache of discarded
+  // suggestions, if present, so that no suggestions are suppressed. This does
+  // not necessarily make previously discarded suggestions reappear, as they may
+  // have been permanently deleted, depending on the provider implementation.
+  virtual void ClearDiscardedSuggestions() {}
+
+  // Discards the suggestion with the given ID. A provider needs to ensure that
+  // a once-discarded suggestion is never delivered again (through the
+  // Delegate). It can do so by deleting it from the underlying source or by
+  // marking it as discarded internally.

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

I'd remove the last sentence, since it's not really relevant for the interface.

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

Done.

+  virtual void Discard(const std::string& suggestion_id) {}

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

This shouldn't have a default implementation.
Probably the Clear methods shouldn't either.

Also, DiscardSuggestion? For (a) more clarity and (b) consistency with the Clear
methods.

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

Removed the default implementation.

I left the default implementation in place for ClearSuggestions and
ClearDiscardedSuggestions. Those are only for debugging purposes and some
provider's might not be able to implement any functionality there. E.g. the
offline page provider probably won't do anything to the stored offline pages
when someone clicks a button for ClearSuggestions on the internals page
(especially not delete the stored pages). Some other providers might discard
suggestions by actually deleting them from the underlying model, so they won't
need to keep track of previously discarded suggestions and thus they won't need
ClearDiscardedSuggestions.

The motivation for "ClearSuggestions()" vs. "Discard(suggestion)" is that the
latter already contains the "suggestion" in the argument. For that reason, I
also removed the "Snippet" from "FetchSnippetImage", so that it now reads
"FetchImage(some_suggestion, ..)".

But to be honest, another reason was: It saved me a lot of renaming from
"Snippet" to "SuggestedContent" to "Card" to "ContentSuggestion". So now that we
fixed the terms, I'm okay with adding the "Suggestion" back in:
"DiscardSuggestion" and/or "FetchSuggestionImage".

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

I think it's still better for those providers to explicitly say "I don't do
anything about this", rather than having some potentially bad default behavior.
But it's not a huge deal.
Fair enough ;)  Yup, I'd prefer adding the "Suggestion" back.

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

Done.

+
+  // 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.

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

nit: don't capitalize internet

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

Done.

+  virtual void FetchImage(const std::string& suggestion_id,
+                          const ImageFetchedCallback& callback) = 0;
+
+ protected:
+  virtual ~ContentSuggestionsProvider() {}
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_