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..7b2765658e10776e0b6feaff12a73567063a587b
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_provider.h
@@ -0,0 +1,98 @@
+// 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(
+        ContentSuggestionsProviderType provider_type,
+        ContentSuggestionCategory changed_category,
+        std::vector<ContentSuggestion> suggestions) = 0;
+
+    // Called when the provider needs to shut down and will not deliver any
+    // suggestions anymore. This unregisters the provider, removes the observer

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

"this unregisters the provider"
I'm a bit confused what 'this' means. If it's meaning the OnProviderShutdown
function, then it's something that implementations need to take care of -- that
would be good to explicitly state (implementations need to makes sure to...).
If it's meaning is the shut down process, then please state if the provider gets
unregistered before or after this function (although I'm not sure where and how
that would be done).
Same for the rest of the comment.

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

Implementations need to take care of that -- but the only implementation of this
interface is the ContentSuggestionsService itself. Since it's part of the
Provider interface, the doc talks to the provider and explains what a provider
can expect/achieve when calling this method.

+    // and immediately removes all suggestions of that provider from all caches
+    // and from the UI.
+    virtual void OnProviderShutdown(
+        ContentSuggestionsProviderType provider_type) = 0;
+  };
+
+  // Accessor for observer, which is notified about new available suggestions.

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

nit: Now it's just a setter. So maybe rephrase to "Sets an observer which is
notified ..".

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

Done.

+  virtual void SetObserver(Observer* observer) = 0;
+
+  // Discards the suggestion with the given ID. A provider needs to ensure that
+  // a once-discarded suggestion is never delivered again (through the
+  // Observer). There is no need to call the observer for an update after a
+  // suggestion has been discarded.
+  // Note that the |suggestion_id| to the provider here is the same as the
+  // |original_id| that was passed to the |ContentSuggestion| constructor.
+  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.
+  // Note that the |suggestion_id| to the provider here is the same as the
+  // |original_id| that was passed to the |ContentSuggestion| constructor.
+  virtual void FetchSuggestionImage(const std::string& suggestion_id,
+                                    const ImageFetchedCallback& callback) = 0;
+
+  // Used only for debugging purposes. Clears all caches so that the next
+  // fetch starts from scratch.
+  virtual void ClearCachedSuggestionsForDebugging() = 0;
+
+  // 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 ClearDiscardedSuggestionsForDebugging() = 0;
+
+  // Returns the type of the provider.
+  ContentSuggestionsProviderType GetProviderType() const {

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

Style guide says to name trivial getters in variable style, i.e. provider_type()
(yeah, I don't like it either)

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

Is this needed anywhere except in RegisterProvider()?
What code is calling RegisterProvider? could we simply pass in the type into
RegisterProvider and remove this method from the interface?


On the topic of naming getters. Actually, it's just an option to use the
lower_case variant. I'm also not a big fan of this, so we could just decide to
go with CamelCase naming.
from https://google.github.io/styleguide/cppguide.html#Function_Names

"Functions that are very cheap to call may instead follow the style for variable
names (all lower-case, with underscores between words)."

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

But we changed "categories()" to "GetCategories()" and all it does it "return
categories_"?

Btw this getter and the entire ProviderType enum would also be obsolete if there
was a category->provider_type mapping ;-)

I changed it to provider_type and removed the comment (that nobody nit'ed
about?)

@tschumann: The getter is used in quite a few places; it might also be used by
provider implementations so that they don't have to hard-code their type
throughout their code (though protected would be enough for that).

+    return provider_type_;
+  }
+
+ protected:
+  ContentSuggestionsProvider(ContentSuggestionsProviderType provider_type)
+      : provider_type_(provider_type) {}
+  virtual ~ContentSuggestionsProvider() {}
+
+ private:
+  const ContentSuggestionsProviderType provider_type_;
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_PROVIDER_H_