[NTP::Downloads] Fetch assets once the manager is loaded.

chrome/browser/ntp_snippets/download_suggestions_provider.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 CHROME_BROWSER_NTP_SNIPPETS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_
 #define CHROME_BROWSER_NTP_SNIPPETS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_
 
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/download/download_history.h"
 #include "components/ntp_snippets/callbacks.h"
 #include "components/ntp_snippets/category.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/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/download_manager.h"
 
 class PrefRegistrySimple;
 class PrefService;
 
 namespace offline_pages {
 struct OfflinePageItem;
 }
 
 // Provides download content suggestions from the offline pages model and the
 // download manager (obtaining the data through DownloadManager and each
 // DownloadItem). Offline page related downloads are referred to as offline page
 // downloads, while the remaining downloads (e.g. images, music, books) are
 // called asset downloads. In case either of the data sources is |nullptr|, it
 // is ignored.
 class DownloadSuggestionsProvider
     : public ntp_snippets::ContentSuggestionsProvider,
       public offline_pages::OfflinePageModel::Observer,
       public content::DownloadManager::Observer,
-      public content::DownloadItem::Observer {
+      public content::DownloadItem::Observer,
+      public DownloadHistory::Observer {

[tschumann, 2017/01/12 11:06:09]

wow, that's a massive list of interfaces to implement. 
I'd try to avoid this and use aggregation instead.
(it makes much more sense to think of the provider having these observers as
opposed to being so many observers).

What I would do instead is a wrapper class, like:

class DownloadHistoryAvailableObserver;

You can provide a fake for this to properly test it.
The DownloadSugestionsProvider can then provide a callback which gets called
when the initial query is completed and in addition ask whether it's already
done (so no need for magic, you could write it in the constructor like this):
if (history_available_observer_->IsHistoryLoaded()) {
  AsynchronouslyFetchOfflinePagesDownloads();
} else {
  history_available_observer_->NotifyWhenReader(base::Bind(this,
DownloadSuggestionsProvider::AsynchronouslyFetchOfflinePagesDownloads));
}

no extra implementations to override, no nullptr to deal with, and you can test
your provider logic.
Testing the wrapper is still tricky, but at least this is very encapsulated now.

[vitaliii, 2017/01/16 08:51:35]

I have never seen this before in Chromium.

treib@, WDYT?

[tschumann, 2017/01/16 09:14:05]

=) Well, not sure if you'd notice after the refactoring. What I definitely see
too often are very heavy classes like this one (it's implementation is huge and
getting proper test coverage is tricky).

[Marc Treib, 2017/01/16 10:33:05]

There definitely are instances of what Tim suggests, though directly
implementing observer interfaces is much more common.
Usually I'm okay with this pattern, but in this case, it is getting a bit
ridiculous, and I agree that Tim's approach would improve things. Doesn't have
to be in this CL though.

[vitaliii, 2017/01/16 14:32:39]

Acknowledged.

  public:
   DownloadSuggestionsProvider(
       ContentSuggestionsProvider::Observer* observer,
       offline_pages::OfflinePageModel* offline_page_model,
       content::DownloadManager* download_manager,
+      DownloadHistory* download_history,
       PrefService* pref_service,
       bool download_manager_ui_enabled);
   ~DownloadSuggestionsProvider() override;
 
   // ContentSuggestionsProvider implementation.
   ntp_snippets::CategoryStatus GetCategoryStatus(
       ntp_snippets::Category category) override;
   ntp_snippets::CategoryInfo GetCategoryInfo(
       ntp_snippets::Category category) override;
   void DismissSuggestion(
@@ skipping 10 matching lines @@
       const base::Callback<bool(const GURL& url)>& filter) override;
   void ClearCachedSuggestions(ntp_snippets::Category category) override;
   void GetDismissedSuggestionsForDebugging(
       ntp_snippets::Category category,
       const ntp_snippets::DismissedSuggestionsCallback& callback) override;
   void ClearDismissedSuggestionsForDebugging(
       ntp_snippets::Category category) override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  static int GetMaxDismissedCountForTesting();
-
  private:
   friend class DownloadSuggestionsProviderTest;
 
   void GetPagesMatchingQueryCallbackForGetDismissedSuggestions(
       const ntp_snippets::DismissedSuggestionsCallback& callback,
       const std::vector<offline_pages::OfflinePageItem>& offline_pages) const;
 
   // OfflinePageModel::Observer implementation.
   void OfflinePageModelLoaded(offline_pages::OfflinePageModel* model) override;
   void OfflinePageAdded(
       offline_pages::OfflinePageModel* model,
       const offline_pages::OfflinePageItem& added_page) override;
   void OfflinePageDeleted(int64_t offline_id,
                           const offline_pages::ClientId& client_id) override;
 
   // content::DownloadManager::Observer implementation.
   void OnDownloadCreated(content::DownloadManager* manager,
                          content::DownloadItem* item) override;
   void ManagerGoingDown(content::DownloadManager* manager) override;
 
   // content::DownloadItem::Observer implementation.
   void OnDownloadUpdated(content::DownloadItem* item) override;
   void OnDownloadOpened(content::DownloadItem* item) override;
   void OnDownloadRemoved(content::DownloadItem* item) override;
   void OnDownloadDestroyed(content::DownloadItem* item) override;
 
+  // DownloadHistory::Observer implementation.
+  void OnHistoryQueryComplete() override;
+  void OnDownloadHistoryDestroyed() override;
+
   // Updates the |category_status_| of the |provided_category_| and notifies the
   // |observer_|, if necessary.
   void NotifyStatusChanged(ntp_snippets::CategoryStatus new_status);
 
   // Requests all offline pages and after asynchronously obtaining the result,
   // prunes dismissed IDs and caches some most recent items. If |notify| is
   // true, notifies |ContentSuggestionsProvider::Observer| about them.
   void AsynchronouslyFetchOfflinePagesDownloads(bool notify);
 
   // Retrieves all asset downloads, prunes dismissed IDs and caches some most
@@ skipping 50 matching lines @@
       bool notify,
       const std::vector<offline_pages::OfflinePageItem>&
           all_download_offline_pages);
 
   // Fires the |OnSuggestionInvalidated| event for the suggestion corresponding
   // to the given |id_within_category| and clears it from the dismissed IDs
   // list, if necessary.
   void InvalidateSuggestion(const std::string& id_within_category);
 
   // Reads dismissed IDs related to asset downloads from prefs.
