[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 97c0a85e71467d22ca6882bf0cb6d34230a05d2e..b69addedbf1bbe82bb150de3198e4c44782bf708 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -55,90 +55,47 @@ scoped_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread> BeginDownload(
     uint32_t download_id,
     base::WeakPtr<DownloadManagerImpl> download_manager) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  // ResourceDispatcherHost{Base} is-not-a URLRequest::Delegate, and
-  // DownloadUrlParameters can-not include resource_dispatcher_host_impl.h, so
-  // we must down cast. RDHI is the only subclass of RDH as of 2012 May 4.
-  scoped_ptr<net::URLRequest> request(
-      params->resource_context()->GetRequestContext()->CreateRequest(
-          params->url(), net::DEFAULT_PRIORITY, NULL));
-  request->set_method(params->method());
-  if (!params->post_body().empty()) {
-    const std::string& body = params->post_body();
-    scoped_ptr<net::UploadElementReader> reader(
-        net::UploadOwnedBytesElementReader::CreateWithString(body));
-    request->set_upload(
-        net::ElementsUploadDataStream::CreateWithReader(std::move(reader), 0));
-  }
-  if (params->post_id() >= 0) {
-    // The POST in this case does not have an actual body, and only works
-    // when retrieving data from cache. This is done because we don't want
-    // to do a re-POST without user consent, and currently don't have a good
-    // plan on how to display the UI for that.
-    DCHECK(params->prefer_cache());
-    DCHECK_EQ("POST", params->method());
-    std::vector<scoped_ptr<net::UploadElementReader>> element_readers;
-    request->set_upload(make_scoped_ptr(new net::ElementsUploadDataStream(
-        std::move(element_readers), params->post_id())));
-  }
-
-  // If we're not at the beginning of the file, retrieve only the remaining
-  // portion.
-  bool has_last_modified = !params->last_modified().empty();
-  bool has_etag = !params->etag().empty();
-
-  // If we've asked for a range, we want to make sure that we only
-  // get that range if our current copy of the information is good.
-  // We shouldn't be asked to continue if we don't have a verifier.
-  DCHECK(params->offset() == 0 || has_etag || has_last_modified);
-
-  if (params->offset() > 0 && (has_etag || has_last_modified)) {
-    request->SetExtraRequestHeaderByName(
-        "Range",
-        base::StringPrintf("bytes=%" PRId64 "-", params->offset()),
-        true);
-
-    // In accordance with RFC 2616 Section 14.27, use If-Range to specify that
-    // the server return the entire entity if the validator doesn't match.
-    // Last-Modified can be used in the absence of ETag as a validator if the
-    // response headers satisfied the HttpUtil::HasStrongValidators() predicate.
-    //
-    // This function assumes that HasStrongValidators() was true and that the
-    // ETag and Last-Modified header values supplied are valid.
-    request->SetExtraRequestHeaderByName(
-        "If-Range", has_etag ? params->etag() : params->last_modified(), true);
-  }
-
-  for (DownloadUrlParameters::RequestHeadersType::const_iterator iter
-           = params->request_headers_begin();
-       iter != params->request_headers_end();
-       ++iter) {
-    request->SetExtraRequestHeaderByName(
-        iter->first, iter->second, false /*overwrite*/);
-  }
 
-  scoped_ptr<DownloadSaveInfo> save_info(new DownloadSaveInfo());
-  save_info->file_path = params->file_path();
-  save_info->suggested_name = params->suggested_name();
-  save_info->offset = params->offset();
-  save_info->hash_state = params->hash_state();
-  save_info->prompt_for_save_location = params->prompt();
-  save_info->file = params->GetFile();
+  scoped_ptr<net::URLRequest> url_request =
+      DownloadRequestCore::CreateRequestOnIOThread(download_id, params.get());
 
+  // If there's a valid renderer process associated with the request, then the
+  // request should be driven by the ResourceLoader. Pass it over to the
+  // ResourceDispatcherHostImpl which will in turn pass it along to the
+  // ResourceLoader.
   if (params->render_process_host_id() != -1) {
-    ResourceDispatcherHost::Get()->BeginDownload(
-        std::move(request), params->referrer(), params->content_initiated(),
-        params->resource_context(), params->render_process_host_id(),
-        params->render_view_host_routing_id(),
-        params->render_frame_host_routing_id(), params->prefer_cache(),
-        params->do_not_prompt_for_login(), std::move(save_info), download_id,
-        params->callback());
+    DownloadInterruptReason reason =
+        ResourceDispatcherHostImpl::Get()->BeginDownload(
+            std::move(url_request), params->referrer(),
+            params->content_initiated(), params->resource_context(),
+            params->render_process_host_id(),
+            params->render_view_host_routing_id(),
+            params->render_frame_host_routing_id(),
+            params->do_not_prompt_for_login());
+
+    // If the download was accepted, the DownloadResourceHandler is now
+    // responsible for driving the request to completion.
+    if (reason == DOWNLOAD_INTERRUPT_REASON_NONE)
+      return nullptr;
+
+    // Otherwise, create an interrupted download.
+    scoped_ptr<DownloadCreateInfo> failed_created_info(
+        new DownloadCreateInfo(base::Time::Now(), net::BoundNetLog(),
+                               make_scoped_ptr(new DownloadSaveInfo)));
+    failed_created_info->url_chain.push_back(params->url());
+    failed_created_info->result = reason;
+    scoped_ptr<ByteStreamReader> empty_byte_stream;
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&DownloadManager::StartDownload, download_manager,
+                   base::Passed(&failed_created_info),
+                   base::Passed(&empty_byte_stream), params->callback()));
     return nullptr;
   }
