Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_provider.h

Index: components/ntp_snippets/content_suggestions_provider.h
diff --git a/components/ntp_snippets/content_suggestions_provider.h b/components/ntp_snippets/content_suggestions_provider.h
new file mode 100644
index 0000000000000000000000000000000000000000..b48d2ab2c47990fcaea03dc67dd2a9a579ece9ae
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_provider.h
@@ -0,0 +1,83 @@
+// 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>
+#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 {

[tschumann, 2016/06/28 11:47:17]

I'm not super solid in design patterns, but wouldn't this be a observer or
consumer?

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

It's technically not a delegate because we use the same object (the service
itself). It's also not really a delegation pattern because the purpose is not to
delegate. If we used two separate objects, then the service (the second object
being called) would be the delegate and not the object implementing this
interface.

The purpose here is to expose only part of the service's interface to the
provider, i.e., a provider needs to call the service for new content, but it
shouldn't call the service's shutdown method or sth like that. I don't know what
design pattern that would be.

It's also not really an observer pattern because there is no flexible
registering and unregistering of interested observers, but there when a provider
is running, there is always exactly one place that it should deliver its content
to, namely to the service.
The Producer/Consumer pattern I've only seen in the context of parallel
computing so far.

+   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 OnSuggestionsChanged(

[tschumann, 2016/06/28 11:47:17]

sorry for nit-picking. It feels a bit weird that a provider notifies no changes.
Shouldn't it notify on new data? And looking at the signature,
OnNewSuggestions() looks like a better name as you don't give any information
about what changed specifically.

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

What do you mean by "notifies no changes"? This method is not only called for
new suggestions, but also to delete suggestions - simply by not passing them
anymore in the new list.
I carefully avoided that term "new" because the current implementation should be
differentiated from the fine-grained interface (Create, Update, Reorder, Delete)
that we might implement in the future. The current implementation is equivalent
to Android's notifyDataSetChanged
(https://developer.android.com/reference/android/widget/BaseAdapter.html#notif...)
in that the callee must go figure out what exactly changed on its own.

[Marc Treib, 2016/06/28 12:05:19]

Sorry, I don't get that.. it does notify, that's what this very method does...?
Agreed, OnNewSuggestions[Available] or something like that would probably be a
better name.

[tschumann, 2016/06/28 12:32:40]

hehe, I wasn't reading closely, i read OnSuggestionChange (and dropped the 'd').
So technically, the name is alright, but I'd prefer the OnNewSomething, in case
we do want to notify about particular changes later (in which case a name like
OnSuggestionChange would be an appropriate choice).

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

It's also not "OnSuggestionChanged" but "OnSuggestionsChanged", so it doesn't
notify about the inner change of a single suggestion, but about a change
(anywhere) in the whole list of all suggestions.

In the future, we might add OnSuggestionAdd, OnSuggestionChange (no plural),
OnSuggestionRemove, etc.

Because "OnSuggestionsChanged" and "OnSuggestionChange" would be confusingly
similar, we could call the current method "OnContentChanged"?

+        const ContentSuggestionsProvider& source,
+        ContentSuggestionCategory changed_category,
+        std::vector<ContentSuggestion> suggestions) = 0;
+    virtual void OnProviderShutdown(const ContentSuggestionsProvider& source){};
+  };
+
+  // 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() {}

[tschumann, 2016/06/28 11:47:17]

wouldn't it be cleaner to make this a pure virtual class?

[Marc Treib, 2016/06/28 12:05:19]

Yep, probably. I commented more or less the same thing :)

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

Note sure, see my comment on the Discard() method over there
https://codereview.chromium.org/2102023002/patch/1/10008

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

Done.

+
+  // 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.
+  virtual void Discard(const std::string& suggestion_id) {}
+
+  // 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.
+  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_