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..af801fb02bb0ac19518c9447eeec897e8a300815
--- /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 <string>
+#include <vector>
+
+#include "base/callback_forward.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 observer before it shuts down.
+class ContentSuggestionsProvider {
+ public:
+  using ImageFetchedCallback = base::Callback<void(const gfx::Image&)>;
+
+  // The observer of a provider is notified when new data is available.
+  class Observer {
+   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(

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

Did we agree on a name here?

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

personally, I'd try to avoid the term 'change' in this call as it suggests more
info about the actual change (which might be added at some point in the future
or not). Given that you simply inform about new data, I'd sympathize more with
the names suggested by Marc before (OnNewSuggestions[Available] or something).

However, this should also be simple enough to change once we add the new
functions (if ever), so I'm not feeling strongly here. Feel free to leave as is
if you prefer the current name.

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

I also suggested OnContentChanged earlier.

The name "OnNewSuggestions" sounds confusing to me because the method is not
only called when there are new suggestions and the |suggestions| parameter does
not only contain the new suggestions, but it contains all suggestions and when
old ones are missing, then the result of calling this function is effectively
deleting suggestions (which is the opposite of "new suggestions").

What happens is: The whole list of available suggestions has (somehow) been
updated. It could be that new suggestions were added, an existing one was
modified, the list was reordered or some suggestions were deleted. And it's
there four more fine-grained events that I would call:
OnSuggestionsAdd/OnNewSuggestions, OnSuggestionModify, OnSuggestionMove,
OnSuggestionsDelete -- so the name OnNewSuggestions would already be taken for
the more specific event.

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

IMO "OnSuggestionsAdded" would be a better (because less ambiguous) name for the
potential fine-grained event.
Anyway, we've bike-shedded this name enough, and we can still change it later if
the need arises. So either "OnSuggestionsChanged" or
"OnNewSuggestions[Available]" are fine with me.

[tschumann, 2016/07/01 09:37:10]

I see your point. My motivation was that the Observer should not interpret the
single elements and not care about partial updates at all. If there's new data
(which might as well be the same that was displayed before), the existing data
should be thrown away and replaced by the new data. The observer should not care
about the deeper meaning of a new suggestion (i.e. whether a suggestion is new
to the user or not) -- it should simply display whatever the service gives. 
IMO, the examples of more fine grained methods fit nicely into this model. 

OnSuggestionsChange() suggest that there's an actual change in data and that the
observer might care about that change. But it might as well be the very same
data set.

Anyways, I feel we're spending too much time on this. Both are probably
acceptable, just wanted to get my point across ;-) Like I said, pick one :-)

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

Ok I understand your point now. It's basically a short form of
"OnNewListOfSuggestionsAvailable" and not
"OnNewSuggestionsHaveBeenAddedToTheList". I can see how "OnSuggestionsChanged"
(short for "OnOverallListOfSuggestionsHasChanged") can be confusing, I changed
it.

+        ContentSuggestionsProviderType provider_type,
+        ContentSuggestionCategory changed_category,
+        std::vector<ContentSuggestion> suggestions) = 0;
+
+    virtual void OnProviderShutdown(

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

can you document this function? When is it called? What happens after the call?

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

Done.

+        ContentSuggestionsProviderType provider_type) = 0;
+  };
+
+  // Returns the type of the provider.
+  virtual ContentSuggestionsProviderType GetProviderType() const = 0;

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

Hm, you could actually store the provider type here in the base class (passed in
from the subclass via a protected ctor). That removes some duplication between
the subclasses, and also makes sure they can't change their type at runtime.
WDYT?

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

Done.

+
+  // Accessor for observer, which is notified about new available suggestions.
+  virtual void SetObserver(Observer* observer) = 0;
+
+  // Used only for debugging purposes. Clears all caches so that the next
+  // fetch starts from scratch.
+  virtual void ClearCachedSuggestions() = 0;

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

for functions which should not be called in production code, I like the
convention to include this fact in there name. This makes it visible at the call
site.
Something like: ClearCachedSuggestionsForDebugging().
If these functions are called in tests, I'd go with
ClearCachedSuggestionsForTesting().

Same goes for ClearDiscardedSuggestions().

Also, test/debug-only methods are not really part of the public interface, so
I'd move them to the end of the public section to not confuse the reader.

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

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() = 0;
+
+  // Discards the suggestion with the given ID. A provider needs to ensure that

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

nit: I'd put the "real" API methods before the debugging-only ones.

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

Done.

+  // a once-discarded suggestion is never delivered again (through the
+  // Observer). There is no need to call the observer for an update after a

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

This formulation leaves it open whether the observer will be called or not. We
should make a decision and stick with it.

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

I didn't want to require that to happen because it would be inefficient. But I
also didn't want to disallow it because it might be inherent to the way a
provider is implemented (e.g. they forward the deletion call to some underlying
data model that is not under our control and that data model emits an event). Of
course the provider could temporarily suppress the event, but that would look
hacky to me.

Since the UI doesn't update anyway, it's not much of a difference: Unnecessary
updates will only bubble up to the cache on the service, not further on. So
maybe we should require an update in any case?

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

I'd slightly prefer sending no update, because then the event can be interpreted
as "show a 'new content available' indicator". But I'm fine either way, as long
as we have consistency, so either all providers send and update, or none do.

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

We have to answer this question twice (for both Observers, from Providers and
towards UI). For the ContentSuggestionsService::Observer (towards the UI) I
agree with you: not sending an update is better. And for consistency, let's just
do the same here. I changed it.

+  // suggestion has been discarded.
+  virtual void DiscardSuggestion(const std::string& suggestion_id) = 0;
+
+  // 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 FetchSuggestionImage(const std::string& suggestion_id,
+                                    const ImageFetchedCallback& callback) = 0;
+
+ protected:
+  virtual ~ContentSuggestionsProvider() {}
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_