[Downloads] Allow acquiring dangerous download file.

content/browser/download/download_manager_impl.cc

 // Copyright (c) 2012 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 "content/browser/download/download_manager_impl.h"
 
 #include <iterator>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 317 matching lines @@
   VLOG(20) << __FUNCTION__ << "()"
            << " shutdown_needed_ = " << shutdown_needed_;
   if (!shutdown_needed_)
     return;
   shutdown_needed_ = false;
 
   FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
   // TODO(benjhayden): Consider clearing observers_.
 
   // Go through all downloads in downloads_.  Dangerous ones we need to
   // remove on disk, and in progress ones we need to cancel.

[Randy Smith (Not in Mondays), 2013/05/28 17:29:40]

This comment seems obsolete.

[asanka, 2013/05/28 22:01:39]

Done.

   for (DownloadMap::iterator it = downloads_.begin(); it != downloads_.end();) {
     DownloadItemImpl* download = it->second;
 
     // Save iterator from potential erases in this set done by called code.
     // Iterators after an erasure point are still valid for lists and
     // associative containers such as sets.
     it++;

[Randy Smith (Not in Mondays), 2013/05/28 17:29:40]

We're no longer removing downloads here, so would it be worthwhile rewriting
this loop to be a bit simpler?

[asanka, 2013/05/28 22:01:39]

Done.

 
-    if (download->IsDangerous() && download->IsPartialDownload()) {
-      // The user hasn't accepted it, so we need to remove it
-      // from the disk.  This may or may not result in it being
-      // removed from the DownloadManager queues and deleted
-      // (specifically, DownloadManager::DownloadRemoved only
-      // removes and deletes it if it's known to the history service)
-      // so the only thing we know after calling this function is that
-      // the download was deleted if-and-only-if it was removed
-      // from all queues.
-      download->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);

[Randy Smith (Not in Mondays), 2013/05/28 17:29:40]

There's a semantic difference we're putting in which I think I'm ok with but I
wanted to call out: It used to be that dangerous downloads cancelled due to
shutdown were removed from the history.  Now they're just cancelled and the
download file removed.  I think that's ok (heck, I think it's better).  But it
is a semantic change worth calling out.

[asanka, 2013/05/28 22:01:39]

Yeah. I updated the CL description to mention this.

-    } else if (download->IsPartialDownload()) {
+    if (download->GetState() == DownloadItem::IN_PROGRESS)
       download->Cancel(false);
-    }
   }
 
   // At this point, all dangerous downloads have had their files removed
   // and all in progress downloads have been cancelled.  We can now delete
   // anything left.
 
   STLDeleteValues(&downloads_);
   downloads_.clear();
 
   // We'll have nothing more to report to the observers after this point.
@@ skipping 295 matching lines @@
   if (delegate_)
     delegate_->OpenDownload(download);
 }
 
 void DownloadManagerImpl::ShowDownloadInShell(DownloadItemImpl* download) {
   if (delegate_)
     delegate_->ShowDownloadInShell(download);
 }
 
 }  // namespace content