Refactoring: Moves image fetching and caching into CachedImageFetcher

components/ntp_snippets/remote/remote_suggestions_provider.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_REMOTE_SUGGESTIONS_PROVIDER_H_
 #define COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_
 
 #include <cstddef>
 #include <deque>
 #include <map>
@@ skipping 28 matching lines @@
 namespace image_fetcher {
 class ImageDecoder;
 class ImageFetcher;
 }  // namespace image_fetcher
 
 namespace ntp_snippets {
 
 class RemoteSuggestionsDatabase;
 class UserClassifier;
 
+// CachedImageFetcher takes care of fetching images from the network and caching
+// them in the database.
+// TODO(tschumann): Move into a separate library and inject the
+// CachedImageFetcher into the RemoteSuggestionsProvider. This allows us to get
+// rid of exposing this member for testing and lets us test the caching logic
+// separately.
+class CachedImageFetcher : public image_fetcher::ImageFetcherDelegate {
+ public:
+  // |pref_service| and |database| need to outlive the created image fetcher
+  // instance.
+  CachedImageFetcher(std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher,
+                     std::unique_ptr<image_fetcher::ImageDecoder> image_decoder,
+                     PrefService* pref_service,
+                     RemoteSuggestionsDatabase* database);
+  ~CachedImageFetcher() override;
+
+  // Fetches the image for a suggestion. The fetcher will first issue a lookup
+  // to the underlying cache with a fallback to the network.
+  void FetchSuggestionImage(const ContentSuggestion::ID& suggestion_id,
+                            const GURL& image_url,
+                            const ImageFetchedCallback& callback);
+
+ private:
+  // image_fetcher::ImageFetcherDelegate implementation.
+  void OnImageDataFetched(const std::string& id_within_category,
+                          const std::string& image_data) override;
+  void OnImageDecodingDone(const ImageFetchedCallback& callback,
+                           const std::string& id_within_category,
+                           const gfx::Image& image);
+  void OnSnippetImageFetchedFromDatabase(
+      const ImageFetchedCallback& callback,
+      const ContentSuggestion::ID& suggestion_id,
+      const GURL& image_url,
+      // SnippetImageCallback requires nonconst reference
+      std::string data);
+  void OnSnippetImageDecodedFromDatabase(
+      const ImageFetchedCallback& callback,
+      const ContentSuggestion::ID& suggestion_id,
+      const GURL& url,
+      const gfx::Image& image);
+  void FetchSnippetImageFromNetwork(const ContentSuggestion::ID& suggestion_id,
+                                    const GURL& url,
+                                    const ImageFetchedCallback& callback);
+
+  std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
+  std::unique_ptr<image_fetcher::ImageDecoder> image_decoder_;
+  RemoteSuggestionsDatabase* database_;
+  // Request throttler for limiting requests to thumbnail images.
+  RequestThrottler thumbnail_requests_throttler_;
+
+  DISALLOW_COPY_AND_ASSIGN(CachedImageFetcher);
+};
+
 // Retrieves fresh content data (articles) from the server, stores them and
 // provides them as content suggestions.
 // This class is final because it does things in its constructor which make it
 // unsafe to derive from it.
 // TODO(treib): Introduce two-phase initialization and make the class not final?
-// TODO(jkrcal): this class grows really, really large. The fact that
-// NTPSnippetService also implements ImageFetcherDelegate adds unnecessary
-// complexity (and after all the Service is conceptually not an
-// ImagerFetcherDeletage ;-)). Instead, the cleaner solution would  be to define
-// a CachedImageFetcher class that handles the caching aspects and looks like an
-// image fetcher to the NTPSnippetService.
-class RemoteSuggestionsProvider final
-    : public ContentSuggestionsProvider,
-      public image_fetcher::ImageFetcherDelegate {
+class RemoteSuggestionsProvider final : public ContentSuggestionsProvider {
  public:
   // |application_language_code| should be a ISO 639-1 compliant string, e.g.
   // 'en' or 'en-US'. Note that this code should only specify the language, not
   // the locale, so 'en_US' (English language with US locale) and 'en-GB_US'
   // (British English person in the US) are not language codes.
   RemoteSuggestionsProvider(
       Observer* observer,
       CategoryFactory* category_factory,
       PrefService* pref_service,
       const std::string& application_language_code,
@@ skipping 76 matching lines @@
   const NTPSnippet::PtrVector& GetDismissedSnippetsForTesting(
       Category category) const {
     return category_contents_.find(category)->second.dismissed;
   }
 
   // Overrides internal clock for testing purposes.
   void SetTickClockForTesting(std::unique_ptr<base::TickClock> tick_clock) {
     tick_clock_ = std::move(tick_clock);
   }
 
+  // TODO(tschumann): remove this method as soon as we inject the fetcher into
+  // the constructor.
+  CachedImageFetcher& GetImageFetcherForTesting() { return image_fetcher_; }
+
  private:
   friend class RemoteSuggestionsProviderTest;
 
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            DontNotifyIfNotAvailable);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            RemoveExpiredDismissedContent);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest,
                            RescheduleOnStateChange);
   FRIEND_TEST_ALL_PREFIXES(RemoteSuggestionsProviderTest, StatusChanges);
@@ skipping 79 matching lines @@
   // Requests can be marked more important by setting |interactive_request| to
   // true (such request might circumvent the daily quota for requests, etc.)
   // Useful for requests triggered by the user.
   void FetchSnippets(bool interactive_request);
 
   // Returns the URL of the image of a snippet if it is among the current or
   // among the archived snippets in the matching category. Returns an empty URL
   // otherwise.
   GURL FindSnippetImageUrl(const ContentSuggestion::ID& suggestion_id) const;
 
-  // image_fetcher::ImageFetcherDelegate implementation.
-  void OnImageDataFetched(const std::string& id_within_category,
-                          const std::string& image_data) override;
-
   // Callbacks for the RemoteSuggestionsDatabase.
   void OnDatabaseLoaded(NTPSnippet::PtrVector snippets);
   void OnDatabaseError();
 
   // Callback for fetch-more requests with the NTPSnippetsFetcher.
   void OnFetchMoreFinished(
       const FetchDoneCallback& fetching_callback,
       NTPSnippetsFetcher::FetchResult fetch_result,
       NTPSnippetsFetcher::OptionalFetchedCategories fetched_categories);
 
@@ skipping 34 matching lines @@
   // 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.
   void NukeAllSnippets();
 
   // Completes the initialization phase of the service, registering the last
   // observers. This is done after construction, once the database is loaded.
   void FinishInitialization();
 
-  void OnSnippetImageFetchedFromDatabase(
-      const ImageFetchedCallback& callback,
-      const ContentSuggestion::ID& suggestion_id,
-      std::string data);
-
-  void OnSnippetImageDecodedFromDatabase(
-      const ImageFetchedCallback& callback,
-      const ContentSuggestion::ID& suggestion_id,
-      const gfx::Image& image);
-
-  void FetchSnippetImageFromNetwork(const ContentSuggestion::ID& suggestion_id,
-                                    const ImageFetchedCallback& callback);
-
-  void OnSnippetImageDecodedFromNetwork(const ImageFetchedCallback& callback,
-                                        const std::string& id_within_category,
-                                        const gfx::Image& image);
-
   // Triggers a state transition depending on the provided status. This method
   // is called when a change is detected by |status_service_|.
   void OnStatusChanged(RemoteSuggestionsStatus old_status,
                        RemoteSuggestionsStatus new_status);
 
   // Verifies state transitions (see |State|'s documentation) and applies them.
   // Also updates the provider status. Does nothing except updating the provider
   // status if called with the current state.
   void EnterState(State state);
 
@@ skipping 43 matching lines @@
 
   // Classifier that tells us how active the user is. Not owned.
   const UserClassifier* user_classifier_;
 
   // Scheduler for fetching snippets. Not owned.
   NTPSnippetsScheduler* scheduler_;
 
   // The snippets fetcher.
   std::unique_ptr<NTPSnippetsFetcher> snippets_fetcher_;
 
-  std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_;
   std::unique_ptr<image_fetcher::ImageDecoder> image_decoder_;
 
   // The database for persisting snippets.
   std::unique_ptr<RemoteSuggestionsDatabase> database_;
+  CachedImageFetcher image_fetcher_;

[jkrcal, 2016/12/12 09:01:11]

Nit: cached_image_fetcher_? (This member name can be confusing as the
constructor receives image_fetcher but this one is quite a different type.)

[Marc Treib, 2016/12/12 10:01:03]

nit2: This is between |database_| and |database_load_start_|, which does not
seem like a good location. Move it after?

[tschumann, 2016/12/13 20:59:09]

moved it down. I'd like to push back on renaming it to cached_image fetcher, as
I'd much have the constructor take the CachedImageFetcher instead of the
image_fetcher::ImageFetcher. (IMO, we should only include type information in
variable names if we really need it and that wouldn't be the case then anymore
;-)).

[jkrcal, 2016/12/13 21:43:00]

Fine for me...

   base::TimeTicks database_load_start_;
 
   // The service that provides events and data about the signin and sync state.
   std::unique_ptr<RemoteSuggestionsStatusService> status_service_;
 
   // Set to true if FetchSnippets is called while the service isn't ready.
   // The fetch will be executed once the service enters the READY state.
   bool fetch_when_ready_;
 
   // Set to true if NukeAllSnippets is called while the service isn't ready.
   // The nuke will be executed once the service finishes initialization or
   // enters the READY state.
   bool nuke_when_initialized_;
 
-  // Request throttler for limiting requests to thumbnail images.
-  RequestThrottler thumbnail_requests_throttler_;
-
   // A clock for getting the time. This allows to inject a tick clock in tests.
   std::unique_ptr<base::TickClock> tick_clock_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteSuggestionsProvider);
 };
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_REMOTE_REMOTE_SUGGESTIONS_PROVIDER_H_