Create ReadingListSuggestionsProvider

components/ntp_snippets/ios/reading_list_suggestions_provider.h

+// Copyright 2017 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_IOS_READING_LIST_SUGGESTIONS_PROVIDER_H_
+#define COMPONENTS_NTP_SNIPPETS_IOS_READING_LIST_SUGGESTIONS_PROVIDER_H_
+
+#include <set>
+#include <string>
+
+#include "components/ntp_snippets/callbacks.h"
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_info.h"
+#include "components/ntp_snippets/category_status.h"
+#include "components/ntp_snippets/content_suggestion.h"
+#include "components/ntp_snippets/content_suggestions_provider.h"
+#include "components/reading_list/ios/reading_list_model_observer.h"
+
+class ReadingListModel;
+
+namespace ntp_snippets {
+
+// Provides content suggestions from the Reading List.
+class ReadingListSuggestionsProvider : public ContentSuggestionsProvider,
+                                       public ReadingListModelObserver {

[tschumann, 2017/03/20 08:58:13]

I wonder if we can avoid having the provider be too many roles.
(just reading the constructor already requires looking up multiple interfaces to
figure out what roles 'this' is playing where).

Inheritance is quite a strong coupling; wouldn't aggregation suffice for
listening to changes on the readinglist model?

I wonder if we would make things easier to encapsulate the readinglist handling
in a separate class (can be fully implemented in the .cc file) on which the
provider registers a simple callback that already get's the updated data to push
along. 
Looking at the ReadingListModelObserver interface, there's a good chance that
additional methods will be implemented which would unnecessarily increase the
provider's complexity.

[gambard, 2017/03/23 15:45:52]

I planned to have the support of ReadingListModelObserver kept minimal (i.e.
observing only a few events, not all of them) and the events would probably only
trigger a notification to the service (i.e. no internal logic inside the
provider).
I fear that the separate class handling the ReadingList observation would only
send a notification to the provider which would then directly forward it to the
ContentSuggestionsService.

I think it is OK to keep it this way for now, then if the provider evolves in
something more complex we should do some inheritance.

+ public:
+  ReadingListSuggestionsProvider(ContentSuggestionsProvider::Observer* observer,
+                                 ReadingListModel* reading_list_model);
+  ~ReadingListSuggestionsProvider() override;
+
+ private:
+  // ContentSuggestionsProvider implementation.

[tschumann, 2017/03/20 08:58:13]

there was some discussion about artificially (and ineffectively) hiding methods
from interfaces like this.
IIRC, we agreed to not do that anymore and just have them public. 

[Marc Treib, 2017/03/20 09:55:26]

It depends - things that aren't conceptually part of the public interface should
IMO be "hidden" like this. (Of course, per your other comment, that kind of
thing is usually not a good sign.)

Here, these methods *are* part of the public interface though, so they should
indeed be public.

[tschumann, 2017/03/20 09:59:58]

The problem is that you cannot effecively hide it like that ;-) Given that we
have consensus here on this particular case, no need to derail the thread
further. Also, there's more discussion documented already here :-)
https://groups.google.com/a/google.com/d/msg/c-style/ThZwK66DbOQ/mKAWxCoAEAAJ

[gambard, 2017/03/23 15:45:52]

I set it public.

+  CategoryStatus GetCategoryStatus(Category category) override;
+  CategoryInfo GetCategoryInfo(Category category) override;
+  void DismissSuggestion(const ContentSuggestion::ID& suggestion_id) override;
+  void FetchSuggestionImage(const ContentSuggestion::ID& suggestion_id,
+                            const ImageFetchedCallback& callback) override;
+  void Fetch(const Category& category,
+             const std::set<std::string>& known_suggestion_ids,
+             const FetchDoneCallback& callback) override;
+  void ClearHistory(
+      base::Time begin,
+      base::Time end,
+      const base::Callback<bool(const GURL& url)>& filter) override;
+  void ClearCachedSuggestions(Category category) override;
+  void GetDismissedSuggestionsForDebugging(
+      Category category,
+      const DismissedSuggestionsCallback& callback) override;
+  void ClearDismissedSuggestionsForDebugging(Category category) override;
+
+  // ReadingListModelObserver implementation.
+  void ReadingListModelLoaded(const ReadingListModel* model) override;
+
+  void FetchReadingListInternal();
+
+  CategoryStatus category_status_;
+  const Category provided_category_;
+
+  ReadingListModel* reading_list_model_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReadingListSuggestionsProvider);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // COMPONENTS_NTP_SNIPPETS_IOS_READING_LIST_SUGGESTIONS_PROVIDER_H_