[Downloads] Ask DownloadHistory if a download was from history.

chrome/browser/download/download_ui_controller.cc

Index: chrome/browser/download/download_ui_controller.cc
diff --git a/chrome/browser/download/download_ui_controller.cc b/chrome/browser/download/download_ui_controller.cc
index 89a3694d8b5592ffca30fb61979f64df5e22a32a..9feecd41de46555a319a3ebeb928985551f34083 100644
--- a/chrome/browser/download/download_ui_controller.cc
+++ b/chrome/browser/download/download_ui_controller.cc
@@ -4,8 +4,12 @@
 
 #include "chrome/browser/download/download_ui_controller.h"
 
+#include "base/callback.h"
 #include "base/stl_util.h"
+#include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_item_model.h"
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "content/public/browser/download_item.h"
@@ -37,6 +41,12 @@ class DefaultUIControllerDelegateAndroid
 
 void DefaultUIControllerDelegateAndroid::NotifyDownloadStarting(
     content::DownloadItem* item) {
+  // The Android DownloadController is only interested in IN_PROGRESS downloads.
+  // Ones which are INTERRUPTED etc. can't be handed over to the Android
+  // DownloadManager.
+  if (item->GetState() != content::DownloadItem::IN_PROGRESS)
+    return;
+
   // GET downloads without authentication are delegated to the Android
   // DownloadManager. Chrome is responsible for the rest.  See
   // InterceptDownloadResourceThrottle::ProcessDownloadRequest().
@@ -78,15 +88,35 @@ void DefaultUIControllerDelegate::NotifyDownloadStarting(
 
 #endif  // !OS_ANDROID
 
+// Predicate that returns true if the given download was not restored from
+// history. Used to construct a DownloadFilter based on a DownloadHistory.
+bool IsNewDownload(DownloadHistory* download_history,
+                   const content::DownloadItem* download_item) {
+  // Incognito profiles won't have a DownloadHistory. All downloads on such
+  // profiles are considered new.
+  return !download_history ||
+         !download_history->WasRestoredFromHistory(download_item);
+}
+
 } // namespace
 
 DownloadUIController::Delegate::~Delegate() {
 }
 
-DownloadUIController::DownloadUIController(content::DownloadManager* manager,
-                                           scoped_ptr<Delegate> delegate)
+DownloadUIController::DownloadFilter
+DownloadUIController::NewDownloadFilterFromDownloadHistory(
+    DownloadHistory* download_history) {
+  return base::Bind(&IsNewDownload, download_history);
+}
+
+DownloadUIController::DownloadUIController(
+    content::DownloadManager* manager,
+    const DownloadFilter& new_download_filter,
+    scoped_ptr<Delegate> delegate)
     : download_notifier_(manager, this),
+      new_download_filter_(new_download_filter),
       delegate_(delegate.Pass()) {
+  DCHECK(!new_download_filter_.is_null());
   if (!delegate_) {
 #if defined(OS_ANDROID)
     delegate_.reset(new DefaultUIControllerDelegateAndroid());
@@ -106,7 +136,7 @@ DownloadUIController::~DownloadUIController() {
 void DownloadUIController::OnDownloadCreated(content::DownloadManager* manager,
                                              content::DownloadItem* item) {
   // If this isn't a new download, there's nothing to do.
-  if (item->GetState() != content::DownloadItem::IN_PROGRESS)
+  if (!new_download_filter_.Run(item))

[Randy Smith (Not in Mondays), 2014/04/10 18:03:23]

There's something basic I'm not understanding here.  This code behaves the same
(modulo the change in this CL) as the old code, but why is that necessary?  If
we're restoring a download from the history, don't we want it to show up in the
UI as well?

[asanka, 2014/04/17 21:16:10]

This code was refactored somewhat, but the question is still valid.

Perhaps this class should be renamed, but its purpose is to pick out downloads
that the user should be actively displayed to the user, and then figure out how
to display such downloads. There's already a mechanism for checking for all
downloads that download history is using.

     return;
 
   DownloadItemModel(item).SetShouldNotifyUI(true);
@@ -126,10 +156,6 @@ void DownloadUIController::OnDownloadUpdated(content::DownloadManager* manager,
   if (item->GetTargetFilePath().empty())
     return;
 
-  // Can't be complete. That would imply that we didn't receive an
-  // OnDownloadUpdated() after the target was determined.
-  DCHECK_NE(content::DownloadItem::COMPLETE, item->GetState());
-
   DownloadItemModel(item).SetShouldNotifyUI(false);
   delegate_->NotifyDownloadStarting(item);
 }