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.

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

Add a "not used yet" comment, similar to ContentSuggestion itself?

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

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;

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

The service interface is taking on a lot of roles.
Would clients ever access the data without being notified through the observer?
If not, I wonder if it wouldn't be cleaner to factor out an data access
interface and hand this into the Observer methods to properly decouple the two
concerns the service is doing (data access + registry). 
Also it seems a bit weird that the observer needs to get access to the service
from somewhere else when it only wants to react on observed events.

[Marc Treib, 2016/06/30 11:34:23]

Hm. I'm not convinced that's worth the extra complexity at this point (but not
really opposed either).
That's fairly common: The service is more or less a singleton (per-profile), you
access it through the factory. And you need the service anyway to register
yourself as an observer.

[tschumann, 2016/06/30 11:58:11]

While that's true, I think it would be nice if the implementation detail that
this is a singleton wouldn't ripple through the layers. It's kind of a dirty  if
you get access to Singletons from anywhere. The context is much nicer to grasp
if a function gets the state from the framework it needs to work on (the
framework <-> API tradeoff. I personally try to minimize framework logic and
move it into easy to use, clean APIs).
Especially functionality like "service shutdown" impose a certain state on the
service -- users of the framework can't use the service after shutdown anymore
(I guess). That's another reason why IMO accessing the service through the
singleton should be minimized. If my observer is unregistered, and only gets
access to the service through the observer functions, it won't get called
anymore and there's no risk of accessing the service in a non-standard state.

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

The observer here is the UI, really. The UI needs access to the service even
outside of calls that came through the observer interface (for the
FetchSuggestionImage and DiscardSuggestion methods). FetchSuggestionImage to me
is also part of the data access API, but even if it's not: As soon as we build a
"Refresh this section" button or infinite scroll or anything like that, the UI
will need direct access to the data model, so it will need to store a pointer to
the service.

We could introduce a smaller interface for all the service methods that the UI
needs (not Shutdown, not RegisterProvider, not the debugging stuff) and have the
UI use the service through that. Currently I don't see a big advantage, but
rather the need to change method signatures in yet another place whenever
something changes (like FetchSnippetImage -> FetchImage -> FetchSuggestionImage
-> remove provider_type argument because it's now in the ID, ...). I think it
slows down code changes more than it currently helps. It should be easy to add
such an interface later on.

By the UI, the new factory will only be used once, namely in the snippets bridge
constructor.

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

+1. I see the point of Tim's suggestion, but I think at this point the extra
indirection would create more confusion than clarity. We might still decide to
add it later, but IMO we're not at that point yet.

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

Acknowledged.

+
+    // Sent when the service is shutting down.

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

what are the post conditions after this function? what's the responsibilty of
the observer?

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

Done.

+    virtual void ContentSuggestionsServiceShutdown() = 0;
+
+   protected:
+    virtual ~Observer() {}
+  };
+
+  ContentSuggestionsService(bool 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 empty image.
+  void FetchSuggestionImage(const std::string& suggestion_id,
+                            const ImageFetchedCallback& callback);
+
+  // 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 ClearCachedSuggestions();
+
+  // 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.
+  // 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*>&
+  providers() const {
+    return providers_;
+  }
+
+ private:
+  // Implementation of ContentSuggestionsProvider::Observer.
+  void OnSuggestionsChanged(
+      ContentSuggestionsProviderType provider_type,
+      ContentSuggestionCategory changed_category,
+      std::vector<ContentSuggestion> suggestions) override;
+
+  void OnProviderShutdown(

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

This is one of the cases where there usually isn't an empty line between
methods. (I guess Tim might disagree with this style? I kinda like having all
methods from one superclass grouped together, but I don't care enough to argue
:) )

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

Yep, this empty line has been inserted because of Tim's comment (otherwise it's
harder to see that there is another function, as the signature of the upper one
is already 4 lines). Styleguide asks to minimize the use of empty lines though.

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

I'd argue that nobody who reads this header should care about these methods.
Whatever.

+      ContentSuggestionsProviderType provider_type) override;
+
+  // When |enabled_| is true the service will retrieve suggestions from the
+  // providers.
+  bool enabled_;
+
+  // All current suggestion categories, in order.

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

what's the order?

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

That's not yet defined. It's a UX question, I guess. Currently it's simply the
order in which content for the respective categories first became available.

+  std::vector<ContentSuggestionCategory> categories_;
+  // All current suggestions grouped by category.
+  std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>
+      suggestions_by_category_;

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

Hm, maybe at this point just a vector of pairs would be simpler? Up to you

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

Acknowledged.

+
+  std::map<ContentSuggestionsProviderType, ContentSuggestionsProvider*>
+      providers_;
+
+  // The observers.

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

This is kinda "add 1 to i" :D

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

you can drop this comment -- it's not adding any value thanks to proper variable
naming :-)

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

Done.

+  base::ObserverList<Observer> observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSuggestionsService);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_SERVICE_H_