-  std::vector<std::string> ReadAssetDismissedIDsFromPrefs() const;
+  std::set<std::string> ReadAssetDismissedIDsFromPrefs() const;
 
   // Writes |dismissed_ids| into prefs for asset downloads.
   void StoreAssetDismissedIDsToPrefs(
-      const std::vector<std::string>& dismissed_ids);
+      const std::set<std::string>& dismissed_ids);
 
   // Reads dismissed IDs related to offline page downloads from prefs.
   std::set<std::string> ReadOfflinePageDismissedIDsFromPrefs() const;
 
   // Writes |dismissed_ids| into prefs for offline page downloads.
   void StoreOfflinePageDismissedIDsToPrefs(
       const std::set<std::string>& dismissed_ids);
 
-  // Adds a suggestion ID to the dismissed list in prefs, if it is not there.
-  // Works for both Offline Page and Asset downloads.
-  void AddToDismissedStorageIfNeeded(
-      const ntp_snippets::ContentSuggestion::ID& suggestion_id);
+  // Reads from prefs dismissed IDs related to either offline page or asset
+  // downloads (given by |for_offline_page_downloads|).
+  std::set<std::string> ReadDismissedIDsFromPrefs(
+      bool for_offline_page_downloads) const;
 
-  // Removes a suggestion ID from the dismissed list in prefs, if it is there.
-  // Works for both Offline Page and Asset downloads.
-  void RemoveFromDismissedStorageIfNeeded(
-      const ntp_snippets::ContentSuggestion::ID& suggestion_id);
+  // Writes |dismissed_ids| into prefs for either offline page or asset
+  // downloads (given by |for_offline_page_downloads|).
+  void StoreDismissedIDsToPrefs(bool for_offline_page_downloads,
+                                const std::set<std::string>& dismissed_ids);
 
   void UnregisterDownloadItemObservers();
 
   ntp_snippets::CategoryStatus category_status_;
   const ntp_snippets::Category provided_category_;
   offline_pages::OfflinePageModel* offline_page_model_;
   content::DownloadManager* download_manager_;
+  DownloadHistory* download_history_;
 
   PrefService* pref_service_;
 
   // Cached offline page downloads. If there are not enough asset downloads, all
   // of these could be shown (they are the most recently visited, not dismissed
   // and not invalidated). Order is undefined. If the model has less than
   // |kMaxSuggestionsCount| offline pages, then all of them which satisfy the
   // criteria above are cached, otherwise only |kMaxSuggestionsCount|.
   std::vector<offline_pages::OfflinePageItem> cached_offline_page_downloads_;
   // Cached asset downloads. If there are not enough offline page downloads, all
   // of these could be shown (they are the most recently downloaded, not
   // dismissed and not invalidated). Order is undefined. If the model has less
   // than |kMaxSuggestionsCount| asset downloads, then all of them which satisfy
   // the criteria above are cached, otherwise only |kMaxSuggestionsCount|.
   std::vector<const content::DownloadItem*> cached_asset_downloads_;
 
   // Whether the Download Manager UI is enabled, in which case the More button
   // for the Downloads section can redirect there.
   const bool download_manager_ui_enabled_;
 
+  bool is_download_manager_loaded_;
+
   base::WeakPtrFactory<DownloadSuggestionsProvider> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadSuggestionsProvider);
 };
 
 #endif  // CHROME_BROWSER_NTP_SNIPPETS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_