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

chrome/browser/ntp_snippets/downloads/download_suggestions_provider.cc

+// 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.
+
+#include "chrome/browser/ntp_snippets/downloads/download_suggestions_provider.h"
+
+#include <algorithm>
+
+#include "base/bind.h"
+#include "base/guid.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "components/ntp_snippets/pref_names.h"
+#include "components/ntp_snippets/pref_util.h"
+#include "components/offline_pages/client_namespace_constants.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/pref_service.h"
+#include "grit/components_strings.h"
+#include "net/base/filename_util.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/image/image.h"
+
+using content::DownloadItem;
+using content::DownloadManager;
+using offline_pages::OfflinePageItem;
+
+namespace ntp_snippets {
+
+namespace {
+
+const int kMaxSuggestionsCount = 5;

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

This should probably be configurable via a variation param - see e.g. the
bookmarks provider for an example. (Can happen in another CL, but maybe add a
TODO)

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

Done.

+const char kAssetDownloadsPrefix[] = "D";
+const char kOfflinePageDownloadsPrefix[] = "O";

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

Just make these two chars (not array)

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

Done.

+
+std::string GetOfflinePagePerCategoryID(int64_t raw_offline_page_id) {
+  // Raw ID is prefixed in order to avoid conflicts with asset downloads.
+  return base::JoinString(
+      {kOfflinePageDownloadsPrefix, base::IntToString(raw_offline_page_id)},
+      "");

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

s/""/std::string()/

But if you're just appending two strings, might as well just do "string1 +
string2".

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

Done.

+}
+
+std::string GetAssetDownloadPerCategoryID(uint32_t raw_download_id) {
+  // Raw ID is prefixed in order to avoid conflicts with offline page downloads.
+  return base::JoinString(
+      {kAssetDownloadsPrefix, base::UintToString(raw_download_id)}, "");

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

Same here

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

Done.

+}
+
+// Determines whether |suggestion_id| corresponds to offline page suggestion or
+// asset download based on |id_within_category| prefix.
+bool IsOfflinePageID(const ContentSuggestion::ID& suggestion_id) {

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

nit: "OfflinePageID" is somewhat overloaded. Maybe "CorrespondsToOfflinePage"?

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

Done.

+  const std::string& id_within_category = suggestion_id.id_within_category();
+  if (!id_within_category.empty()) {
+    if (id_within_category[0] == kOfflinePageDownloadsPrefix[0])
+      return true;
+    if (id_within_category[0] == kAssetDownloadsPrefix[0])
+      return false;
+  }
+  NOTREACHED() << "Unknown id_within_category " << id_within_category;
+  return false;
+}
+
+struct OrderOfflinePagesByMostRecentlyVisitedFirst {
+  bool operator()(const OfflinePageItem* left,
+                  const OfflinePageItem* right) const {
+    return left->last_access_time > right->last_access_time;
+  }
+};
+
+bool IsOfflinePageDownload(const offline_pages::ClientId& client_id) {
+  return (client_id.name_space == offline_pages::kAsyncNamespace ||
+          client_id.name_space == offline_pages::kDownloadNamespace ||
+          client_id.name_space == offline_pages::kNTPSuggestionsNamespace) &&

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

What's kNTPSuggestionsNamespace?

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

This is for offlined NTP items, e.g. right click on an NTP item and "Save linked
page for later".

+         base::IsValidGUID(client_id.id);

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

Is this needed? Would we ever expect to get an invalid GUID here?

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

It was done like that previously (and the logic was taken from
DownloadUIAdapter::IsVisibleInUI).
I will replace with DCHECK and experiment.

+}
+
+bool IsDownloadCompleted(const DownloadItem* item) {

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

nit: "const DownloadItem&" if it can't be null

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

Wow, I did not know that this is possible with pointers to an abstract class.

+  return item->GetState() == DownloadItem::DownloadState::COMPLETE &&
+         !item->GetFileExternallyRemoved();
+}
+
+struct OrderDownloadsMostRecentlyDownloadedCompletedFirst {
+  bool operator()(const DownloadItem* left, const DownloadItem* right) const {
+    if (IsDownloadCompleted(left) != IsDownloadCompleted(right))
+      return IsDownloadCompleted(left);
+    return left->GetEndTime() > right->GetEndTime();
+  }
+};
+
+struct SuggestionItemWrapper {
+  base::Time time;
+  bool is_offline_page;
+  int index;
+  bool operator<(const SuggestionItemWrapper& other) const {
+    return time > other.time;
+  }
+};
+
+}  // namespace
+
+DownloadSuggestionsProvider::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)
+    : ContentSuggestionsProvider(observer, category_factory),
+      category_status_(CategoryStatus::AVAILABLE_LOADING),
+      provided_category_(
+          category_factory->FromKnownCategory(KnownCategories::DOWNLOADS)),
+      offline_page_proxy_(offline_page_proxy),
+      download_manager_notifier_(download_manager, this),
+      pref_service_(pref_service),
+      download_manager_ui_enabled_(download_manager_ui_enabled),
+      weak_ptr_factory_(this) {
+  observer->OnCategoryStatusChanged(this, provided_category_, category_status_);
+  offline_page_proxy_->AddObserver(this);
+  FetchAllDownloadsAndSubmitSuggestions();
+}
+
+DownloadSuggestionsProvider::~DownloadSuggestionsProvider() {
+  offline_page_proxy_->RemoveObserver(this);
+}
+
+CategoryStatus DownloadSuggestionsProvider::GetCategoryStatus(
+    Category category) {
+  if (category == provided_category_)
+    return category_status_;
+  NOTREACHED() << "Unknown category " << category.id();
+  return CategoryStatus::NOT_PROVIDED;

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

Just do
DCHECK_EQ(provided_category_, category);
return category_status_;

(no need to handle DCHECK or NOTREACHED failures - treat them like asserts)

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

Done.

+}
+
+CategoryInfo DownloadSuggestionsProvider::GetCategoryInfo(Category category) {
+  if (category == provided_category_) {
+    return CategoryInfo(
+        l10n_util::GetStringUTF16(IDS_NTP_DOWNLOAD_SUGGESTIONS_SECTION_HEADER),
+        ContentSuggestionsCardLayout::MINIMAL_CARD,
+        /*has_more_button=*/download_manager_ui_enabled_,
+        /*show_if_empty=*/false);
+  }
+  NOTREACHED() << "Unknown category " << category.id();

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

Same here

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

Done.

+  return CategoryInfo(base::string16(),
+                      ContentSuggestionsCardLayout::MINIMAL_CARD,
+                      /*has_more_button=*/false,
+                      /*show_if_empty=*/false);
+}
+
+void DownloadSuggestionsProvider::DismissSuggestion(
+    const ContentSuggestion::ID& suggestion_id) {
+  DCHECK_EQ(provided_category_, suggestion_id.category());
+  std::set<std::string> dismissed_ids =
+      ReadDismissedIDsFromPrefs(suggestion_id);
+  dismissed_ids.insert(suggestion_id.id_within_category());
+  StoreDismissedIDsToPrefs(suggestion_id, dismissed_ids);

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

nit: I find it a bit confusing to pass in the suggestion_id if you only need to
know offline vs asset - maybe just pass in that bool? Or just call
StoreAsset/OfflinePageDismissedIDsToPrefs directly?

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

I did the former. 
The latter is less attractive, because this function is called in more than one
place.

+
+  RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(suggestion_id);
+}
+
+void DownloadSuggestionsProvider::FetchSuggestionImage(
+    const ContentSuggestion::ID& suggestion_id,
+    const ImageFetchedCallback& callback) {
+  // TODO(vitaliii): Fetch proper thumbnail from OfflinePageModel once it is
+  // available there.
+  // TODO(vitaliii): Provide site's favicon for assets downloads.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, gfx::Image()));
+}
+
+void DownloadSuggestionsProvider::ClearHistory(
+    base::Time begin,
+    base::Time end,
+    const base::Callback<bool(const GURL& url)>& filter) {
+  ClearDismissedSuggestionsForDebugging(provided_category_);

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

This will call FetchAllDownloadsAndSubmitSuggestions, which we probably don't
want here.

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

Good point! Actually current cache is completely ignored by all |Fetch.*|
functions,
so this internal |FetchAllDownloadsAndSubmitSuggestions| call does the job.

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

Hm, but that is async, right? So maybe it's best to keep the .clear() calls, and
add a comment that ClearDismissed...() will trigger a re-fetch.

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

Done.

+  cached_offline_page_downloads_.clear();
+  cached_asset_downloads_.clear();
+  FetchAllDownloadsAndSubmitSuggestions();
+}
+
+void DownloadSuggestionsProvider::ClearCachedSuggestions(Category category) {
+  DCHECK_EQ(provided_category_, category);
+  cached_offline_page_downloads_.clear();
+  cached_asset_downloads_.clear();
+  FetchAllDownloadsAndSubmitSuggestions();

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

Would we expect this to change anything? The caches should already be up to date
with the "backends", right?

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

Good point.

+}
+
+void DownloadSuggestionsProvider::GetDismissedSuggestionsForDebugging(
+    Category category,
+    const DismissedSuggestionsCallback& callback) {
+  DCHECK_EQ(provided_category_, category);
+  // TODO(vitaliii): Implement.
+  callback.Run(std::vector<ContentSuggestion>());
+}
+
+void DownloadSuggestionsProvider::ClearDismissedSuggestionsForDebugging(
+    Category category) {
+  DCHECK_EQ(provided_category_, category);
+  StoreAssetDismissedIDsToPrefs(std::set<std::string>());
+  StoreOfflinePageDismissedIDsToPrefs(std::set<std::string>());
+  FetchAllDownloadsAndSubmitSuggestions();
+}
+
+// static
+void DownloadSuggestionsProvider::RegisterProfilePrefs(
+    PrefRegistrySimple* registry) {
+  registry->RegisterListPref(prefs::kDismissedAssetDownloadSuggestions);
+  registry->RegisterListPref(prefs::kDismissedOfflinePageDownloadSuggestions);
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Private methods
+
+void DownloadSuggestionsProvider::OfflinePageModelChanged(
+    const std::vector<offline_pages::OfflinePageItem>& offline_pages) {
+  NotifyStatusChanged(CategoryStatus::AVAILABLE);

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

SubmitContentSuggestions will change the status, so I don't think it's required
here.

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

Yes, but it will happen just before the new suggestions are sent.
However, I would like to notify the service before we start processing the data
(in case some loading indicator is shown).
Note that |NotifyStatusChanged| does nothing when called with the same status
for the second time.

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

But all this happens synchronously, so the UI doesn't get a chance to do
anything at all in the meantime.

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

Done.

+  ProcessAllOfflinePages(offline_pages);
+  SubmitContentSuggestions();
+}
+
+void DownloadSuggestionsProvider::OfflinePageDeleted(
+    int64_t offline_id,
+    const offline_pages::ClientId& client_id) {
+  // Because we never switch to NOT_PROVIDED dynamically, there can be no open
+  // UI containing an invalidated suggestion unless the status is something
+  // other than NOT_PROVIDED, so only notify invalidation in that case.

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

Actually, can the status *ever* be NOT_PROVIDED now?

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

Looks like not, but I am not sure. 
However, I do not like hardcoding this assumption, because it may backfire when
we start using NOT_PROVIDED, so I remove this check completely.

+  if (category_status_ != CategoryStatus::NOT_PROVIDED &&
+      IsOfflinePageDownload(client_id)) {
+    InvalidateSuggestion(GetOfflinePagePerCategoryID(offline_id));
+  }
+}
+
+void DownloadSuggestionsProvider::OnDownloadCreated(DownloadManager* manager,
+                                                    DownloadItem* item) {
+  // This is called when new downloads are started and on startup for existing
+  // ones.
+  if (CacheAssetDownloadIfNeeded(item))
+    SubmitContentSuggestions();
+}
+
+void DownloadSuggestionsProvider::OnDownloadUpdated(DownloadManager* manager,
+                                                    DownloadItem* item) {
+  // Unfinished downloads may become completed, therefore, this call cannot be
+  // ignored.
+  if (CacheAssetDownloadIfNeeded(item))
+    SubmitContentSuggestions();
+}
+void DownloadSuggestionsProvider::OnDownloadOpened(DownloadManager* manager,

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

nit: empty line before

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

Done.

+                                                   DownloadItem* item) {
+  // Ignored.
+}
+void DownloadSuggestionsProvider::OnDownloadRemoved(DownloadManager* manager,

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

Also here

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

Done.

+                                                    DownloadItem* item) {
+  if (!IsDownloadCompleted(item))
+    return;
+  // TODO(vitaliii): Implement a better way to clean up dismissed IDs (in case
+  // some calls are missed).
+  InvalidateSuggestion(GetAssetDownloadPerCategoryID(item->GetId()));
+}
+
+void DownloadSuggestionsProvider::NotifyStatusChanged(
+    CategoryStatus new_status) {
+  DCHECK_NE(CategoryStatus::NOT_PROVIDED, category_status_);

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

Should this check new_status instead of category_status_? Or maybe both?

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

Done.

+  if (category_status_ == new_status)
+    return;
+  category_status_ = new_status;
+  observer()->OnCategoryStatusChanged(this, provided_category_, new_status);

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

nit: s/new_status/category_status_/

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

Done.

+}
+
+void DownloadSuggestionsProvider::FetchOfflinePagesDownloads() {
+  offline_page_proxy_->GetAllPages(
+      base::Bind(&DownloadSuggestionsProvider::ProcessAllOfflinePages,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadSuggestionsProvider::
+    FetchOfflinePagesDownloadsAndSubmitSuggestions() {
+  offline_page_proxy_->GetAllPages(
+      base::Bind(&DownloadSuggestionsProvider::OfflinePageModelChanged,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void DownloadSuggestionsProvider::FetchAssetsDownloads() {
+  DownloadManager* manager = download_manager_notifier_.GetManager();
+  if (!manager) {
+    // The manager has gone down.
+    return;
+  }
+
+  NotifyStatusChanged(CategoryStatus::AVAILABLE);

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

Why is this here?

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

Done.

+
+  std::vector<DownloadItem*> downloads;
+  manager->GetAllDownloads(&downloads);
+
+  std::set<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  std::set<std::string> new_dismissed_ids;

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

Maybe old_dismissed_ids and retained_dismissed_ids?

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

Done.

+  std::vector<DownloadItem*> not_dismissed_downloads;
+  for (DownloadItem* item : downloads) {

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

const?

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

Done.

+    std::string within_category_id =
+        GetAssetDownloadPerCategoryID(item->GetId());
+    if (!dismissed_ids.count(within_category_id)) {
+      not_dismissed_downloads.push_back(item);
+    } else {
+      new_dismissed_ids.insert(within_category_id);
+    }
+  }
+
+  if (dismissed_ids.size() != new_dismissed_ids.size())
+    StoreAssetDismissedIDsToPrefs(new_dismissed_ids);
+
+  downloads = not_dismissed_downloads;

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

Maybe rename "downloads" to "all_downloads", "not_dismissed_downloads" to just
"downloads", and remove this line?

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

Done.

+  if (static_cast<int>(downloads.size()) > kMaxSuggestionsCount) {
+    std::nth_element(downloads.begin(),

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

Nice!
But since nth_element isn't very common, and IMO the name doesn't really say
what it does, maybe this is worth a comment?

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

Done.

+                     downloads.begin() + kMaxSuggestionsCount, downloads.end(),
+                     OrderDownloadsMostRecentlyDownloadedCompletedFirst());
+    downloads.resize(kMaxSuggestionsCount);
+  }
+
+  cached_asset_downloads_.clear();
+  for (DownloadItem* item : downloads) {
+    if (IsDownloadCompleted(item))

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

Wouldn't it be easier if the filtering happened before the sorting?

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

Done.

+      cached_asset_downloads_.push_back(item);
+  }
+}
+
+void DownloadSuggestionsProvider::FetchAllDownloadsAndSubmitSuggestions() {
+  FetchAssetsDownloads();
+  FetchOfflinePagesDownloadsAndSubmitSuggestions();

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

I was gonna say that this isn't safe since the offline page items might come
back before the asset ones. But it looks like FetchAssetsDownloads() is
synchronous but FetchOfflinePagesDownloads() isn't. Can we somehow differentiate
in the naming to reflect that?

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

Done.

+}
+
+void DownloadSuggestionsProvider::SubmitContentSuggestions() {
+  NotifyStatusChanged(CategoryStatus::AVAILABLE);
+
+  std::vector<SuggestionItemWrapper> suggestion_items;
+  for (int i = 0; i < static_cast<int>(cached_offline_page_downloads_.size());
+       ++i) {
+    SuggestionItemWrapper wrapped_item;
+    wrapped_item.is_offline_page = true;
+    wrapped_item.index = i;

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

Mh, this is a bit yucky. Maybe instead of bool+index, just have two pointers
(only one of which will be non-null)?

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

Acknowledged.

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

...this reply confused me ;P
"Obsolete, SuggestionItemWrapper doesn't exist anymore"

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

I've got it. Sorry about that.

+    wrapped_item.time = cached_offline_page_downloads_[i].last_access_time;
+    suggestion_items.push_back(wrapped_item);
+  }
+
+  for (int i = 0; i < static_cast<int>(cached_asset_downloads_.size()); ++i) {
+    SuggestionItemWrapper wrapped_item;
+    wrapped_item.is_offline_page = false;
+    wrapped_item.index = i;
+    wrapped_item.time = cached_asset_downloads_[i]->GetEndTime();
+    suggestion_items.push_back(wrapped_item);
+  }
+
+  std::sort(suggestion_items.begin(), suggestion_items.end());

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

Or in fact: Can the sorting just happen after converting everything to
ContentSuggestions? Then we wouldn't need the wrapper at all.

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

Done.

+
+  std::vector<ContentSuggestion> suggestions;
+  for (const SuggestionItemWrapper& wrapped_item : suggestion_items) {
+    if (suggestions.size() >= kMaxSuggestionsCount)
+      break;
+
+    if (wrapped_item.is_offline_page) {
+      suggestions.push_back(ConvertOfflinePage(
+          cached_offline_page_downloads_[wrapped_item.index]));
+    } else {
+      suggestions.push_back(
+          ConvertDownloadItem(cached_asset_downloads_[wrapped_item.index]));
+    }
+  }
+
+  observer()->OnNewSuggestions(this, provided_category_,
+                               std::move(suggestions));
+}
+
+ContentSuggestion DownloadSuggestionsProvider::ConvertOfflinePage(
+    const OfflinePageItem& offline_page) const {
+  // TODO(vitaliii): Make sure the URL is actually opened as an offline URL even
+  // when the user is online.

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

Do we have a bug for this? If not, could you file one? And then please reference
it here, like "crbug.com/12345". This probably applies to some other TODOs as
well.
(I know from personal experience that otherwise it's super hard to keep track of
everything that still needs to be done.)

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

Done.
Why do not you like 
https://cs.chromium.org/search/?q=TODO+file:%5Esrc/components/ntp_snippets/&s...
? :)

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

TODOs in the code aren't a good issue tracking system. E.g. there's no
prioritization or timing, and if Patrick or Michael want to know what still
needs to be done, they won't find it here.

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

I see, good point.

+  ContentSuggestion suggestion(
+      ContentSuggestion::ID(provided_category_, GetOfflinePagePerCategoryID(
+                                                    offline_page.offline_id)),
+      offline_page.url);
+
+  if (offline_page.title.empty()) {
+    // TODO(vitaliii): Remove this fallback once the OfflinePageModel provides
+    // titles for all (relevant) OfflinePageItems.
+    suggestion.set_title(base::UTF8ToUTF16(offline_page.url.spec()));
+  } else {
+    suggestion.set_title(offline_page.title);
+  }
+  suggestion.set_publish_date(offline_page.creation_time);
+  suggestion.set_publisher_name(base::UTF8ToUTF16(offline_page.url.host()));
+  return suggestion;
+}
+
+ContentSuggestion DownloadSuggestionsProvider::ConvertDownloadItem(
+    const DownloadItem* download_item) const {
+  ContentSuggestion::ID per_category_id(

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

This is not a per-category-ID. But I'd just inline the
"ContentSuggestion::ID(..)" below where you create the ContentSuggestion itself.

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

Done.

+      provided_category_,
+      GetAssetDownloadPerCategoryID(download_item->GetId()));
+  // TODO(vitaliii): Ensure that files are opened in browser, but not downloaded
+  // again.
+  ContentSuggestion suggestion(
+      per_category_id,
+      net::FilePathToFileURL(download_item->GetTargetFilePath()));
+  // TODO(vitaliii): Set proper title.
+  suggestion.set_title(
+      base::UTF8ToUTF16(download_item->GetTargetFilePath().BaseName().value()));

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

I think this won't work on all platforms, since paths may be encoded in
different ways, not necessarily UTF8. In Windows in particular, they're UTF-16.
And FilePath doesn't seem to provide any convenient way to convert to string...
:-/
If all this is compiled on Android only, then it'll probably be fine as-is.

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

This is Anroid only for now, added DCHECK for future.

+  suggestion.set_publish_date(download_item->GetEndTime());
+  suggestion.set_publisher_name(
+      base::UTF8ToUTF16(download_item->GetURL().host()));
+  // TODO(vitaliii): Set suggestion icon.
+  return suggestion;
+}
+
+bool DownloadSuggestionsProvider::CacheAssetDownloadIfNeeded(
+    const content::DownloadItem* item) {
+  if (!IsDownloadCompleted(item))
+    return false;
+
+  std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_;

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

Please avoid non-const references in general.
Here in particular, it makes it hard to see that in fact a member variable is
getting accessed/modified. Please just spell out cached_asset_downloads_.

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

Done.

+  if (std::find(cache.begin(), cache.end(), item) != cache.end())

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

optional: base::ContainsValue might be a bit easier to read

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

Done.

+    return false;
+
+  std::set<std::string> dismissed_ids = ReadAssetDismissedIDsFromPrefs();
+  if (dismissed_ids.count(GetAssetDownloadPerCategoryID(item->GetId())))
+    return false;
+
+  DCHECK(cache.size() <= kMaxSuggestionsCount);

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

DCHECK_LE?

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

Done.

+  if (cache.size() == kMaxSuggestionsCount) {
+    std::vector<const content::DownloadItem*>::iterator oldest =

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

optional: auto oldest_it = ...?

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

Done.

+        std::max_element(cache.begin(), cache.end(),
+                         OrderDownloadsMostRecentlyDownloadedCompletedFirst());
+    if (item->GetEndTime() <= (*oldest)->GetEndTime())

[vitaliii, 2016/10/11 08:15:56]

Would using an index here be better?

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

I don't follow - an index for what?

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

i.e. 
int i = oldest - cached_asset_downloads_.begin();
cached_asset_downloads_[i] = item;

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

Ah - no, what you have is fine.

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

Acknowledged.

+      return false;
+
+    (*oldest) = item;

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

nit: parens not required

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

Done.

+  } else {
+    cache.push_back(item);
+  }
+
+  return true;
+}
+
+bool DownloadSuggestionsProvider::RemoveSuggestionFromCacheIfPresent(
+    const ContentSuggestion::ID& suggestion_id) {
+  DCHECK_EQ(provided_category_, suggestion_id.category());
+  if (IsOfflinePageID(suggestion_id)) {
+    std::vector<offline_pages::OfflinePageItem>& cache =

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

As above: Please spell out cached_offline_page_downloads_

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

Done.

+        cached_offline_page_downloads_;
+    for (int i = 0; i < static_cast<int>(cache.size()); ++i) {

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

optional: This might be slightly nicer as an std::find_if with a lambda

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

Done.

+      if (GetOfflinePagePerCategoryID(cache[i].offline_id) ==
+          suggestion_id.id_within_category()) {
+        cache.erase(cache.begin() + i);
+        return true;
+      }
+    }
+    return false;
+  } else {

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

No "else" after "return"

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

Done.

+    std::vector<const content::DownloadItem*>& cache = cached_asset_downloads_;

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

Also here: Please spell out

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

Done.

+    for (int i = 0; i < static_cast<int>(cache.size()); ++i) {
+      if (GetAssetDownloadPerCategoryID(cache[i]->GetId()) ==
+          suggestion_id.id_within_category()) {
+        cache.erase(cache.begin() + i);
+        return true;
+      }
+    }
+    return false;
+  }
+}
+
+void DownloadSuggestionsProvider::
+    RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(
+        const ContentSuggestion::ID& suggestion_id) {
+  DCHECK_EQ(provided_category_, suggestion_id.category());
+  if (RemoveSuggestionFromCacheIfPresent(suggestion_id)) {

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

if (!Remove...)
  return;
and save a level of indentation.

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

Done.

+    if (IsOfflinePageID(suggestion_id)) {
+      if (cached_offline_page_downloads_.size() == kMaxSuggestionsCount - 1) {
+        // Previously there were |kMaxSuggestionsCount| cached suggestion,
+        // therefore, overall there may be more than |kMaxSuggestionsCount|
+        // suggestions in the model and now one of them may be cached instead of
+        // the removed one. Even though, the suggestions are not immediately
+        // used the cache has to be kept up to date, because it may be used when
+        // other data source is updated.
+        FetchOfflinePagesDownloads();
+      }
+    } else {
+      if (cached_asset_downloads_.size() == kMaxSuggestionsCount - 1) {
+        // The same as the case above.
+        FetchAssetsDownloads();
+      }
+    }
+  }
+}
+
+void DownloadSuggestionsProvider::ProcessAllOfflinePages(
+    const std::vector<offline_pages::OfflinePageItem>& all_offline_pages) {
+  std::set<std::string> old_dismissed_ids =
+      ReadOfflinePageDismissedIDsFromPrefs();
+  std::set<std::string> new_dismissed_ids;
+  std::vector<const OfflinePageItem*> items;
+  // Filtering out dismissed items and pruning dismissed IDs.
+  for (const OfflinePageItem& item : all_offline_pages) {
+    if (!IsOfflinePageDownload(item.client_id))
+      continue;
+
+    std::string per_category_id = GetOfflinePagePerCategoryID(item.offline_id);
+    if (!old_dismissed_ids.count(per_category_id))

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

I think the dismissed IDs sometimes have the "O"/"D" prefix and sometimes they
don't.
Actually, if you always store them with the prefix, then there's no need for two
separate prefs.

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

For the former - I do not think so, I added DCHECK.
For the latter, then pruning becomes much less trivial, however, it should be
manageable.
So should I have one pref or store ids without prefixes?
Also regarding one pref, I am not sure what happens if we get two updates from
different sources at the same time. For example, let's say we have an update for
offline pages and we read |old_dismissed_ids|. Just after this we have another
update, but for asset downloads and read same |old_dismissed_ids|, prune them
and store. Then we get back to offline pages, prune their ids (missing asset
downloads pruning) and store (rewriting asset downloads pruning). Can this be
the case?

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

I looked over all the code, and it looks like you do use them with the prefix
always. I guess I just got confused by the many kinds of IDs around here...
But it makes everything else (adding or checking dismissed IDs) easier :)
I think I'd prefer one.
Everything in this class happens on one thread. So unless you explicitly wait
for something asynchronous between reading the IDs and writing them back, it's
all good. There's no such thing as "at the same time" :)

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

Let's stick with this approach for now and then I will have a look whether there
is some neat way to prune IDs in one set.
I added a TODO and a bug.

+      items.push_back(&item);
+    else
+      new_dismissed_ids.insert(per_category_id);
+  }
+
+  if (static_cast<int>(items.size()) > kMaxSuggestionsCount) {
+    std::nth_element(items.begin(), items.begin() + kMaxSuggestionsCount,
+                     items.end(),
+                     OrderOfflinePagesByMostRecentlyVisitedFirst());

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

You could use a lambda here

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

Done.

+    items.resize(kMaxSuggestionsCount);
+  }
+
+  cached_offline_page_downloads_.clear();
+  for (const OfflinePageItem* item : items) {
+    cached_offline_page_downloads_.push_back(*item);
+  }
+
+  if (old_dismissed_ids.size() != new_dismissed_ids.size()) {
+    StoreOfflinePageDismissedIDsToPrefs(new_dismissed_ids);
+  }
+}
+
+void DownloadSuggestionsProvider::InvalidateSuggestion(
+    const std::string& per_category_id) {
+  ContentSuggestion::ID suggestion_id(provided_category_, per_category_id);
+  observer()->OnSuggestionInvalidated(this, suggestion_id);
+
+  std::set<std::string> dismissed_ids =
+      ReadDismissedIDsFromPrefs(suggestion_id);
+  auto it = dismissed_ids.find(per_category_id);
+  if (it != dismissed_ids.end()) {
+    dismissed_ids.erase(it);
+    StoreDismissedIDsToPrefs(suggestion_id, dismissed_ids);
+  }
+
+  RemoveSuggestionFromCacheAndRetrieveMoreIfNeeded(suggestion_id);
+}
+
+std::set<std::string>
+DownloadSuggestionsProvider::ReadAssetDismissedIDsFromPrefs() const {
+  return prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, prefs::kDismissedAssetDownloadSuggestions);
+}
+
+void DownloadSuggestionsProvider::StoreAssetDismissedIDsToPrefs(
+    const std::set<std::string>& dismissed_ids) {
+  prefs::StoreDismissedIDsToPrefs(
+      pref_service_, prefs::kDismissedAssetDownloadSuggestions, dismissed_ids);
+}
+
+std::set<std::string>
+DownloadSuggestionsProvider::ReadOfflinePageDismissedIDsFromPrefs() const {
+  return prefs::ReadDismissedIDsFromPrefs(
+      *pref_service_, prefs::kDismissedOfflinePageDownloadSuggestions);
+}
+
+void DownloadSuggestionsProvider::StoreOfflinePageDismissedIDsToPrefs(
+    const std::set<std::string>& dismissed_ids) {
+  prefs::StoreDismissedIDsToPrefs(
+      pref_service_, prefs::kDismissedOfflinePageDownloadSuggestions,
+      dismissed_ids);
+}
+
+std::set<std::string> DownloadSuggestionsProvider::ReadDismissedIDsFromPrefs(
+    const ContentSuggestion::ID& suggestion_id) const {
+  if (IsOfflinePageID(suggestion_id)) {
+    return ReadOfflinePageDismissedIDsFromPrefs();
+  } else {

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

nit: No "else" after "return"

Or maybe just get rid of the two Read methods and do
prefs::ReadDismissedIDsFromPrefs(*pref_service_, is_offline ?
prefs::kDismissedOfflinePageDownloadSuggestions : prefs::...);
?

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

Done.

+    return ReadAssetDismissedIDsFromPrefs();
+  }
+}
+
+void DownloadSuggestionsProvider::StoreDismissedIDsToPrefs(
+    const ContentSuggestion::ID& suggestion_id,
+    const std::set<std::string>& dismissed_ids) {
+  if (IsOfflinePageID(suggestion_id)) {
+    StoreOfflinePageDismissedIDsToPrefs(dismissed_ids);
+  } else {

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

Also here

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

Done.

+    StoreAssetDismissedIDsToPrefs(dismissed_ids);
+  }
+}
+
+}  // namespace ntp_snippets