[Downloads] Always call DM::StartDownload() for explicit downloads.

content/browser/download/download_manager_impl.cc

Index: content/browser/download/download_manager_impl.cc
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 1705678218f674266595dc212c0fc0894c188156..f3fa4dd59a68f3582eb9200fc4ac5daef46c0e35 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -356,6 +356,9 @@ void DownloadManagerImpl::StartDownload(
     const DownloadUrlParameters::OnStartedCallback& on_started) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(info);
+  // |stream| is only non-nil if the download request was successful.
+  DCHECK(info->interrupt_reason != DOWNLOAD_INTERRUPT_REASON_NONE ||
+         stream.get());
   uint32 download_id = info->download_id;
   const bool new_download = (download_id == content::DownloadItem::kInvalidId);
   base::Callback<void(uint32)> got_id(base::Bind(
@@ -374,7 +377,7 @@ void DownloadManagerImpl::StartDownload(
 
 void DownloadManagerImpl::StartDownloadWithId(
     scoped_ptr<DownloadCreateInfo> info,
-    scoped_ptr<ByteStreamReader> stream,
+    scoped_ptr<ByteStreamReader> byte_stream,
     const DownloadUrlParameters::OnStartedCallback& on_started,
     bool new_download,
     uint32 id) {
@@ -398,7 +401,6 @@ void DownloadManagerImpl::StartDownloadWithId(
     }
     download = item_iterator->second;
     DCHECK_EQ(DownloadItem::INTERRUPTED, download->GetState());
-    download->MergeOriginInfoOnResume(*info);
   }
 
   base::FilePath default_download_directory;
@@ -409,35 +411,45 @@ void DownloadManagerImpl::StartDownloadWithId(
                           &default_download_directory, &skip_dir_check);
   }
 
-  // Create the download file and start the download.
-  scoped_ptr<DownloadFile> download_file(
-      file_factory_->CreateFile(
-          info->save_info.Pass(), default_download_directory,
-          info->url(), info->referrer_url,
-          delegate_ && delegate_->GenerateFileHash(),
-          stream.Pass(), download->GetBoundNetLog(),
-          download->DestinationObserverAsWeakPtr()));
-
-  // Attach the client ID identifying the app to the AV system.
-  if (download_file.get() && delegate_) {
-    download_file->SetClientGuid(
-        delegate_->ApplicationClientIdForFileScanning());
+  scoped_ptr<DownloadFile> download_file;
+
+  if (info->interrupt_reason == DOWNLOAD_INTERRUPT_REASON_NONE) {
+    DCHECK(byte_stream.get());
+    // Create the download file and start the download.
+    download_file.reset(
+        file_factory_->CreateFile(*info->save_info,
+                                  default_download_directory,
+                                  info->url(),
+                                  info->referrer_url,
+                                  delegate_ && delegate_->GenerateFileHash(),
+                                  info->save_info->file_stream.Pass(),

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

Just curious: Why make this an explicit argument?

[asanka, 2014/03/19 20:37:06]

The ownership of FileStream is being passed into the DownloadFile but it can't
modify the the rest of DownloadSaveInfo.

+                                  byte_stream.Pass(),
+                                  download->GetBoundNetLog(),
+                                  download->DestinationObserverAsWeakPtr()));
+
+    // Attach the client ID identifying the app to the AV system.
+    if (download_file.get() && delegate_) {
+      download_file->SetClientGuid(
+          delegate_->ApplicationClientIdForFileScanning());
+    }
   }
 
-  scoped_ptr<DownloadRequestHandleInterface> req_handle(
-      new DownloadRequestHandle(info->request_handle));
-  download->Start(download_file.Pass(), req_handle.Pass());
-
-  // For interrupted downloads, Start() will transition the state to
-  // IN_PROGRESS and consumers will be notified via OnDownloadUpdated().
-  // For new downloads, we notify here, rather than earlier, so that
-  // the download_file is bound to download and all the usual
-  // setters (e.g. Cancel) work.
+  // OnDownloadCreated notification needs to be issued before Start() is called
+  // so that observers can choose to observe the initial state transition that
+  // happens during DownloadItem::Start(). Active DownloadItems are created in
+  // an IN_PROGRESS state. If the download is already interrupted, then Start()
+  // will transition the download to INTERRUPTED. This is currently the only
+  // distinction between a download created in response to an interrupted
+  // request, and an interrupted download being recreated from history.

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

How are you dealing with the issue in the deleted comment that used to be below,
i.e. making sure Cancel&etc. work after the consumers get ahold of the
DownloadItem?  The DownloadFile isn't bound at this point.

I'll note that I'm feeling a bit pulled towards a state transition diagram where
we always start in INTERRUPTED and transition to IN_PROGRESS in Start() if
that's appropriate.  That would fit with the above concern, and I think keep the
state diagram cleaner.  It would either mean we need to teach consumers to
expect that in observation of download items, or we mask it somehow (maybe with
a STARTING state that acts like INTERRUPTED from our perspective).  WDYT?

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

As an alternative to the above comment, we could just create the download item
in the proper state by adding an argument to CreateActiveItem() above.  Given
that that means we won't be doing an IN_PROGRESS->INTERRUPTED state transition,
it might solve the question of when to post the OnDownloadCreated()
notification.

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

The above thoughts bring up a side issue: When we're resuming a download, and
the incoming DCI has a non-NONE DIR, does the consumer see an updated transition
when going from INTERRUPTED->INTERRUPTED with a new DIR?

[asanka, 2014/03/19 20:37:06]

I've been mulling a STARTING state as mentioned elsewhere. The shuffle here is
almost purely to account for the problem of external observers not being able to
distinguish between new downloads that start interrupted vs. old interrupted
downloads being restored from history.

[asanka, 2014/03/19 20:37:06]

Yeah. The consumer is supposed to see an OnDownloadUpdated() notification.

   if (new_download)
     FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
+  scoped_ptr<DownloadRequestHandleInterface> req_handle(
+      new DownloadRequestHandle(info->request_handle));
+  download->Start(download_file.Pass(), req_handle.Pass(), *info);
+
   if (!on_started.is_null())
-    on_started.Run(download, DOWNLOAD_INTERRUPT_REASON_NONE);
+    on_started.Run(download, info->interrupt_reason);

[Randy Smith (Not in Mondays), 2014/03/18 21:07:28]

nit: This violates the contract on started_cb described in
DownloadUrlParameters; that comment should be fixed.

[asanka, 2014/03/19 20:37:06]

Done.

 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {