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

chrome/browser/ntp_snippets/download_suggestions_provider.h

Index: chrome/browser/ntp_snippets/download_suggestions_provider.h
diff --git a/chrome/browser/ntp_snippets/download_suggestions_provider.h b/chrome/browser/ntp_snippets/download_suggestions_provider.h
index 5ee5baf25853edef5678104e8f3436945cf55fd4..2265519064146c697105c80a1084360c8fa23867 100644
--- a/chrome/browser/ntp_snippets/download_suggestions_provider.h
+++ b/chrome/browser/ntp_snippets/download_suggestions_provider.h
@@ -12,6 +12,7 @@
 #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"
@@ -37,12 +38,14 @@ 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;
@@ -73,8 +76,6 @@ class DownloadSuggestionsProvider
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
-  static int GetMaxDismissedCountForTesting();
-
  private:
   friend class DownloadSuggestionsProviderTest;
 
@@ -101,6 +102,10 @@ class DownloadSuggestionsProvider
   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);
@@ -171,11 +176,11 @@ class DownloadSuggestionsProvider
   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;
@@ -184,15 +189,15 @@ class DownloadSuggestionsProvider
   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();
 
@@ -200,6 +205,7 @@ class DownloadSuggestionsProvider
   const ntp_snippets::Category provided_category_;
   offline_pages::OfflinePageModel* offline_page_model_;
   content::DownloadManager* download_manager_;
+  DownloadHistory* download_history_;
 
   PrefService* pref_service_;
 
@@ -220,6 +226,8 @@ class DownloadSuggestionsProvider
   // 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);