Download Feedback Service should upload all eligible downloads

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index f2494eb3fc479ad88b90c3c71d59901c646be2cd..f5a2ec8e5a6c6c8b916c87066693a47ab47155c0 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -93,6 +93,17 @@ static base::FilePath DownloadFileDetach(
   return full_path;
 }
 
+static base::FilePath MakeCopyOfDownloadFile(
+  DownloadFile* download_file) {
+  DCHECK_CURRENTLY_ON(BrowserThread::FILE);
+  base::FilePath temp_file_path;
+  if(base::CreateTemporaryFile(&temp_file_path) &&
+     base::CopyFile(download_file->FullPath(), temp_file_path))
+    return temp_file_path;
+  else
+    return base::FilePath();

[asanka, 2016/11/01 21:09:23]

Can this branch delete the file at |temp_file_path| if |temp_file_path| contains
a valid path? 

base::CopyFile may leave a partial file around if it fails part-way. Leaving
those files around can be harmful in some cases. E.g. if CopyFile failed because
the temporary file location ran out of space lots of things will go wrong
quickly unless we free up the space again.

Also FYI the download file and the temp file may end up on different storage
devices and may be subject to different size restrictions.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

+}
+
 static void DownloadFileCancel(std::unique_ptr<DownloadFile> download_file) {
   DCHECK_CURRENTLY_ON(BrowserThread::FILE);
   download_file->Cancel();
@@ -299,23 +310,32 @@ void DownloadItemImpl::ValidateDangerousDownload() {
 }
 
 void DownloadItemImpl::StealDangerousDownload(
+    bool delete_file_afterward,
     const AcquireFileCallback& callback) {
   DVLOG(20) << __func__ << "() download = " << DebugString(true);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(IsDangerous());

[asanka, 2016/11/01 20:58:58]

Can you also DCHECK AllDataSaved() ?

It appears that the logic below will only have the desired effect if
AllDataSaved() is true. I.e. the file is complete.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

 
-  if (download_file_) {
+  if (delete_file_afterward) {
+    if (download_file_){
+      BrowserThread::PostTaskAndReplyWithResult(
+          BrowserThread::FILE,
+          FROM_HERE,
+          base::Bind(&DownloadFileDetach, base::Passed(&download_file_)),
+          callback);
+    } else {
+      callback.Run(current_path_);
+    }
+    current_path_.clear();
+    Remove();
+    // We have now been deleted.
+  } else if (download_file_) {

[asanka, 2016/11/01 20:58:58]

If there's no download_file_, then you can callback.Run(current_path_).
Otherwise that branch will cause the callback to never be run. All branches must
invoke |callback| at some point. Otherwise the download will stall.

[Jialiu Lin, 2016/11/03 20:16:50]

Done.

     BrowserThread::PostTaskAndReplyWithResult(
         BrowserThread::FILE,
         FROM_HERE,
-        base::Bind(&DownloadFileDetach, base::Passed(&download_file_)),
+        base::Bind(&MakeCopyOfDownloadFile, download_file_.get()),
         callback);
-  } else {
-    callback.Run(current_path_);
   }
-  current_path_.clear();
-  Remove();
-  // We have now been deleted.
 }
 
 void DownloadItemImpl::Pause() {
@@ -1375,7 +1395,6 @@ void DownloadItemImpl::MaybeCompleteDownload() {
           base::Bind(&DownloadItemImpl::MaybeCompleteDownload,
                      weak_ptr_factory_.GetWeakPtr())))
     return;
-
   // Confirm we're in the proper set of states to be here; have all data, have a
   // history handle, (validated or safe).
   DCHECK_EQ(IN_PROGRESS_INTERNAL, state_);