FetchMore functionality backend

components/ntp_snippets/remote/ntp_snippets_service.h

Index: components/ntp_snippets/remote/ntp_snippets_service.h
diff --git a/components/ntp_snippets/remote/ntp_snippets_service.h b/components/ntp_snippets/remote/ntp_snippets_service.h
index af9c017bafbe1f0903c0e174b8afc768348eedfa..e9dffef8188b5f2126a33602eb62bf6f3fcec2e3 100644
--- a/components/ntp_snippets/remote/ntp_snippets_service.h
+++ b/components/ntp_snippets/remote/ntp_snippets_service.h
@@ -116,6 +116,7 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
   void DismissSuggestion(const ContentSuggestion::ID& suggestion_id) override;
   void FetchSuggestionImage(const ContentSuggestion::ID& suggestion_id,
                             const ImageFetchedCallback& callback) override;
+  void FetchMore() override;
   void ClearHistory(
       base::Time begin,
       base::Time end,
@@ -149,6 +150,10 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
 
  private:
   friend class NTPSnippetsServiceTest;
+
+  // Forward declaration of the Strategy used to process fetch requests.
+  class NewNTPSnippetsHandlingStrategy;
+
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest,
                            RemoveExpiredDismissedContent);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest, RescheduleOnStateChange);
@@ -208,6 +213,7 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
 
   // Callback for the NTPSnippetsFetcher.
   void OnFetchFinished(
+      std::unique_ptr<NewNTPSnippetsHandlingStrategy> strategy,
       NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories);
 
   // Moves all snippets from |to_archive| into the archive of the |category|.
@@ -215,8 +221,11 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
   // short.
   void ArchiveSnippets(Category category, NTPSnippet::PtrVector* to_archive);
 
-  // Replace old snippets in |category| by newly available snippets.
-  void ReplaceSnippets(Category category, NTPSnippet::PtrVector new_snippets);
+  // Add newly available snippets in |category| to the provided content.
+  // If |replace_snippets| is set, archive old snippets.
+  void StoreSnippets(const Category& category,
+                     NTPSnippet::PtrVector new_snippets,
+                     NewNTPSnippetsHandlingStrategy* strategy);
 
   // Removes expired dismissed snippets from the service and the database.
   void ClearExpiredDismissedSnippets();
@@ -283,8 +292,68 @@ class NTPSnippetsService final : public ContentSuggestionsProvider,
   void RestoreCategoriesFromPrefs();
   void StoreCategoriesToPrefs();
 
+  // Implementation for |FetchSnippets| and |FetchMore| that calls the snippet
+  // fetcher and replaces or adds the fetched snippets depending on the passed
+  // |strategy|.
+  void FetchSnippetsFromHostsImpl(
+      const std::set<std::string>& hosts,
+      bool interactive_request,
+      std::unique_ptr<NewNTPSnippetsHandlingStrategy> strategy);
+
+  void MarkEmptyCategoriesAsLoading();
+
+  void AssignExpiryDates(NTPSnippet::PtrVector* snippets);
+
+  void AddIncompleteSnippets(NTPSnippet::PtrVector* snippets);
+
   State state_;
 
+  // TODO(fhorschig): Consider moving this into a separate File.
+  // This class implements a Strategy pattern to determine the handling of new
+  // snippets.
+  // As friend of NTPSnippetService, it calls private methods to handle the
+  // persistence of snippets, invoking correct notification for observers, etc.
+  class NewNTPSnippetsHandlingStrategy {
+   public:
+    virtual void NotifySuggestions() = 0;
+    virtual void SaveNewSnippets(const Category& category,
+                                 NTPSnippet::PtrVector new_snippets) = 0;
+    virtual void CollectIdsToExclude(std::set<std::string>* ids) = 0;
+
+   protected:
+    explicit NewNTPSnippetsHandlingStrategy(NTPSnippetsService* service);
+
+    NTPSnippetsService* service_;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(NewNTPSnippetsHandlingStrategy);
+  };
+  friend class NewNTPSnippetsHandlingStrategy;

[Marc Treib, 2016/10/18 08:17:45]

Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown too
big, and we should consider splitting it into multiple smaller classes at some
point. But IMO this doesn't really help - consider:
- Given that it's a friend and calls private methods, it doesn't provide any
encapsulation. It's not possible to read (and understand) the code for either
the service or the strategy by itself.
- IMO it makes the code harder to read, because the logic is more scattered - to
understand what Service::StoreSnippets does, you'll have to figure out what
strategy you currently have, and then have to jump back and forth between
Service and Strategy.

The alternative would be to keep all the logic inline, and pass around a "bool
is_more_request". I think that'd be simpler overall - WDYT?

[fhorschig, 2016/10/20 13:10:38]

I absolutely agree with the private methods and I removed the strategy for now
as it depended on too much private members.
I personally consider inheritance much more understandable than a high nesting
depth which worsens with an additional bool parameter.

Currently, the logic is inlined, which forced me to pass a total of 7 parameters
to NTPSnippetsFetcher::FetchSnippetsFromHost. I would propose to introduce a
data object that handles the request parameters, the callback and would function
as delegate for the snippets_fetcher_. This would require logic to move out of
both classes ... What is your opinion about this approach?

[Marc Treib, 2016/10/20 16:51:39]

I guess that might work... it's hard to judge without looking into it in detail.
In any case, I'd suggest doing restructurings like that in a separate CL (before
or after this one, whichever is easier).

[tschumann, 2016/10/24 06:38:25]

What about introducing a NTPSnippetsFetcher::Parameters struct which we could
simply populate here?  This should help with the many parameters.
For the strategy, I'd introduce and enum first and implement the logic inside
the service and then have a look what a good abstraction would be. As Marc
pointed out, we need to introduce an additional abstraction and I'd rather see
another layer on top of the ntp snippets database which hides away snippets
archival and could also do replacements and extensions of categories.

[Marc Treib, 2016/10/28 14:49:48]

NTPSnippetsFetcher::FetchSnippets now takes a Params struct
(https://codereview.chromium.org/2454063002/)

+
+  // This class handles fetch requests that require all existing snippets to be
+  // replaced by newly fetched ones.
+  // It invokes the |OnNewSuggestions()| notification for observers.
+  class ReplaceExistingSnippets : public NewNTPSnippetsHandlingStrategy {
+   public:
+    explicit ReplaceExistingSnippets(NTPSnippetsService* service);
+    void CollectIdsToExclude(std::set<std::string>* ids) override;
+    void SaveNewSnippets(const Category& category,
+                         NTPSnippet::PtrVector new_snippets) override;
+    void NotifySuggestions() override;
+  };
+
+  // This class handles fetch requests that add more snippets.
+  // It invokes the |OnMoreSuggestions()| notification for observers.
+  class AddNewSnippets : public NewNTPSnippetsHandlingStrategy{
+   public:
+    explicit AddNewSnippets(NTPSnippetsService* service);
+
+    void CollectIdsToExclude(std::set<std::string>* ids) override;
+    void SaveNewSnippets(const Category& category,
+                         NTPSnippet::PtrVector new_snippets) override;
+    void NotifySuggestions() override;
+  };
+
   PrefService* pref_service_;
 
   const Category articles_category_;