[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 <utility>
 
 #include "base/bind.h"
@@ skipping 37 matching lines @@
 #include "url/origin.h"
 
 namespace content {
 namespace {
 
 scoped_ptr<UrlDownloader, BrowserThread::DeleteOnIOThread> BeginDownload(
     scoped_ptr<DownloadUrlParameters> params,
     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())));
+
+  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) {
+    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;
   }
 
-  // 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();
-
-  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());
-    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());
 }
 
 class DownloadItemFactoryImpl : public DownloadItemFactory {
  public:
   DownloadItemFactoryImpl() {}
   ~DownloadItemFactoryImpl() override {}
 
   DownloadItemImpl* CreatePersistedItem(
       DownloadItemImplDelegate* delegate,
@@ skipping 180 matching lines @@
   url_downloaders_.clear();
 
   // We'll have nothing more to report to the observers after this point.
   observers_.Clear();
 
   if (delegate_)
     delegate_->Shutdown();
   delegate_ = NULL;
 }
 
 void DownloadManagerImpl::StartDownload(

[Randy Smith (Not in Mondays), 2016/02/10 21:48:45]

Suggestion: I find myself wondering if we should consider a name change of some
sort for this routine, given that it may or may not start the download depending
on the error code that comes in.  (Same thought for DII::Start()).

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

DII::Start() is, I think, OK since it starts the state machine and chugs along
even for an interrupted download (since it needs to determine the target). I
agree that StartDownload() isn't a great name since it may not start a download.
This particular method mostly just means that we have a download response. So
maybe something like, OnResponseReady()?

     scoped_ptr<DownloadCreateInfo> info,
     scoped_ptr<ByteStreamReader> stream,
     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(
       &DownloadManagerImpl::StartDownloadWithId, weak_factory_.GetWeakPtr(),
       base::Passed(&info), base::Passed(&stream), on_started, new_download));
   if (new_download) {
     GetNextId(got_id);
   } else {
     got_id.Run(download_id);
   }
@@ skipping 15 matching lines @@
     // 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);
       // 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;
   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(
-      std::move(info->save_info), default_download_directory, info->url(),
-      info->referrer_url, delegate_ && delegate_->GenerateFileHash(),
-      std::move(stream), 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->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().
   // 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));
 
   if (!on_started.is_null())
-    on_started.Run(download, DOWNLOAD_INTERRUPT_REASON_NONE);
+    on_started.Run(download, info->result);
 }
 
 void DownloadManagerImpl::CheckForHistoryFilesRemoval() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   for (DownloadMap::iterator it = downloads_.begin();
        it != downloads_.end(); ++it) {
     DownloadItemImpl* item = it->second;
     CheckForFileRemoval(item);
   }
 }
@@ skipping 304 matching lines @@
   if (delegate_)
     delegate_->OpenDownload(download);
 }
 
 void DownloadManagerImpl::ShowDownloadInShell(DownloadItemImpl* download) {
   if (delegate_)
     delegate_->ShowDownloadInShell(download);
 }
 
 }  // namespace content