Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_service.h

Index: components/ntp_snippets/content_suggestions_service.h
diff --git a/components/ntp_snippets/content_suggestions_service.h b/components/ntp_snippets/content_suggestions_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..72f208177a653e5a72773cec26899e06b8c787f2
--- /dev/null
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -0,0 +1,139 @@
+// 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_SERVICE_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
+
+#include <stddef.h>
+
+#include <map>
+#include <set>
+#include <string>
+#include <vector>
+
+#include "base/observer_list.h"
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/ntp_snippets/content_suggestions_provider_type.h"
+
+namespace gfx {
+class Image;
+}

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

necessary?

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

Done.

+
+namespace ntp_snippets {
+
+class ContentSuggestionsServiceObserver;
+
+// Retrieves suggestions from a number of ContentSuggestionsProviders and serves
+// them to the NTP.
+class ContentSuggestionsService : public KeyedService,
+                                  public ContentSuggestionsProvider::Delegate {
+ public:
+  using ImageFetchedCallback =
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
+
+  ContentSuggestionsService(bool enabled);

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

nit: usually enums are better to read here.
Compare the following client code:
ContentSuggestionsService(ContentSuggestionsService::kEnabled)
ContentSuggestionsService(ContentSuggestionsService::kDisabled)
vs.
ContentSuggestionsService(true)
ContentSuggestionsService(false)

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

True, but wouldn't that question pretty much every boolean in the code? For
example, the RemoveSuggestionsOfProvider helper method below could return
"::kNothingChanged" or "::kSomethingChanged" instead of true and false. I
haven't found any other service that uses an enum or labeled constants instead
of true/false. But I have found sth like this in unit tests:
ContentSuggestionsService( /* enabled= */ false)

[tschumann, 2016/06/30 10:20:06]

It essentially applies to every boolean parameter except setters. Return values
are fine as the method name should give enough context.

The comment-at-call-side pattern works nicely if we have automated checks in
place that makes sure they match the parameter name. Otherwise, things are
easily screwed up when writing or refactoring the called method. In google3, we
have a presubmit check for this, not sure about Chromium.

As this is a constructor, I'd be in favor of documenting the semantics  nicely
with the type system (if we're indeed lacking the comment-checks in chromium).

[Philipp Keck, 2016/06/30 10:41:53]

Done. Unlike your code example above, I used an actual enum, not just two static
fields. I'm not 100% happy with the naming of that enum, but it follows the
convention of naming the enum after the property.

+  ~ContentSuggestionsService() override;
+
+  // Inherited from KeyedService.
+  void Shutdown() override;
+
+  // Available suggestions
+  const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>&
+  suggestions() const {
+    return suggestions_;
+  }
+  const std::vector<ContentSuggestion>& suggestions(
+      ContentSuggestionCategory category) {
+    return suggestions_[category];
+  }
+
+  // Fetches the image for the suggestion with the given |suggestion_id| and
+  // runs the |callback|. If that suggestion doesn't exist or the fetch fails,
+  // the callback gets an empty image.
+  void FetchImage(const ContentSuggestionsProviderType provider_type,
+                  const std::string& suggestion_id,
+                  const ImageFetchedCallback& callback);
+
+  // Removes all suggestions from all caches or internal stores in all
+  // providers. It does, however, not remove any suggestions from the provider's
+  // sources, so if their configuration hasn't changed, they should return the
+  // same results when they fetch the next time. In particular, calling this
+  // method will not mark any suggestions as discarded.
+  void ClearSuggestions();
+
+  // Only for debugging use through the internals page. Some providers
+  // internally store a list of discarded suggestions to prevent them from
+  // reappearing. This function clears all such lists in all providers, making
+  // discarded suggestions reappear (only for certain providers).
+  void ClearDiscardedSuggestions();
+
+  // Discards the suggestion with the given |suggestion_id|, if it exists.
+  void Discard(const ContentSuggestionsProviderType provider_type,
+               const std::string& suggestion_id);
+
+  // Observer accessors (outgoing observers, from this service towards UI)
+  void AddObserver(ContentSuggestionsServiceObserver* observer);
+  void RemoveObserver(ContentSuggestionsServiceObserver* observer);
+
+  // Implementation of ContentSuggestionsProvider::Delegate (incoming data)
+  void OnSuggestionsChanged(
+      const ContentSuggestionsProvider& source,
+      ContentSuggestionCategory changed_category,
+      std::vector<ContentSuggestion> suggestions) override;
+  void OnProviderShutdown(const ContentSuggestionsProvider& source) override;

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

nit: please insert an empty line between methods.

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

Ok. What are the criteria for doing that? Because AddObserver/RemoveObserver,
getters/setters, constructors/destructors are usually visually grouped by not
having an empty line between them.

The styleguide doesn't seem to specify anything, but the example does not
contain blank lines:
https://google.github.io/styleguide/cppguide.html#Class_Format

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

AFAIK there aren't any hard rules. Some people prefer one way, others another :)

[tschumann, 2016/06/30 10:20:06]

Good point!
I agree that in the AddObserver/RemoveObserver case an empty line would not be
necessary.
In this case however, I find it pretty hard to parse and actually see the second
method. Then again, it's simply implementing the Delegate interface and there's
nothing of particular interest to the reader. 
I need to think a bit more about a good rule of thumb...

One point worth mentioning is, that these less interesting details actually
muddy  the public interface of ContentSuggestionService -- they should only be
called through the Delegate interface. For now, maybe move them to the end of
the public section to keep the public interface together.

Maybe we can actually move this onto a private nested class or into the .cc
file.

[Philipp Keck, 2016/06/30 10:41:53]

This is already resolved, see 
https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con...
and
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets...

+
+  // Registers a new ContentSuggestionsProvider. The service will call
+  // ContentSuggestionsProvider::setDelegate with a delegate object, through
+  // which the registered provider should report its new suggestions. This
+  // method will fail when a provider of the same type is already registered.
+  void RegisterProvider(ContentSuggestionsProvider* provider);
+
+  // For use by the snippets-internals page only.
+  const std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>&
+  providers() const {
+    return providers_;
+  }
+
+ private:
+  // When |enabled_| is true the service will retrieve suggestions from the
+  // providers.
+  bool enabled_;
+
+  // All current suggestions.
+  std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>
+      suggestions_;
+
+  std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>
+      providers_;
+
+  // The observers.
+  base::ObserverList<ContentSuggestionsServiceObserver> observers_;
+
+  // Helper method to remove all suggestions of the given |provider_type| from
+  // the given |list|. Returns true if anything changed.
+  bool RemoveSuggestionsOfProvider(

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

shouldn't functions go before members? (declaration order).

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

Acknowledged, this member was removed anyway.

+      std::vector<ContentSuggestion>* list,
+      ContentSuggestionsProviderType provider_type);
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
+};
+
+class ContentSuggestionsServiceObserver {
+ public:
+  // Fired every time the service loads a new set of data. This event is fired
+  // per |changed_category|. The new data is then available through
+  // |suggestions()[changed_category]|.
+  virtual void OnSuggestionsChanged(
+      ContentSuggestionCategory changed_category) = 0;
+  // Sent when the service is shutting down.

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

please insert empty line.

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

Done.

+  virtual void ContentSuggestionsServiceShutdown() = 0;
+
+ protected:
+  virtual ~ContentSuggestionsServiceObserver() {}
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_