FetchMore functionality backend

components/ntp_snippets/remote/ntp_snippets_service.h

 // Copyright 2015 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_REMOTE_NTP_SNIPPETS_SERVICE_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_SERVICE_H_
 
 #include <cstddef>
 #include <map>
 #include <memory>
@@ skipping 98 matching lines @@
   // tasks will be re-scheduled even if they already exist and have the correct
   // periods.
   void RescheduleFetching(bool force);
 
   // ContentSuggestionsProvider implementation
   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 FetchMore() 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;
 
@@ skipping 13 matching lines @@
   }
 
   // Dismissed snippets, only for unit tests.
   const NTPSnippet::PtrVector& GetDismissedSnippetsForTesting(
       Category category) const {
     return categories_.find(category)->second.dismissed;
   }
 
  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);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest, StatusChanges);
   FRIEND_TEST_ALL_PREFIXES(NTPSnippetsServiceTest,
                            SuggestionsFetchedOnSignInAndSignOut);
 
   // Possible state transitions:
   //       NOT_INITED --------+
   //       /       \          |
@@ skipping 39 matching lines @@
   // image_fetcher::ImageFetcherDelegate implementation.
   void OnImageDataFetched(const std::string& id_within_category,
                           const std::string& image_data) override;
 
   // Callbacks for the NTPSnippetsDatabase.
   void OnDatabaseLoaded(NTPSnippet::PtrVector snippets);
   void OnDatabaseError();
 
   // 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|.
   // It also deletes the snippets from the DB and keeps the archive reasonably
   // 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();
 
   // Removes images from the DB that are not referenced from any known snippet.
   // Needs to iterate the whole snippet database -- so do it often enough to
   // keep it small but not too often as it still iterates over the file system.
   void ClearOrphanedImages();
 
   // Clears all stored snippets and updates the observer.
@@ skipping 46 matching lines @@
 
   // Updates the internal status for |category| to |category_status_| and
   // notifies the content suggestions observer if it changed.
   void UpdateCategoryStatus(Category category, CategoryStatus status);
   // Calls UpdateCategoryStatus() for all provided categories.
   void UpdateAllCategoryStatus(CategoryStatus status);
 
   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_;
 
   // TODO(sfiera): Reduce duplication of CategoryContent with CategoryInfo.
   struct CategoryContent {
     CategoryStatus status = CategoryStatus::INITIALIZING;
 
     // The title of the section, localized to the running UI language.
     base::string16 localized_title;
@@ skipping 59 matching lines @@
 
   // Request throttler for limiting requests to thumbnail images.
   RequestThrottler thumbnail_requests_throttler_;
 
   DISALLOW_COPY_AND_ASSIGN(NTPSnippetsService);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_NTP_SNIPPETS_SERVICE_H_