Add ContentSuggestionsService

components/ntp_snippets/content_suggestions_service.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_SERVICE_H_
+#define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_
+
+#include <stddef.h>
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#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;
+}
+
+namespace ntp_snippets {
+
+// Retrieves suggestions from a number of ContentSuggestionsProviders and serves
+// them grouped into categories.
+// NOTE: This class is not yet in use, please use NTPSnippetService for now
+// (see ntp_snippet_service.h).

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

nit: NTPSnippetsService and ntp_snippets_service.h (plural)

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

Done.

+class ContentSuggestionsService : public KeyedService,
+                                  public ContentSuggestionsProvider::Observer {
+ public:
+  using ImageFetchedCallback =
+      base::Callback<void(const std::string& suggestion_id, const gfx::Image&)>;
+
+  class Observer {
+   public:
+    // Fired every time the service loads a new set of data. The new data is
+    // then available through the getters of the service.
+    virtual void OnSuggestionsChanged() = 0;
+
+    // Sent when the service is shutting down. After the service has shut down,
+    // the getters and other methods should not be used anymore.

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

getters of the service?
Is it safe to say, that the service shouldn't be used anymore?
Would be good to get those semantics as simple as possible.
How do we actually make sure all parts in the system know that the service
singleton must not be touched anymore?

Further thought: Is there a chance we can get rid of the shutdown semantics at
all? [happy to discuss this offline]

[Marc Treib, 2016/07/01 09:51:52]

Probably not; these are a fixed part of the KeyedService system.
That said, if the inter-service dependencies are properly set (i.e. all
providers depend on the service), then all providers will be shut down before
the service itself is shut down, so there's no reason to have the
ContentSuggestionsServiceShutdown method. However, that requires all providers
to be KeyedServices...

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

We probably should not require providers to be KeyedServices because that would
in many cases require an additional KeyedService even though there is another
one right behind that (e.g. for offline pages). But we can require that every
provider is owned by a KeyedService (either itself or the one behind) and that
one will depend on the ContentSuggestionsService. So if the source (like the
offline pages model) doesn't want to depend on the content suggestions, we need
to set up a third KeyedService, otherwise we're fine with two.

I would remove this event completely. There is no harm in calling the service
after it has shut down. It will probably at some point be replaced with an
OnStateChanged() event.

+    // TODO(pke) Introduce proper, explicit state of the service and make calls
+    // to methods after shutdown fail.
+    virtual void ContentSuggestionsServiceShutdown() = 0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  enum Enabled : bool {

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

I'd call the enum itself State or something like that.
Also maybe enum class, since you seem to be using it in the "scoped" style
anyway?

+    Enable = true,

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

shouldn't these follow the constant naming convention, i.e. kEnabled?
(https://google.github.io/styleguide/cppguide.html#Enumerator_Names)

[Marc Treib, 2016/07/01 09:51:52]

In Chrome, MACRO_STYLE is officially preferred
(https://www.chromium.org/developers/coding-style), but both styles are used in
practice.

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

Done.

+    Disable = false,
+  };
+
+  ContentSuggestionsService(Enabled enabled);
+  ~ContentSuggestionsService() override;
+
+  // Inherited from KeyedService.
+  void Shutdown() override;
+
+  // Getters for available categories and suggestions in these categories
+  const std::vector<ContentSuggestionCategory>& GetCategories() const {
+    return categories_;
+  }
+  const std::vector<ContentSuggestion>& GetSuggestionsForCategory(
+      ContentSuggestionCategory category) const;
+
+  // 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 em// NOTE: This class is not yet in use, please use
+  // NTPSnippet for now
+  // (see ntp_snippet.h).pty image.

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

Copy&paste fail?

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

Done. I need to turn off middle-click paste..

+  void FetchSuggestionImage(const std::string& suggestion_id,
+                            const ImageFetchedCallback& callback);
+
+  // Discards the suggestion with the given |suggestion_id|, if it exists.
+  // This won't necessarily trigger an update through the observers.
+  void DiscardSuggestion(const std::string& suggestion_id);
+
+  // Observer accessors.
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  // Registers a new ContentSuggestionsProvider. This method will fail if 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*>&
+  GetProvidersForDebugging() const {
+    return providers_;
+  }
+
+  // Only for debugging use through the internals page.
+  // 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 ClearCachedSuggestionsForDebugging();
+
+  // 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 ClearDiscardedSuggestionsForDebugging();
+
+ private:
+  // Implementation of ContentSuggestionsProvider::Observer.
+  void OnSuggestionsChanged(
+      ContentSuggestionsProviderType provider_type,
+      ContentSuggestionCategory changed_category,
+      std::vector<ContentSuggestion> suggestions) override;
+
+  void OnProviderShutdown(
+      ContentSuggestionsProviderType provider_type) override;
+
+  // When |enabled_| is true the service will retrieve suggestions from the
+  // providers.
+  Enabled enabled_;
+
+  // All current suggestion categories, in order.
+  std::vector<ContentSuggestionCategory> categories_;
+  // All current suggestions grouped by category.
+  std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>
+      suggestions_by_category_;
+
+  std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>
+      providers_;
+
+  base::ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_