Added callback to DownloadUrl() so we can find download failures where the download item isn't created at all.

Added observer class for browser tests, which uses the callback.

BUG=117033
TEST=None

Review URL: http://codereview.chromium.org/9570005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@126251 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 51 insertions, 31 deletions

diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 3d608a4..82aba33 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -177,11 +177,7 @@ void DownloadFileManager::StartDownload(
   DCHECK(info);
 
   DownloadManager* manager = request_handle.GetDownloadManager();
-  if (!manager) {
-    request_handle.CancelRequest();
-    delete info;
-    return;
-  }
+  DCHECK(manager);  // Checked in |DownloadResourceHandler::StartOnUIThread()|.
 
   // |bound_net_log| will be used for logging the both the download item's and
   // the download file's events.
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 47d4a4f..446ead2 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -57,11 +57,12 @@ namespace {
 
 // Param structs exist because base::Bind can only handle 6 args.
 struct URLParams {
-  URLParams(const GURL& url, const GURL& referrer, int64 post_id)
-    : url_(url), referrer_(referrer), post_id_(post_id) {}
+  URLParams(const GURL& url, const GURL& referrer, int64 post_id, bool cache)
+    : url_(url), referrer_(referrer), post_id_(post_id), prefer_cache_(cache) {}
   GURL url_;
   GURL referrer_;
   int64 post_id_;
+  bool prefer_cache_;
 };
 
 struct RenderParams {
@@ -71,12 +72,13 @@ struct RenderParams {
   int render_view_id_;
 };
 
-void BeginDownload(const URLParams& url_params,
-                   bool prefer_cache,
-                   const DownloadSaveInfo& save_info,
-                   ResourceDispatcherHostImpl* resource_dispatcher_host,
-                   const RenderParams& render_params,
-                   content::ResourceContext* context) {
+void BeginDownload(
+    const URLParams& url_params,
+    const DownloadSaveInfo& save_info,
+    ResourceDispatcherHostImpl* resource_dispatcher_host,
+    const RenderParams& render_params,
+    content::ResourceContext* context,
+    const content::DownloadManager::OnStartedCallback& callback) {
   scoped_ptr<net::URLRequest> request(
       new net::URLRequest(url_params.url_, resource_dispatcher_host));
   request->set_referrer(url_params.referrer_.spec());
@@ -85,7 +87,7 @@ void BeginDownload(const URLParams& url_params,
     // 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(prefer_cache);
+    DCHECK(url_params.prefer_cache_);
     request->set_method("POST");
     scoped_refptr<net::UploadData> upload_data = new net::UploadData();
     upload_data->set_identifier(url_params.post_id_);
@@ -96,9 +98,9 @@ void BeginDownload(const URLParams& url_params,
       context,
       render_params.render_process_id_,
       render_params.render_view_id_,
-      prefer_cache,
+      url_params.prefer_cache_,
       save_info,
-      ResourceDispatcherHostImpl::DownloadStartedCallback());
+      callback);
 }
 
 class MapValueIteratorAdapter {
@@ -858,7 +860,8 @@ void DownloadManagerImpl::DownloadUrl(
     bool prefer_cache,
     int64 post_id,
     const DownloadSaveInfo& save_info,
-    WebContents* web_contents) {
+    WebContents* web_contents,
+    const OnStartedCallback& callback) {
   ResourceDispatcherHostImpl* resource_dispatcher_host =
       ResourceDispatcherHostImpl::Get();
   DCHECK(resource_dispatcher_host);
@@ -870,13 +873,13 @@ void DownloadManagerImpl::DownloadUrl(
       BrowserThread::IO, FROM_HERE,
       base::Bind(
           &BeginDownload,
-          URLParams(url, referrer, post_id),
-          prefer_cache,
+          URLParams(url, referrer, post_id, prefer_cache),
           save_info,
           resource_dispatcher_host,
           RenderParams(web_contents->GetRenderProcessHost()->GetID(),
                        web_contents->GetRenderViewHost()->GetRoutingID()),
-          web_contents->GetBrowserContext()->GetResourceContext()));
+          web_contents->GetBrowserContext()->GetResourceContext(),
+          callback));
 }
 
 void DownloadManagerImpl::AddObserver(Observer* observer) {
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 2703f00..13a8691 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -63,7 +63,8 @@ class CONTENT_EXPORT DownloadManagerImpl
                            bool prefer_cache,
                            int64 post_id,
                            const DownloadSaveInfo& save_info,
-                           content::WebContents* web_contents) OVERRIDE;
+                           content::WebContents* web_contents,
+                           const OnStartedCallback& callback) OVERRIDE;
   virtual void AddObserver(Observer* observer) OVERRIDE;
   virtual void RemoveObserver(Observer* observer) OVERRIDE;
   virtual void OnPersistentStoreQueryComplete(
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 2118915..5e6bd40 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -37,6 +37,20 @@ using content::DownloadManager;
 using content::ResourceDispatcherHostImpl;
 using content::ResourceRequestInfoImpl;
 
+namespace {
+
+void CallStartedCBOnUIThread(
+    const DownloadResourceHandler::OnStartedCallback& started_cb,
+    DownloadId id,
+    net::Error error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (started_cb.is_null())
+    return;
+  started_cb.Run(id, error);
+}
+
+}  // namespace
+
 DownloadResourceHandler::DownloadResourceHandler(
     int render_process_host_id,
     int render_view_id,
@@ -162,10 +176,11 @@ bool DownloadResourceHandler::OnResponseStarted(
 }
 
 void DownloadResourceHandler::CallStartedCB(DownloadId id, net::Error error) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   if (started_cb_.is_null())
     return;
-  started_cb_.Run(id, error);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&CallStartedCBOnUIThread, started_cb_, id, error));
   started_cb_.Reset();
 }
 
@@ -301,10 +316,7 @@ void DownloadResourceHandler::OnResponseCompletedInternal(
   download_stats::RecordAcceptsRanges(accept_ranges_, bytes_read_);
 
   // If the callback was already run on the UI thread, this will be a noop.
-  BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::Bind(&DownloadResourceHandler::CallStartedCB, this,
-                 download_id_, error_code));
+  CallStartedCB(download_id_, error_code);
 
   // We transfer ownership to |DownloadFileManager| to delete |buffer_|,
   // so that any functions queued up on the FILE thread are executed
@@ -329,6 +341,7 @@ void DownloadResourceHandler::StartOnUIThread(
   if (!download_manager) {
     // NULL in unittests or if the page closed right after starting the
     // download.
+    CallStartedCB(download_id_, net::ERR_ACCESS_DENIED);
     return;
   }
   DownloadId download_id = download_manager->delegate()->GetNextId();
@@ -385,6 +398,10 @@ void DownloadResourceHandler::CheckWriteProgress() {
 }
 
 DownloadResourceHandler::~DownloadResourceHandler() {
+  // This won't do anything if the callback was called before.
+  // If it goes through, it will likely be because OnWillStart() returned
+  // false somewhere in the chain of resource handlers.
+  CallStartedCB(download_id_, net::ERR_ACCESS_DENIED);
 }
 
 void DownloadResourceHandler::StartPauseTimer() {
@@ -405,7 +422,9 @@ std::string DownloadResourceHandler::DebugString() const {
                             " render_view_id_ = " "%d"
                             " save_info_.file_path = \"%" PRFilePath "\""
                             " }",
-                            request_->url().spec().c_str(),
+                            request_ ?
+                                request_->url().spec().c_str() :
+                                "<NULL request>",
                             download_id_.local(),
                             global_id_.child_id,
                             global_id_.request_id,
diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h
index 5b9c708..042a6ed 100644
--- a/content/browser/download/download_resource_handler.h
+++ b/content/browser/download/download_resource_handler.h
@@ -13,6 +13,7 @@
 #include "base/timer.h"
 #include "content/browser/download/download_types.h"
 #include "content/browser/renderer_host/resource_handler.h"
+#include "content/public/browser/download_manager.h"
 #include "content/public/browser/download_id.h"
 #include "content/public/browser/global_request_id.h"
 #include "net/base/net_errors.h"
@@ -32,8 +33,7 @@ class URLRequest;
 // Forwards data to the download thread.
 class DownloadResourceHandler : public ResourceHandler {
  public:
-  typedef base::Callback<void(content::DownloadId, net::Error)>
-      OnStartedCallback;
+  typedef content::DownloadManager::OnStartedCallback OnStartedCallback;
 
   static const size_t kLoadsToWrite = 100;  // number of data buffers queued
 
diff --git a/content/browser/download/drag_download_file.cc b/content/browser/download/drag_download_file.cc
index f7dc101..d7b3c79 100644
--- a/content/browser/download/drag_download_file.cc
+++ b/content/browser/download/drag_download_file.cc
@@ -141,7 +141,8 @@ void DragDownloadFile::InitiateDownload() {
                                  false,
                                  -1,
                                  save_info,
-                                 web_contents_);
+                                 web_contents_,
+                                 DownloadManager::OnStartedCallback());
 }
 
 void DragDownloadFile::DownloadCompleted(bool is_successful) {