[NTPSnippets] Show all downloads on the NTP (3/3): Downloads provider.

chrome/browser/ntp_snippets/downloads/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_DOWNLOADS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_

[Marc Treib, 2016/10/11 12:07:42]

I don't think this needs a "downloads" subfolder. There isn't that much stuff in
c/b/ntp_snippets, and the bookmarks stuff also doesn't have a subfolder.

How Android-specific would you say all this is? If it is, then maybe
chrome/browser/android/ntp/ would be a better place for it. WDYT?

[vitaliii, 2016/10/13 08:43:07]

As it was discussed, this provider is not by design exclusive to android, to it
stays here. 

+#define CHROME_BROWSER_NTP_SNIPPETS_DOWNLOADS_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/all_download_item_notifier.h"
+#include "components/ntp_snippets/category.h"
+#include "components/ntp_snippets/category_factory.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/ntp_snippets/offline_pages/offline_page_proxy.h"
+#include "components/offline_pages/offline_page_item.h"

[Marc Treib, 2016/10/11 12:07:42]

I think a forward declaration is enough here

[vitaliii, 2016/10/13 08:43:07]

Done.

+
+class PrefRegistrySimple;
+class PrefService;
+
+namespace gfx {
+class Image;
+}
+
+namespace ntp_snippets {

[Marc Treib, 2016/10/11 12:07:42]

No namespace; that's used only for the component.

[vitaliii, 2016/10/13 08:43:07]

Done.

+
+// Provides download content suggestions from the offline pages model (obtaining
+// the data through OfflinePageProxy) and the download manager (obtaining the
+// data through AllDownloadItemNotifier). Offline pages related downloads are
+// referred to as offline page downloads, while the remaining downloads (e.g.
+// images, music, books) are called asset downloads.
+class DownloadSuggestionsProvider : public ContentSuggestionsProvider,
+                                    public OfflinePageProxy::Observer,
+                                    public AllDownloadItemNotifier::Observer {
+ public:
+  DownloadSuggestionsProvider(
+      ContentSuggestionsProvider::Observer* observer,
+      CategoryFactory* category_factory,
+      const scoped_refptr<OfflinePageProxy>& offline_page_proxy,
+      content::DownloadManager* download_manager,
+      PrefService* pref_service,
+      bool download_manager_ui_enabled);
+  ~DownloadSuggestionsProvider() override;
+
+  // 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 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;
+
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
+ private:
+  // OfflinePageProxy::Observer implementation.
+  void OfflinePageModelChanged(
+      const std::vector<offline_pages::OfflinePageItem>& offline_pages)
+      override;
+  void OfflinePageDeleted(int64_t offline_id,
+                          const offline_pages::ClientId& client_id) override;
+
+  // AllDownloadItemNotifier::Observer implementation.
+  void OnDownloadCreated(content::DownloadManager* manager,
+                         content::DownloadItem* item) override;
+  void OnDownloadUpdated(content::DownloadManager* manager,
+                         content::DownloadItem* item) override;
+  void OnDownloadOpened(content::DownloadManager* manager,
+                        content::DownloadItem* item) override;
+  void OnDownloadRemoved(content::DownloadManager* manager,
+                         content::DownloadItem* item) override;
+
+  // Updates the |category_status_| of the |provided_category_| and notifies the
+  // |observer_|, if necessary.
+  void NotifyStatusChanged(CategoryStatus new_status);
+
+  // Requests all offline pages and after asynchronously obtaining the result,
+  // prunes dismissed IDs and caches some most recent items, but does not notify
+  // the service.

[Marc Treib, 2016/10/11 12:07:41]

"the service" being ContentSuggestionsService? Technically the provider doesn't
know about that, it just has an observer.

[vitaliii, 2016/10/13 08:43:07]

Done.

+  void FetchOfflinePagesDownloads();
+
+  // Like |FetchOfflinePagesDownloads| above, but with
+  // |SubmitContentSuggestions| afterwards.
+  void FetchOfflinePagesDownloadsAndSubmitSuggestions();
+
+  // Retrieves all asset downloads, prunes dismissed IDs and caches some most
+  // recent items, but does not notify the service.
+  void FetchAssetsDownloads();
+
+  // Retrieves both offline page and asset downloads, updates the internal cache
+  // and notifies the service.
+  void FetchAllDownloadsAndSubmitSuggestions();
+
+  // Takes |kMaxSuggestionsCount| the most recent cached suggestions and
+  // notifies the service about them.
+  void SubmitContentSuggestions();
+
+  // Converts an OfflinePageItem to a ContentSuggestion for the
+  // |provided_category_|.
+  ContentSuggestion ConvertOfflinePage(
+      const offline_pages::OfflinePageItem& offline_page) const;
+
+  // Converts DownloadItem to a ContentSuggestion for the |provided_category_|.
+  ContentSuggestion ConvertDownloadItem(
+      const content::DownloadItem* download_item) const;
+
+  // Adds |download_item| to internal asset download cache if all of the
+  // following holds:
+  // - the download is completed;
+  // - its suggestion has not been dismissed;
+  // - there are less than |kMaxSuggestionsCount| items cached or the oldest
+  //     cached item is older than the current item (the oldest item is removed
+  //     then);
+  // - the item is not present in the cache yet.
+  // Returns |true| if the item has been added.
+  bool CacheAssetDownloadIfNeeded(const content::DownloadItem* item);
+
+  // Removes item corresponding to |suggestion_id| either from offline pages or
+  // asset download cache (depends on the suggestion). Returns |false| if there
+  // are no corresponding item is not cached.

[Marc Treib, 2016/10/11 12:07:42]

"if there are no corresponding item is not cached" doesn't parse :)

[vitaliii, 2016/10/13 08:43:07]

Done.

+  bool RemoveSuggestionFromCacheIfPresent(
+      const ContentSuggestion::ID& suggestion_id);
+
+  // Removes item corresponding to |suggestion_id| from cache and fetches all
+  // corresponding downloads to update the cache if there are any not in cache.

[vitaliii, 2016/10/13 08:43:07]

Can you parse this one?

+  void RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(
+      const ContentSuggestion::ID& suggestion_id);
+
+  // Processes a list of all offline pages available at the moment pruning
+  // dismissed IDs and updating internal cache.
+  void ProcessAllOfflinePages(
+      const std::vector<offline_pages::OfflinePageItem>& all_offline_pages);
+
+  // Fires the |OnSuggestionInvalidated| event for the suggestion corresponding
+  // to the given |id_in_category| and clears it from the dismissed IDs list, if
+  // necessary.
+  void InvalidateSuggestion(const std::string& id_in_category);
+
+  // Reads dismissed IDs related to asset downloads from Prefs.

[Marc Treib, 2016/10/11 12:07:41]

nit: prefs, not capitalized. Also below.

[vitaliii, 2016/10/13 08:43:07]

Done.

+  std::set<std::string> ReadAssetDismissedIDsFromPrefs() const;
+
+  // Writes |dismissed_ids| into Prefs for asset downloads.
+  void StoreAssetDismissedIDsToPrefs(
+      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);
+
+  // Reads from Prefs dismissed IDs related to either offline page or asset
+  // downloads (depending on |suggestion_id|).
+  std::set<std::string> ReadDismissedIDsFromPrefs(
+      const ContentSuggestion::ID& suggestion_id) const;
+
+  // Writes |dismissed_ids| into Prefs for either offline page or asset
+  // downloads (depending on |suggestion_id|).
+  void StoreDismissedIDsToPrefs(const ContentSuggestion::ID& suggestion_id,
+                                const std::set<std::string>& dismissed_ids);
+
+  CategoryStatus category_status_;
+  const Category provided_category_;
+  scoped_refptr<OfflinePageProxy> offline_page_proxy_;
+  AllDownloadItemNotifier download_manager_notifier_;
+
+  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|.

[Marc Treib, 2016/10/11 12:07:42]

nit: For the last sentence, I'd just say something like "Contains up to
|kMaxSuggestionsCount| items" (or integrate that into one of the previous
sentences)

[vitaliii, 2016/10/13 08:43:07]

But the suggested sentence does not say that there must be
|kMaxSuggestionsCount| items cached if the model has at least
|kMaxSuggestionsCount| items.

[Marc Treib, 2016/10/13 12:11:25]

...eh, fair enough.

[vitaliii, 2016/10/15 18:36:30]

Acknowledged.

+  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.
+  bool download_manager_ui_enabled_;

[Marc Treib, 2016/10/11 12:07:41]

nit: const?

[vitaliii, 2016/10/13 08:43:07]

Done.

+
+  base::WeakPtrFactory<DownloadSuggestionsProvider> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadSuggestionsProvider);
+};
+
+}  // namespace ntp_snippets
+
+#endif  // CHROME_BROWSER_NTP_SNIPPETS_DOWNLOADS_DOWNLOAD_SUGGESTIONS_PROVIDER_H_