+
   return scoped_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread>(
-      UrlDownloader::BeginDownload(download_manager, std::move(request),
-                                   params->referrer(), params->prefer_cache(),
-                                   std::move(save_info), download_id,
-                                   params->callback())
+      UrlDownloader::BeginDownload(download_manager, std::move(url_request),
+                                   params->referrer())
           .release());
 }
 
@@ -345,6 +302,10 @@ void DownloadManagerImpl::StartDownload(
     const DownloadUrlParameters::OnStartedCallback& on_started) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DCHECK(info);
+  // |stream| is only non-nil if the download request was successful.
+  DCHECK(info->result != DOWNLOAD_INTERRUPT_REASON_NONE || stream.get());

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

Should this be an XOR?  I..e shouldn't result != NONE && stream.get() also be a
failure?

[asanka, 2016/02/11 03:43:07]

Done.

+  DVLOG(20) << __FUNCTION__ << "()"
+            << " result=" << DownloadInterruptReasonToString(info->result);
   uint32_t download_id = info->download_id;
   const bool new_download = (download_id == content::DownloadItem::kInvalidId);
   base::Callback<void(uint32_t)> got_id(base::Bind(
@@ -380,13 +341,12 @@ void DownloadManagerImpl::StartDownloadWithId(
       if (!on_started.is_null())
         on_started.Run(NULL, DOWNLOAD_INTERRUPT_REASON_USER_CANCELED);
       // The ByteStreamReader lives and dies on the FILE thread.
-      BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE,
-                                stream.release());
+      if (stream)
+        BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE,
+                                  stream.release());
       return;
     }
     download = item_iterator->second;
-    DCHECK_EQ(download->GetState(), DownloadItem::IN_PROGRESS);
-    download->MergeOriginInfoOnResume(*info);
   }
 
   base::FilePath default_download_directory;
@@ -397,20 +357,26 @@ 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(
-      std::move(info->save_info), default_download_directory, info->url(),
-      info->referrer_url, delegate_ && delegate_->GenerateFileHash(),
-      std::move(stream), 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->result == DOWNLOAD_INTERRUPT_REASON_NONE) {

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

nit, suggestion: I think this test is equivalent to if (stream) (DCHECK supports
that but doesn't rule out stream && result != NONE, but that's the above
comment).   Presuming that they are equivalent, I'd rather that this entire
function used only one of those tests (and checked the other condition with
DCHECKs) throughout rather than "if (stream)" above and "if (result == NONE)"
here for consistency.  I think when people see different tests in the code, they
instinctively assume tests for different things.

[asanka, 2016/02/11 03:43:07]

Got it. Changed the other test to if(result == NONE) which is the important
aspect we are checking. All others are side-effects.

+    DCHECK(stream.get());

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

Hmmm.  Was there a specific reason to removing this DCHECK?  I'm asking in case
I'm missing something--I think of using DCHECKs as a fine way to document and
keep in sync parallel expressions of the same state.

[Randy Smith (Not in Mondays), 2016/02/12 18:01:18]

Ping?

[asanka, 2016/02/12 18:31:46]

It was redundant with the DCHECK at the top of the function. I can reinstate it
if it makes it easier to keep track of what's valid where.

[Randy Smith (Not in Mondays), 2016/02/12 18:58:10]

Nah, I'm good.  Thanks for the pointer.

+    // Create the download file and start the download.

[Randy Smith (Not in Mondays), 2016/02/05 00:51:41]

nit: The placement of this comment makes me feel like it's inaccurate, as
DII::Start() is called whether or not we're in this conditional.  Now
DII::Start() may be called on an error resulting in the download not actually
starting, but the placement of the comment and the name of the method still
makes the comment confusing.

[asanka, 2016/02/11 03:43:07]

I removed the extra comments which I thought were spurious.

The DownloadFile is the other end of the bytestream. Creating it doesn't start
the download. That process happens after DII::Start.

+    download_file.reset(file_factory_->CreateFile(
+        *info->save_info, default_download_directory, info->url(),
+        info->referrer_url, delegate_ && delegate_->GenerateFileHash(),
+        std::move(info->save_info->file), std::move(stream),
+        download->GetBoundNetLog(), download->DestinationObserverAsWeakPtr()));
+
+    // Attach the client ID identifying the app to the AV system.
+    if (download_file.get() && delegate_) {

[Randy Smith (Not in Mondays), 2016/02/11 22:08:08]

nit: My understanding of the style guide is that curly braces should exist
around multi-line conditional clauses.

[Randy Smith (Not in Mondays), 2016/02/12 18:01:18]

Ping?

[asanka, 2016/02/12 18:31:46]

Ah. Done.

+      download_file->SetClientGuid(
+          delegate_->ApplicationClientIdForFileScanning());
+    }
   }
 
-  download->Start(std::move(download_file), std::move(info->request_handle));
+  download->Start(std::move(download_file), std::move(info->request_handle),
+                  *info);
 
   // For interrupted downloads, Start() will transition the state to
   // IN_PROGRESS and consumers will be notified via OnDownloadUpdated().
@@ -421,7 +387,7 @@ void DownloadManagerImpl::StartDownloadWithId(
     FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
 
   if (!on_started.is_null())
-    on_started.Run(download, DOWNLOAD_INTERRUPT_REASON_NONE);
+    on_started.Run(download, info->result);
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {