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

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 338 matching lines @@
     delegate_->Shutdown();
   delegate_ = NULL;
 }
 
 void DownloadManagerImpl::StartDownload(
     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<ByteStreamReader> stream,
     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(
       &DownloadManagerImpl::StartDownloadWithId,
       weak_factory_.GetWeakPtr(),
       base::Passed(info.Pass()),
       base::Passed(stream.Pass()),
       on_started,
       new_download));
   if (new_download) {
     GetNextId(got_id);
   } else {
     got_id.Run(download_id);
   }
 }
 
 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) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_NE(content::DownloadItem::kInvalidId, id);
   DownloadItemImpl* download = NULL;
   if (new_download) {
     download = CreateActiveItem(id, *info);
   } else {
     DownloadMap::iterator item_iterator = downloads_.find(id);
     // Trying to resume an interrupted download.
     if (item_iterator == downloads_.end() ||
         (item_iterator->second->GetState() == DownloadItem::CANCELLED)) {
       // If the download is no longer known to the DownloadManager, then it was
       // removed after it was resumed. Ignore. If the download is cancelled
       // while resuming, then also ignore the request.
       info->request_handle.CancelRequest();
       if (!on_started.is_null())
         on_started.Run(NULL, DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
       return;
     }
     download = item_iterator->second;
     DCHECK_EQ(DownloadItem::INTERRUPTED, download->GetState());
-    download->MergeOriginInfoOnResume(*info);
   }
 
   base::FilePath default_download_directory;
   if (delegate_) {
     base::FilePath website_save_directory;  // Unused
     bool skip_dir_check = false;            // Unused
     delegate_->GetSaveDir(GetBrowserContext(), &website_save_directory,
                           &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()));
+  scoped_ptr<DownloadFile> download_file;
 
-  // Attach the client ID identifying the app to the AV system.
-  if (download_file.get() && delegate_) {
-    download_file->SetClientGuid(
-        delegate_->ApplicationClientIdForFileScanning());
+  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());
+    }
   }
 
+  // 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());
-
-  // 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.
-  if (new_download)
-    FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
+  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() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
     CheckForFileRemoval(item);
   }
 }
@@ skipping 257 matching lines @@
   if (delegate_)
     delegate_->OpenDownload(download);
 }
 
 void DownloadManagerImpl::ShowDownloadInShell(DownloadItemImpl* download) {
   if (delegate_)
     delegate_->ShowDownloadInShell(download);
 }
 
 }  // namespace content