summaryrefslogtreecommitdiffstats
path: root/content
diff options
context:
space:
mode:
authorrdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-08-19 14:53:00 +0000
committerrdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2012-08-19 14:53:00 +0000
commite2c1a644a1be1b3e0a99148ee3590b6da506d86e (patch)
tree59a8b98bf0cf48cb8229d00cd98021622b6700e7 /content
parent34df2abd18b58f0a0b41fc67237a2f33c75e91b2 (diff)
downloadchromium_src-e2c1a644a1be1b3e0a99148ee3590b6da506d86e.zip
chromium_src-e2c1a644a1be1b3e0a99148ee3590b6da506d86e.tar.gz
chromium_src-e2c1a644a1be1b3e0a99148ee3590b6da506d86e.tar.bz2
Revert 152213 - Replace the DownloadFileManager with direct ownership of DownloadFile.
This CL is a re-land of http://codereview.chromium.org/10799005, which was reverted due to test flakiness. BUG=123998 R=benjhayden@chromium.org TBR=jam@chromium.org Review URL: https://chromiumcodereview.appspot.com/10836293 BUG=143600 TBR=rdsmith@chromium.org Review URL: https://chromiumcodereview.appspot.com/10861002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@152282 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'content')
-rw-r--r--content/browser/browser_context.cc4
-rw-r--r--content/browser/browser_main_loop.cc5
-rw-r--r--content/browser/download/base_file.cc1
-rw-r--r--content/browser/download/base_file.h2
-rw-r--r--content/browser/download/download_file.h24
-rw-r--r--content/browser/download/download_file_factory.cc32
-rw-r--r--content/browser/download/download_file_factory.h44
-rw-r--r--content/browser/download/download_file_impl.cc129
-rw-r--r--content/browser/download/download_file_impl.h47
-rw-r--r--content/browser/download/download_file_manager.cc227
-rw-r--r--content/browser/download/download_file_manager.h180
-rw-r--r--content/browser/download/download_file_manager_unittest.cc413
-rw-r--r--content/browser/download/download_file_unittest.cc149
-rw-r--r--content/browser/download/download_item_impl.cc213
-rw-r--r--content/browser/download/download_item_impl.h54
-rw-r--r--content/browser/download/download_item_impl_delegate.cc13
-rw-r--r--content/browser/download/download_item_impl_delegate.h14
-rw-r--r--content/browser/download/download_item_impl_unittest.cc243
-rw-r--r--content/browser/download/download_manager_impl.cc198
-rw-r--r--content/browser/download/download_manager_impl.h50
-rw-r--r--content/browser/download/download_manager_impl_unittest.cc141
-rw-r--r--content/browser/download/download_resource_handler.cc1
-rw-r--r--content/browser/download/mock_download_file.cc12
-rw-r--r--content/browser/download/mock_download_file.h3
-rw-r--r--content/browser/download/save_package.cc11
-rw-r--r--content/browser/renderer_host/resource_dispatcher_host_impl.cc4
-rw-r--r--content/browser/renderer_host/resource_dispatcher_host_impl.h8
-rw-r--r--content/content_browser.gypi5
-rw-r--r--content/content_tests.gypi1
-rw-r--r--content/public/browser/download_destination_observer.h37
-rw-r--r--content/public/browser/download_manager.h23
-rw-r--r--content/public/test/mock_download_manager.h14
-rw-r--r--content/public/test/test_file_error_injector.cc322
-rw-r--r--content/public/test/test_file_error_injector.h34
34 files changed, 1648 insertions, 1010 deletions
diff --git a/content/browser/browser_context.cc b/content/browser/browser_context.cc
index 527fe43..4916ea0 100644
--- a/content/browser/browser_context.cc
+++ b/content/browser/browser_context.cc
@@ -7,6 +7,7 @@
#include "content/browser/appcache/chrome_appcache_service.h"
#include "webkit/database/database_tracker.h"
#include "content/browser/dom_storage/dom_storage_context_impl.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_manager_impl.h"
#include "content/browser/in_process_webkit/indexed_db_context_impl.h"
#include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
@@ -79,8 +80,11 @@ DownloadManager* BrowserContext::GetDownloadManager(
if (!context->GetUserData(kDownloadManagerKeyName)) {
ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
DCHECK(rdh);
+ DownloadFileManager* file_manager = rdh->download_file_manager();
+ DCHECK(file_manager);
scoped_refptr<DownloadManager> download_manager =
new DownloadManagerImpl(
+ file_manager,
scoped_ptr<DownloadItemFactory>(),
GetContentClient()->browser()->GetNetLog());
diff --git a/content/browser/browser_main_loop.cc b/content/browser/browser_main_loop.cc
index 5c418ff..faed5fc 100644
--- a/content/browser/browser_main_loop.cc
+++ b/content/browser/browser_main_loop.cc
@@ -16,6 +16,7 @@
#include "base/string_number_conversions.h"
#include "base/threading/thread_restrictions.h"
#include "content/browser/browser_thread_impl.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/save_file_manager.h"
#include "content/browser/gamepad/gamepad_service.h"
#include "content/browser/gpu/browser_gpu_channel_host_factory.h"
@@ -565,8 +566,10 @@ void BrowserMainLoop::ShutdownThreadsAndCleanUp() {
// Clean up state that lives on or uses the file_thread_ before
// it goes away.
- if (resource_dispatcher_host_.get())
+ if (resource_dispatcher_host_.get()) {
+ resource_dispatcher_host_.get()->download_file_manager()->Shutdown();
resource_dispatcher_host_.get()->save_file_manager()->Shutdown();
+ }
break;
case BrowserThread::PROCESS_LAUNCHER:
thread_to_stop = &process_launcher_thread_;
diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc
index debf0fb..1d5d1e2 100644
--- a/content/browser/download/base_file.cc
+++ b/content/browser/download/base_file.cc
@@ -220,6 +220,7 @@ BaseFile::BaseFile(const FilePath& full_path,
calculate_hash_(calculate_hash),
detached_(false),
bound_net_log_(bound_net_log) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
memcpy(sha256_hash_, kEmptySha256Hash, kSha256HashLen);
if (file_stream_.get()) {
file_stream_->SetBoundNetLogSource(bound_net_log_);
diff --git a/content/browser/download/base_file.h b/content/browser/download/base_file.h
index c488c34..7ada03c 100644
--- a/content/browser/download/base_file.h
+++ b/content/browser/download/base_file.h
@@ -29,8 +29,6 @@ class FileStream;
// for DownloadFile and SaveFile, which keep more state information.
class CONTENT_EXPORT BaseFile {
public:
- // May be constructed on any thread. All other routines (including
- // destruction) must occur on the FILE thread.
BaseFile(const FilePath& full_path,
const GURL& source_url,
const GURL& referrer_url,
diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h
index e53602d..e707a26 100644
--- a/content/browser/download/download_file.h
+++ b/content/browser/download/download_file.h
@@ -11,6 +11,7 @@
#include "base/callback_forward.h"
#include "base/file_path.h"
#include "content/common/content_export.h"
+#include "content/public/browser/download_id.h"
#include "content/public/browser/download_interrupt_reasons.h"
namespace content {
@@ -23,12 +24,6 @@ class DownloadManager;
// cancelled, the DownloadFile is destroyed.
class CONTENT_EXPORT DownloadFile {
public:
- // Callback used with Initialize. On a successful initialize, |reason| will
- // be DOWNLOAD_INTERRUPT_REASON_NONE; on a failed initialize, it will be
- // set to the reason for the failure.
- typedef base::Callback<void(content::DownloadInterruptReason reason)>
- InitializeCallback;
-
// Callback used with Rename(). On a successful rename |reason| will be
// DOWNLOAD_INTERRUPT_REASON_NONE and |path| the path the rename
// was done to. On a failed rename, |reason| will contain the
@@ -38,10 +33,10 @@ class CONTENT_EXPORT DownloadFile {
virtual ~DownloadFile() {}
+ // If calculate_hash is true, sha256 hash will be calculated.
// Returns DOWNLOAD_INTERRUPT_REASON_NONE on success, or a network
- // error code on failure. Upon completion, |callback| will be
- // called on the UI thread as per the comment above.
- virtual void Initialize(const InitializeCallback& callback) = 0;
+ // error code on failure.
+ virtual DownloadInterruptReason Initialize() = 0;
// Rename the download file to |full_path|. If that file exists and
// |overwrite_existing_file| is false, |full_path| will be uniquified by
@@ -73,11 +68,14 @@ class CONTENT_EXPORT DownloadFile {
// Returns the current (intermediate) state of the hash as a byte string.
virtual std::string GetHashState() = 0;
- // For testing. Must be called on FILE thread.
- static int GetNumberOfDownloadFiles();
+ // Cancels the download request associated with this file.
+ virtual void CancelDownloadRequest() = 0;
+
+ virtual int Id() const = 0;
+ virtual DownloadManager* GetDownloadManager() = 0;
+ virtual const DownloadId& GlobalId() const = 0;
- protected:
- static int number_active_objects_;
+ virtual std::string DebugString() const = 0;
};
} // namespace content
diff --git a/content/browser/download/download_file_factory.cc b/content/browser/download/download_file_factory.cc
deleted file mode 100644
index 8678607..0000000
--- a/content/browser/download/download_file_factory.cc
+++ /dev/null
@@ -1,32 +0,0 @@
-// 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_file_factory.h"
-
-#include "content/browser/download/download_file_impl.h"
-#include "content/browser/power_save_blocker.h"
-
-namespace content {
-
-DownloadFileFactory::~DownloadFileFactory() {}
-
-content::DownloadFile* DownloadFileFactory::CreateFile(
- const content::DownloadSaveInfo& save_info,
- GURL url,
- GURL referrer_url,
- int64 received_bytes,
- bool calculate_hash,
- scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
- base::WeakPtr<content::DownloadDestinationObserver> observer) {
- scoped_ptr<content::PowerSaveBlocker> psb(
- new content::PowerSaveBlocker(
- content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
- "Download in progress"));
- return new DownloadFileImpl(
- save_info, url, referrer_url, received_bytes, calculate_hash,
- stream.Pass(), bound_net_log, psb.Pass(), observer);
-}
-
-} // namespace content
diff --git a/content/browser/download/download_file_factory.h b/content/browser/download/download_file_factory.h
deleted file mode 100644
index 97cb22b..0000000
--- a/content/browser/download/download_file_factory.h
+++ /dev/null
@@ -1,44 +0,0 @@
-// 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.
-//
-#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_
-#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_
-
-#include "base/memory/scoped_ptr.h"
-#include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
-#include "content/common/content_export.h"
-#include "googleurl/src/gurl.h"
-
-namespace net {
-class BoundNetLog;
-}
-
-namespace content {
-
-struct DownloadSaveInfo;
-
-class ByteStreamReader;
-class DownloadDestinationObserver;
-class DownloadFile;
-class DownloadManager;
-
-class CONTENT_EXPORT DownloadFileFactory {
- public:
- virtual ~DownloadFileFactory();
-
- virtual content::DownloadFile* CreateFile(
- const content::DownloadSaveInfo& save_info,
- GURL url,
- GURL referrer_url,
- int64 received_bytes,
- bool calculate_hash,
- scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
- base::WeakPtr<content::DownloadDestinationObserver> observer);
-};
-
-} // namespace content
-
-#endif // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_FACTORY_H_
diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc
index 5bd2412..a1ab62a 100644
--- a/content/browser/download/download_file_impl.cc
+++ b/content/browser/download/download_file_impl.cc
@@ -16,7 +16,7 @@
#include "content/browser/download/download_net_log_parameters.h"
#include "content/browser/power_save_blocker.h"
#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/download_destination_observer.h"
+#include "content/public/browser/download_manager.h"
#include "content/browser/download/download_stats.h"
#include "net/base/io_buffer.h"
@@ -27,72 +27,57 @@ using content::DownloadManager;
const int kUpdatePeriodMs = 500;
const int kMaxTimeBlockingFileThreadMs = 1000;
-int content::DownloadFile::number_active_objects_ = 0;
-
DownloadFileImpl::DownloadFileImpl(
- const content::DownloadSaveInfo& save_info,
- const GURL& url,
- const GURL& referrer_url,
- int64 received_bytes,
- bool calculate_hash,
+ const DownloadCreateInfo* info,
scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
+ DownloadRequestHandleInterface* request_handle,
+ scoped_refptr<DownloadManager> download_manager,
+ bool calculate_hash,
scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
- base::WeakPtr<content::DownloadDestinationObserver> observer)
- : file_(save_info.file_path,
- url,
- referrer_url,
- received_bytes,
+ const net::BoundNetLog& bound_net_log)
+ : file_(info->save_info.file_path,
+ info->url(),
+ info->referrer_url,
+ info->received_bytes,
calculate_hash,
- save_info.hash_state,
- save_info.file_stream,
+ info->save_info.hash_state,
+ info->save_info.file_stream,
bound_net_log),
stream_reader_(stream.Pass()),
+ id_(info->download_id),
+ request_handle_(request_handle),
+ download_manager_(download_manager),
bytes_seen_(0),
bound_net_log_(bound_net_log),
- observer_(observer),
weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
power_save_blocker_(power_save_blocker.Pass()) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DCHECK(download_manager.get());
}
DownloadFileImpl::~DownloadFileImpl() {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
- --number_active_objects_;
}
-void DownloadFileImpl::Initialize(const InitializeCallback& callback) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
+content::DownloadInterruptReason DownloadFileImpl::Initialize() {
update_timer_.reset(new base::RepeatingTimer<DownloadFileImpl>());
-
- net::Error net_result = file_.Initialize();
- if (net_result != net::OK) {
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE, base::Bind(
- callback, content::ConvertNetErrorToInterruptReason(
- net_result, content::DOWNLOAD_INTERRUPT_FROM_DISK)));
- return;
+ net::Error result = file_.Initialize();
+ if (result != net::OK) {
+ return content::ConvertNetErrorToInterruptReason(
+ result, content::DOWNLOAD_INTERRUPT_FROM_DISK);
}
stream_reader_->RegisterCallback(
base::Bind(&DownloadFileImpl::StreamActive, weak_factory_.GetWeakPtr()));
download_start_ = base::TimeTicks::Now();
-
// Initial pull from the straw.
StreamActive();
- BrowserThread::PostTask(
- BrowserThread::UI, FROM_HERE, base::Bind(
- callback, content::DOWNLOAD_INTERRUPT_REASON_NONE));
-
- ++number_active_objects_;
+ return content::DOWNLOAD_INTERRUPT_REASON_NONE;
}
content::DownloadInterruptReason DownloadFileImpl::AppendDataToFile(
const char* data, size_t data_len) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
if (!update_timer_->IsRunning()) {
update_timer_->Start(FROM_HERE,
base::TimeDelta::FromMilliseconds(kUpdatePeriodMs),
@@ -106,8 +91,6 @@ content::DownloadInterruptReason DownloadFileImpl::AppendDataToFile(
void DownloadFileImpl::Rename(const FilePath& full_path,
bool overwrite_existing_file,
const RenameCompletionCallback& callback) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-
FilePath new_path(full_path);
if (!overwrite_existing_file) {
// Make the file unique if requested.
@@ -143,14 +126,6 @@ void DownloadFileImpl::Rename(const FilePath& full_path,
}
void DownloadFileImpl::Detach() {
- // Done here on Windows so that anti-virus scanners invoked by
- // the attachment service actually see the data; see
- // http://crbug.com/127999.
- // Done here for mac because we only want to do this once; see
- // http://crbug.com/13120 for details.
- // Other platforms don't currently do source annotation.
- AnnotateWithSourceInformation();
-
file_.Detach();
}
@@ -188,6 +163,36 @@ std::string DownloadFileImpl::GetHashState() {
return file_.GetHashState();
}
+// DownloadFileInterface implementation.
+void DownloadFileImpl::CancelDownloadRequest() {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ request_handle_->CancelRequest();
+}
+
+int DownloadFileImpl::Id() const {
+ return id_.local();
+}
+
+DownloadManager* DownloadFileImpl::GetDownloadManager() {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ return download_manager_.get();
+}
+
+const DownloadId& DownloadFileImpl::GlobalId() const {
+ return id_;
+}
+
+std::string DownloadFileImpl::DebugString() const {
+ return base::StringPrintf("{"
+ " id_ = " "%d"
+ " request_handle = %s"
+ " Base File = %s"
+ " }",
+ id_.local(),
+ request_handle_->DebugString().c_str(),
+ file_.DebugString().c_str());
+}
+
void DownloadFileImpl::StreamActive() {
base::TimeTicks start(base::TimeTicks::Now());
base::TimeTicks now;
@@ -256,18 +261,18 @@ void DownloadFileImpl::StreamActive() {
download_stats::RecordContiguousWriteTime(now - start);
- // Take care of communication with our observer.
+ // Take care of communication with our controller.
if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
// Error case for both upstream source and file write.
- // Shut down processing and signal an error to our observer.
- // Our observer will clean us up.
+ // Shut down processing and signal an error to our controller.
+ // Our controller will clean us up.
stream_reader_->RegisterCallback(base::Closure());
weak_factory_.InvalidateWeakPtrs();
SendUpdate(); // Make info up to date before error.
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
- base::Bind(&content::DownloadDestinationObserver::DestinationError,
- observer_, reason));
+ base::Bind(&DownloadManager::OnDownloadInterrupted,
+ download_manager_, id_.local(), reason));
} else if (state == content::ByteStreamReader::STREAM_COMPLETE) {
// Signal successful completion and shut down processing.
stream_reader_->RegisterCallback(base::Closure());
@@ -275,12 +280,11 @@ void DownloadFileImpl::StreamActive() {
std::string hash;
if (!GetHash(&hash) || file_.IsEmptyHash(hash))
hash.clear();
- SendUpdate();
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
- base::Bind(
- &content::DownloadDestinationObserver::DestinationCompleted,
- observer_, hash));
+ base::Bind(&DownloadManager::OnResponseCompleted,
+ download_manager_, id_.local(),
+ BytesSoFar(), hash));
}
if (bound_net_log_.IsLoggingAllEvents()) {
bound_net_log_.AddEvent(
@@ -293,12 +297,7 @@ void DownloadFileImpl::StreamActive() {
void DownloadFileImpl::SendUpdate() {
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
- base::Bind(&content::DownloadDestinationObserver::DestinationUpdate,
- observer_, BytesSoFar(), CurrentSpeed(), GetHashState()));
-}
-
-// static
-int content::DownloadFile::GetNumberOfDownloadFiles() {
- return number_active_objects_;
+ base::Bind(&DownloadManager::UpdateDownload,
+ download_manager_, id_.local(),
+ BytesSoFar(), CurrentSpeed(), GetHashState()));
}
-
diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h
index 64ae527..b3630e5 100644
--- a/content/browser/download/download_file_impl.h
+++ b/content/browser/download/download_file_impl.h
@@ -14,14 +14,13 @@
#include "base/timer.h"
#include "content/browser/download/base_file.h"
#include "content/browser/download/byte_stream.h"
-#include "content/public/browser/download_save_info.h"
+#include "content/browser/download/download_request_handle.h"
#include "net/base/net_log.h"
struct DownloadCreateInfo;
namespace content {
class ByteStreamReader;
-class DownloadDestinationObserver;
class DownloadManager;
class PowerSaveBlocker;
}
@@ -30,27 +29,17 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
public:
// Takes ownership of the object pointed to by |request_handle|.
// |bound_net_log| will be used for logging the download file's events.
- // May be constructed on any thread. All methods besides the constructor
- // (including destruction) must occur on the FILE thread.
- //
- // Note that the DownloadFileImpl automatically reads from the passed in
- // stream, and sends updates and status of those reads to the
- // DownloadDestinationObserver.
- DownloadFileImpl(
- const content::DownloadSaveInfo& save_info,
- const GURL& url,
- const GURL& referrer_url,
- int64 received_bytes,
- bool calculate_hash,
- scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
- scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
- base::WeakPtr<content::DownloadDestinationObserver> observer);
-
+ DownloadFileImpl(const DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ DownloadRequestHandleInterface* request_handle,
+ scoped_refptr<content::DownloadManager> download_manager,
+ bool calculate_hash,
+ scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
+ const net::BoundNetLog& bound_net_log);
virtual ~DownloadFileImpl();
// DownloadFile functions.
- virtual void Initialize(const InitializeCallback& callback) OVERRIDE;
+ virtual content::DownloadInterruptReason Initialize() OVERRIDE;
virtual void Rename(const FilePath& full_path,
bool overwrite_existing_file,
const RenameCompletionCallback& callback) OVERRIDE;
@@ -63,6 +52,11 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
virtual int64 CurrentSpeed() const OVERRIDE;
virtual bool GetHash(std::string* hash) OVERRIDE;
virtual std::string GetHashState() OVERRIDE;
+ virtual void CancelDownloadRequest() OVERRIDE;
+ virtual int Id() const OVERRIDE;
+ virtual content::DownloadManager* GetDownloadManager() OVERRIDE;
+ virtual const content::DownloadId& GlobalId() const OVERRIDE;
+ virtual std::string DebugString() const OVERRIDE;
protected:
// For test class overrides.
@@ -86,9 +80,20 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
// with DownloadFile and get rid of BaseFile.
scoped_ptr<content::ByteStreamReader> stream_reader_;
+ // The unique identifier for this download, assigned at creation by
+ // the DownloadFileManager for its internal record keeping.
+ content::DownloadId id_;
+
// Used to trigger progress updates.
scoped_ptr<base::RepeatingTimer<DownloadFileImpl> > update_timer_;
+ // The handle to the request information. Used for operations outside the
+ // download system, specifically canceling a download.
+ scoped_ptr<DownloadRequestHandleInterface> request_handle_;
+
+ // DownloadManager this download belongs to.
+ scoped_refptr<content::DownloadManager> download_manager_;
+
// Statistics
size_t bytes_seen_;
base::TimeDelta disk_writes_time_;
@@ -96,8 +101,6 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile {
net::BoundNetLog bound_net_log_;
- base::WeakPtr<content::DownloadDestinationObserver> observer_;
-
base::WeakPtrFactory<DownloadFileImpl> weak_factory_;
// RAII handle to keep the system from sleeping while we're downloading.
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
new file mode 100644
index 0000000..7bebd92
--- /dev/null
+++ b/content/browser/download/download_file_manager.cc
@@ -0,0 +1,227 @@
+// 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_file_manager.h"
+
+#include <set>
+#include <string>
+
+#include "base/bind.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/utf_string_conversions.h"
+#include "content/browser/download/base_file.h"
+#include "content/browser/download/download_create_info.h"
+#include "content/browser/download/download_file_impl.h"
+#include "content/browser/download/download_interrupt_reasons_impl.h"
+#include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/download_stats.h"
+#include "content/browser/power_save_blocker.h"
+#include "content/browser/web_contents/web_contents_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_manager.h"
+#include "content/public/browser/download_manager_delegate.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/io_buffer.h"
+
+using content::BrowserThread;
+using content::DownloadFile;
+using content::DownloadId;
+using content::DownloadManager;
+
+namespace {
+
+class DownloadFileFactoryImpl
+ : public DownloadFileManager::DownloadFileFactory {
+ public:
+ DownloadFileFactoryImpl() {}
+
+ virtual content::DownloadFile* CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) OVERRIDE;
+};
+
+DownloadFile* DownloadFileFactoryImpl::CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) {
+ return new DownloadFileImpl(
+ info, stream.Pass(), new DownloadRequestHandle(info->request_handle),
+ download_manager, calculate_hash,
+ scoped_ptr<content::PowerSaveBlocker>(
+ new content::PowerSaveBlocker(
+ content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
+ "Download in progress")).Pass(),
+ bound_net_log);
+}
+
+} // namespace
+
+DownloadFileManager::DownloadFileManager(DownloadFileFactory* factory)
+ : download_file_factory_(factory) {
+ if (download_file_factory_ == NULL)
+ download_file_factory_.reset(new DownloadFileFactoryImpl);
+}
+
+DownloadFileManager::~DownloadFileManager() {
+ DCHECK(downloads_.empty());
+}
+
+void DownloadFileManager::Shutdown() {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&DownloadFileManager::OnShutdown, this));
+}
+
+void DownloadFileManager::OnShutdown() {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ STLDeleteValues(&downloads_);
+}
+
+void DownloadFileManager::CreateDownloadFile(
+ scoped_ptr<DownloadCreateInfo> info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ scoped_refptr<DownloadManager> download_manager, bool get_hash,
+ const net::BoundNetLog& bound_net_log,
+ const CreateDownloadFileCallback& callback) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DCHECK(info.get());
+ VLOG(20) << __FUNCTION__ << "()" << " info = " << info->DebugString();
+
+ scoped_ptr<DownloadFile> download_file(download_file_factory_->CreateFile(
+ info.get(), stream.Pass(), download_manager, get_hash, bound_net_log));
+
+ content::DownloadInterruptReason interrupt_reason(
+ download_file->Initialize());
+ if (interrupt_reason == content::DOWNLOAD_INTERRUPT_REASON_NONE) {
+ DCHECK(GetDownloadFile(info->download_id) == NULL);
+ downloads_[info->download_id] = download_file.release();
+ }
+
+ BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+ base::Bind(callback, interrupt_reason));
+}
+
+DownloadFile* DownloadFileManager::GetDownloadFile(
+ DownloadId global_id) {
+ DownloadFileMap::iterator it = downloads_.find(global_id);
+ return it == downloads_.end() ? NULL : it->second;
+}
+
+// This method will be sent via a user action, or shutdown on the UI thread, and
+// run on the download thread. Since this message has been sent from the UI
+// thread, the download may have already completed and won't exist in our map.
+void DownloadFileManager::CancelDownload(DownloadId global_id) {
+ VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id;
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DownloadFileMap::iterator it = downloads_.find(global_id);
+ if (it == downloads_.end())
+ return;
+
+ DownloadFile* download_file = it->second;
+ VLOG(20) << __FUNCTION__ << "()"
+ << " download_file = " << download_file->DebugString();
+ download_file->Cancel();
+
+ EraseDownload(global_id);
+}
+
+void DownloadFileManager::CompleteDownload(
+ DownloadId global_id, const base::Closure& callback) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+ if (!ContainsKey(downloads_, global_id))
+ return;
+
+ DownloadFile* download_file = downloads_[global_id];
+
+ VLOG(20) << " " << __FUNCTION__ << "()"
+ << " id = " << global_id
+ << " download_file = " << download_file->DebugString();
+
+ // Done here on Windows so that anti-virus scanners invoked by
+ // the attachment service actually see the data; see
+ // http://crbug.com/127999.
+ // Done here for mac because we only want to do this once; see
+ // http://crbug.com/13120 for details.
+ // Other platforms don't currently do source annotation.
+ download_file->AnnotateWithSourceInformation();
+
+ download_file->Detach();
+
+ EraseDownload(global_id);
+
+ // Notify our caller we've let it go.
+ BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+}
+
+void DownloadFileManager::OnDownloadManagerShutdown(DownloadManager* manager) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DCHECK(manager);
+
+ std::set<DownloadFile*> to_remove;
+
+ for (DownloadFileMap::iterator i = downloads_.begin();
+ i != downloads_.end(); ++i) {
+ DownloadFile* download_file = i->second;
+ if (download_file->GetDownloadManager() == manager) {
+ download_file->CancelDownloadRequest();
+ to_remove.insert(download_file);
+ }
+ }
+
+ for (std::set<DownloadFile*>::iterator i = to_remove.begin();
+ i != to_remove.end(); ++i) {
+ downloads_.erase((*i)->GlobalId());
+ delete *i;
+ }
+}
+
+// Actions from the UI thread and run on the download thread
+
+void DownloadFileManager::RenameDownloadFile(
+ DownloadId global_id,
+ const FilePath& full_path,
+ bool overwrite_existing_file,
+ const RenameCompletionCallback& callback) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+ DownloadFile* download_file = GetDownloadFile(global_id);
+ if (!download_file) {
+ BrowserThread::PostTask(
+ BrowserThread::UI, FROM_HERE,
+ base::Bind(callback, content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,
+ FilePath()));
+ return;
+ }
+
+ download_file->Rename(full_path, overwrite_existing_file, callback);
+}
+
+int DownloadFileManager::NumberOfActiveDownloads() const {
+ return downloads_.size();
+}
+
+void DownloadFileManager::EraseDownload(DownloadId global_id) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+
+ if (!ContainsKey(downloads_, global_id))
+ return;
+
+ DownloadFile* download_file = downloads_[global_id];
+
+ VLOG(20) << " " << __FUNCTION__ << "()"
+ << " id = " << global_id
+ << " download_file = " << download_file->DebugString();
+
+ downloads_.erase(global_id);
+
+ delete download_file;
+}
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
new file mode 100644
index 0000000..919f1c4
--- /dev/null
+++ b/content/browser/download/download_file_manager.h
@@ -0,0 +1,180 @@
+// 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.
+//
+// The DownloadFileManager owns a set of DownloadFile objects, each of which
+// represent one in progress download and performs the disk IO for that
+// download. The DownloadFileManager itself is a singleton object owned by the
+// ResourceDispatcherHostImpl.
+//
+// The DownloadFileManager uses the file_thread for performing file write
+// operations, in order to avoid disk activity on either the IO (network) thread
+// and the UI thread. It coordinates the notifications from the network and UI.
+//
+// A typical download operation involves multiple threads:
+//
+// Updating an in progress download
+// io_thread
+// |----> data ---->|
+// file_thread (writes to disk)
+// |----> stats ---->|
+// ui_thread (feedback for user and
+// updates to history)
+//
+// Cancel operations perform the inverse order when triggered by a user action:
+// ui_thread (user click)
+// |----> cancel command ---->|
+// file_thread (close file)
+// |----> cancel command ---->|
+// io_thread (stops net IO
+// for download)
+//
+// The DownloadFileManager tracks download requests, mapping from a download
+// ID (unique integer created in the IO thread) to the DownloadManager for the
+// contents (profile) where the download was initiated. In the event of a
+// contents closure during a download, the DownloadFileManager will continue to
+// route data to the appropriate DownloadManager. In progress downloads are
+// cancelled for a DownloadManager that exits (such as when closing a profile).
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_MANAGER_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_MANAGER_H_
+
+#include <map>
+
+#include "base/atomic_sequence_num.h"
+#include "base/basictypes.h"
+#include "base/callback_forward.h"
+#include "base/gtest_prod_util.h"
+#include "base/hash_tables.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/timer.h"
+#include "content/browser/download/download_file.h"
+#include "content/common/content_export.h"
+#include "content/public/browser/download_id.h"
+#include "content/public/browser/download_interrupt_reasons.h"
+#include "net/base/net_errors.h"
+#include "ui/gfx/native_widget_types.h"
+
+struct DownloadCreateInfo;
+class DownloadRequestHandle;
+class FilePath;
+
+namespace content {
+class ByteStreamReader;
+class DownloadId;
+class DownloadManager;
+}
+
+namespace net {
+class BoundNetLog;
+}
+
+// Manages all in progress downloads.
+// Methods are virtual to allow mocks--this class is not intended
+// to be a base class.
+class CONTENT_EXPORT DownloadFileManager
+ : public base::RefCountedThreadSafe<DownloadFileManager> {
+ public:
+ // Callback used with CreateDownloadFile(). |reason| will be
+ // DOWNLOAD_INTERRUPT_REASON_NONE on a successful creation.
+ typedef base::Callback<void(content::DownloadInterruptReason reason)>
+ CreateDownloadFileCallback;
+
+ // Callback used with RenameDownloadFile().
+ typedef content::DownloadFile::RenameCompletionCallback
+ RenameCompletionCallback;
+
+ class DownloadFileFactory {
+ public:
+ virtual ~DownloadFileFactory() {}
+
+ virtual content::DownloadFile* CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) = 0;
+ };
+
+ // Takes ownership of the factory.
+ // Passing in a NULL for |factory| will cause a default
+ // |DownloadFileFactory| to be used.
+ explicit DownloadFileManager(DownloadFileFactory* factory);
+
+ // Create a download file and record it in the download file manager.
+ virtual void CreateDownloadFile(
+ scoped_ptr<DownloadCreateInfo> info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ scoped_refptr<content::DownloadManager> download_manager,
+ bool hash_needed,
+ const net::BoundNetLog& bound_net_log,
+ const CreateDownloadFileCallback& callback);
+
+ // Called on shutdown on the UI thread.
+ virtual void Shutdown();
+
+ // Handlers for notifications sent from the UI thread and run on the
+ // FILE thread. These are both terminal actions with respect to the
+ // download file, as far as the DownloadFileManager is concerned -- if
+ // anything happens to the download file after they are called, it will
+ // be ignored.
+ // We call back to the UI thread in the case of CompleteDownload so that
+ // we know when we can hand the file off to other consumers.
+ virtual void CancelDownload(content::DownloadId id);
+ virtual void CompleteDownload(content::DownloadId id,
+ const base::Closure& callback);
+
+ // Called on FILE thread by DownloadManager at the beginning of its shutdown.
+ virtual void OnDownloadManagerShutdown(content::DownloadManager* manager);
+
+ // Rename the download file, uniquifying if overwrite was not requested.
+ virtual void RenameDownloadFile(
+ content::DownloadId id,
+ const FilePath& full_path,
+ bool overwrite_existing_file,
+ const RenameCompletionCallback& callback);
+
+ // The number of downloads currently active on the DownloadFileManager.
+ // Primarily for testing.
+ virtual int NumberOfActiveDownloads() const;
+
+ void SetFileFactoryForTesting(scoped_ptr<DownloadFileFactory> file_factory) {
+ download_file_factory_.reset(file_factory.release());
+ }
+
+ DownloadFileFactory* GetFileFactoryForTesting() const {
+ return download_file_factory_.get(); // Explicitly NOT a scoped_ptr.
+ }
+
+ protected:
+ virtual ~DownloadFileManager();
+
+ private:
+ friend class base::RefCountedThreadSafe<DownloadFileManager>;
+ friend class DownloadFileManagerTest;
+ friend class DownloadManagerTest;
+ FRIEND_TEST_ALL_PREFIXES(DownloadManagerTest, StartDownload);
+
+ // Clean up helper that runs on the download thread.
+ void OnShutdown();
+
+ // Called only on the download thread.
+ content::DownloadFile* GetDownloadFile(content::DownloadId global_id);
+
+ // Erases the download file with the given the download |id| and removes
+ // it from the maps.
+ void EraseDownload(content::DownloadId global_id);
+
+ typedef base::hash_map<content::DownloadId, content::DownloadFile*>
+ DownloadFileMap;
+
+ // A map of all in progress downloads. It owns the download files.
+ DownloadFileMap downloads_;
+
+ scoped_ptr<DownloadFileFactory> download_file_factory_;
+
+ DISALLOW_COPY_AND_ASSIGN(DownloadFileManager);
+};
+
+#endif // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_FILE_MANAGER_H_
diff --git a/content/browser/download/download_file_manager_unittest.cc b/content/browser/download/download_file_manager_unittest.cc
new file mode 100644
index 0000000..52ce433
--- /dev/null
+++ b/content/browser/download/download_file_manager_unittest.cc
@@ -0,0 +1,413 @@
+// 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_file_manager.h"
+
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/message_loop.h"
+#include "base/scoped_temp_dir.h"
+#include "base/string_number_conversions.h"
+#include "content/browser/browser_thread_impl.h"
+#include "content/browser/download/byte_stream.h"
+#include "content/browser/download/download_create_info.h"
+#include "content/browser/download/download_interrupt_reasons_impl.h"
+#include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/mock_download_file.h"
+#include "content/public/browser/download_id.h"
+#include "content/public/test/mock_download_manager.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using content::BrowserThread;
+using content::BrowserThreadImpl;
+using content::DownloadId;
+using content::MockDownloadManager;
+
+using ::testing::_;
+using ::testing::AtLeast;
+using ::testing::Mock;
+using ::testing::Return;
+using ::testing::SaveArg;
+using ::testing::StrictMock;
+using ::testing::StrEq;
+
+namespace {
+
+// MockDownloadManager with the addition of a mock callback used for testing.
+class TestDownloadManager : public MockDownloadManager {
+ public:
+ MOCK_METHOD3(OnDownloadRenamed,
+ void(int download_id,
+ content::DownloadInterruptReason reason,
+ const FilePath& full_path));
+ private:
+ ~TestDownloadManager() {}
+};
+
+class MockDownloadFileFactory :
+ public DownloadFileManager::DownloadFileFactory {
+
+ public:
+ MockDownloadFileFactory() {}
+ virtual ~MockDownloadFileFactory() {}
+
+ virtual content::DownloadFile* CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) OVERRIDE;
+
+ MockDownloadFile* GetExistingFile(const DownloadId& id);
+
+ private:
+ std::map<DownloadId, MockDownloadFile*> files_;
+};
+
+content::DownloadFile* MockDownloadFileFactory::CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) {
+ DCHECK(files_.end() == files_.find(info->download_id));
+ MockDownloadFile* created_file = new StrictMock<MockDownloadFile>();
+ files_[info->download_id] = created_file;
+
+ ON_CALL(*created_file, GetDownloadManager())
+ .WillByDefault(Return(download_manager));
+ EXPECT_CALL(*created_file, Initialize());
+
+ return created_file;
+}
+
+MockDownloadFile* MockDownloadFileFactory::GetExistingFile(
+ const DownloadId& id) {
+ DCHECK(files_.find(id) != files_.end());
+ return files_[id];
+}
+
+class MockDownloadRequestHandle : public DownloadRequestHandle {
+ public:
+ MockDownloadRequestHandle(content::DownloadManager* manager)
+ : manager_(manager) {}
+
+ virtual content::DownloadManager* GetDownloadManager() const OVERRIDE;
+
+ private:
+
+ content::DownloadManager* manager_;
+};
+
+content::DownloadManager* MockDownloadRequestHandle::GetDownloadManager()
+ const {
+ return manager_;
+}
+
+void NullCallback() { }
+
+} // namespace
+
+class DownloadFileManagerTest : public testing::Test {
+ public:
+ // State of renamed file. Used with RenameFile().
+ enum RenameFileState {
+ IN_PROGRESS,
+ COMPLETE
+ };
+
+ // Whether to overwrite the target filename in RenameFile().
+ enum RenameFileOverwrite {
+ OVERWRITE,
+ DONT_OVERWRITE
+ };
+
+ static const char* kTestData1;
+ static const char* kTestData2;
+ static const char* kTestData3;
+ static const char* kTestData4;
+ static const char* kTestData5;
+ static const char* kTestData6;
+ static const int32 kDummyDownloadId;
+ static const int32 kDummyDownloadId2;
+ static const int kDummyChildId;
+ static const int kDummyRequestId;
+
+ // We need a UI |BrowserThread| in order to destruct |download_manager_|,
+ // which has trait |BrowserThread::DeleteOnUIThread|. Without this,
+ // calling Release() on |download_manager_| won't ever result in its
+ // destructor being called and we get a leak.
+ DownloadFileManagerTest()
+ : last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
+ ui_thread_(BrowserThread::UI, &loop_),
+ file_thread_(BrowserThread::FILE, &loop_) {
+ }
+
+ ~DownloadFileManagerTest() {
+ }
+
+ virtual void SetUp() {
+ download_manager_ = new TestDownloadManager();
+ request_handle_.reset(new MockDownloadRequestHandle(download_manager_));
+ download_file_factory_ = new MockDownloadFileFactory;
+ download_file_manager_ = new DownloadFileManager(download_file_factory_);
+ }
+
+ virtual void TearDown() {
+ // When a DownloadManager's reference count drops to 0, it is not
+ // deleted immediately. Instead, a task is posted to the UI thread's
+ // message loop to delete it.
+ // So, drop the reference count to 0 and run the message loop once
+ // to ensure that all resources are cleaned up before the test exits.
+ download_manager_ = NULL;
+ ui_thread_.message_loop()->RunAllPending();
+ }
+
+ void ProcessAllPendingMessages() {
+ loop_.RunAllPending();
+ }
+
+ // Clears all gmock expectations for the download file |id| and the manager.
+ void ClearExpectations(DownloadId id) {
+ MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
+ Mock::VerifyAndClearExpectations(file);
+ Mock::VerifyAndClearExpectations(download_manager_);
+ }
+
+ void OnDownloadFileCreated(content::DownloadInterruptReason reason) {
+ last_reason_ = reason;
+ }
+
+ // Create a download item on the DFM.
+ // |info| is the information needed to create a new download file.
+ // |id| is the download ID of the new download file.
+ void CreateDownloadFile(scoped_ptr<DownloadCreateInfo> info) {
+ // Mostly null out args; they'll be passed to MockDownloadFileFactory
+ // to be ignored anyway.
+ download_file_manager_->CreateDownloadFile(
+ info.Pass(), scoped_ptr<content::ByteStreamReader>(),
+ download_manager_, true, net::BoundNetLog(),
+ base::Bind(&DownloadFileManagerTest::OnDownloadFileCreated,
+ // The test jig will outlive all download files.
+ base::Unretained(this)));
+
+ // Anything that isn't DOWNLOAD_INTERRUPT_REASON_NONE.
+ last_reason_ = content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED;
+ ProcessAllPendingMessages();
+ EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, last_reason_);
+ }
+
+ // Renames the download file.
+ // |id| is the download ID of the download file.
+ // |new_path| is the new file path.
+ // |unique_path| is the actual path that the download file will be
+ // renamed to. If there is an existing file at
+ // |new_path| and |replace| is false, then |new_path|
+ // will be uniquified.
+ // |rename_error| is the error to inject. For no error,
+ // use content::DOWNLOAD_INTERRUPT_REASON_NONE.
+ // |state| whether we are renaming an in-progress download or a
+ // completed download.
+ // |should_overwrite| indicates whether to replace or uniquify the file.
+ void RenameFile(const DownloadId& id,
+ const FilePath& new_path,
+ bool should_overwrite) {
+ MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
+ ASSERT_TRUE(file != NULL);
+ content::DownloadFile::RenameCompletionCallback rename_callback;
+
+ EXPECT_CALL(*file, Rename(new_path, should_overwrite, _))
+ .WillOnce(SaveArg<2>(&rename_callback));
+
+ content::DownloadFile::RenameCompletionCallback passed_callback(
+ base::Bind(&TestDownloadManager::OnDownloadRenamed,
+ download_manager_, id.local()));
+
+ download_file_manager_->RenameDownloadFile(
+ id, new_path, should_overwrite, passed_callback);
+
+ EXPECT_TRUE(rename_callback.Equals(passed_callback));
+ }
+
+ void Complete(DownloadId id) {
+ MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
+ ASSERT_TRUE(file != NULL);
+
+ EXPECT_CALL(*file, AnnotateWithSourceInformation())
+ .WillOnce(Return());
+ EXPECT_CALL(*file, Detach())
+ .WillOnce(Return());
+ int num_downloads = download_file_manager_->NumberOfActiveDownloads();
+ EXPECT_LT(0, num_downloads);
+ download_file_manager_->CompleteDownload(id, base::Bind(NullCallback));
+ EXPECT_EQ(num_downloads - 1,
+ download_file_manager_->NumberOfActiveDownloads());
+ EXPECT_EQ(NULL, download_file_manager_->GetDownloadFile(id));
+ }
+
+ void CleanUp(DownloadId id) {
+ // Expected calls:
+ // DownloadFileManager::CancelDownload
+ // DownloadFile::Cancel
+ // DownloadFileManager::EraseDownload
+ // if no more downloads:
+ // DownloadFileManager::StopUpdateTimer
+ MockDownloadFile* file = download_file_factory_->GetExistingFile(id);
+ ASSERT_TRUE(file != NULL);
+
+ EXPECT_CALL(*file, Cancel());
+
+ download_file_manager_->CancelDownload(id);
+
+ EXPECT_EQ(NULL, download_file_manager_->GetDownloadFile(id));
+ }
+
+ protected:
+ scoped_refptr<TestDownloadManager> download_manager_;
+ scoped_ptr<MockDownloadRequestHandle> request_handle_;
+ MockDownloadFileFactory* download_file_factory_;
+ scoped_refptr<DownloadFileManager> download_file_manager_;
+
+ // Error from creating download file.
+ content::DownloadInterruptReason last_reason_;
+
+ // Per-download statistics.
+ std::map<DownloadId, int64> byte_count_;
+ std::map<DownloadId, int> error_count_;
+
+ private:
+ MessageLoop loop_;
+
+ // UI thread.
+ BrowserThreadImpl ui_thread_;
+
+ // File thread to satisfy debug checks in DownloadFile.
+ BrowserThreadImpl file_thread_;
+};
+
+const char* DownloadFileManagerTest::kTestData1 =
+ "Let's write some data to the file!\n";
+const char* DownloadFileManagerTest::kTestData2 = "Writing more data.\n";
+const char* DownloadFileManagerTest::kTestData3 = "Final line.";
+const char* DownloadFileManagerTest::kTestData4 = "Writing, writing, writing\n";
+const char* DownloadFileManagerTest::kTestData5 = "All we do is writing,\n";
+const char* DownloadFileManagerTest::kTestData6 = "Rawhide!";
+
+const int32 DownloadFileManagerTest::kDummyDownloadId = 23;
+const int32 DownloadFileManagerTest::kDummyDownloadId2 = 77;
+const int DownloadFileManagerTest::kDummyChildId = 3;
+const int DownloadFileManagerTest::kDummyRequestId = 67;
+
+TEST_F(DownloadFileManagerTest, Cancel) {
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+
+ CreateDownloadFile(info.Pass());
+
+ CleanUp(dummy_id);
+}
+
+TEST_F(DownloadFileManagerTest, Complete) {
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+
+ CreateDownloadFile(info.Pass());
+
+ Complete(dummy_id);
+}
+
+TEST_F(DownloadFileManagerTest, Rename) {
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+ ScopedTempDir download_dir;
+ ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+
+ CreateDownloadFile(info.Pass());
+
+ FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
+ RenameFile(dummy_id, foo, true);
+
+ CleanUp(dummy_id);
+}
+
+TEST_F(DownloadFileManagerTest, RenameNoOverwrite) {
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+ ScopedTempDir download_dir;
+ ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+
+ CreateDownloadFile(info.Pass());
+
+ FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
+ RenameFile(dummy_id, foo, false);
+
+ CleanUp(dummy_id);
+}
+
+TEST_F(DownloadFileManagerTest, RenameTwice) {
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+ ScopedTempDir download_dir;
+ ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+
+ CreateDownloadFile(info.Pass());
+
+ FilePath crfoo(download_dir.path().Append(
+ FILE_PATH_LITERAL("foo.txt.crdownload")));
+ RenameFile(dummy_id, crfoo, true);
+
+ FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
+ RenameFile(dummy_id, foo, true);
+
+ CleanUp(dummy_id);
+}
+
+TEST_F(DownloadFileManagerTest, TwoDownloads) {
+ // Same as StartDownload, at first.
+ scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo);
+ DownloadId dummy_id(download_manager_.get(), kDummyDownloadId);
+ info->download_id = dummy_id;
+ scoped_ptr<DownloadCreateInfo> info2(new DownloadCreateInfo);
+ DownloadId dummy_id2(download_manager_.get(), kDummyDownloadId2);
+ info2->download_id = dummy_id2;
+ ScopedTempDir download_dir;
+ ASSERT_TRUE(download_dir.CreateUniqueTempDir());
+
+ CreateDownloadFile(info.Pass());
+ CreateDownloadFile(info2.Pass());
+
+ FilePath crbar(download_dir.path().Append(
+ FILE_PATH_LITERAL("bar.txt.crdownload")));
+ RenameFile(dummy_id2, crbar, true);
+
+ FilePath crfoo(download_dir.path().Append(
+ FILE_PATH_LITERAL("foo.txt.crdownload")));
+ RenameFile(dummy_id, crfoo, true);
+
+
+ FilePath bar(download_dir.path().Append(FILE_PATH_LITERAL("bar.txt")));
+ RenameFile(dummy_id2, bar, true);
+
+ CleanUp(dummy_id2);
+
+ FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt")));
+ RenameFile(dummy_id, foo, true);
+
+ CleanUp(dummy_id);
+}
+
+// TODO(ahendrickson) -- A test for download manager shutdown.
+// Expected call sequence:
+// OnDownloadManagerShutdown
+// DownloadFile::GetDownloadManager
+// DownloadFile::CancelDownloadRequest
+// DownloadFile::~DownloadFile
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index 78da487..24eaade 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -12,7 +12,6 @@
#include "content/browser/download/download_file_impl.h"
#include "content/browser/download/download_request_handle.h"
#include "content/browser/power_save_blocker.h"
-#include "content/public/browser/download_destination_observer.h"
#include "content/public/browser/download_interrupt_reasons.h"
#include "content/public/browser/download_manager.h"
#include "content/public/test/mock_download_manager.h"
@@ -26,6 +25,7 @@ using content::BrowserThread;
using content::BrowserThreadImpl;
using content::DownloadFile;
using content::DownloadId;
+using content::DownloadManager;
using ::testing::_;
using ::testing::AnyNumber;
using ::testing::DoAll;
@@ -48,21 +48,14 @@ class MockByteStreamReader : public content::ByteStreamReader {
MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
};
-class MockDownloadDestinationObserver
- : public content::DownloadDestinationObserver {
+class LocalMockDownloadManager : public content::MockDownloadManager {
public:
- MOCK_METHOD3(DestinationUpdate, void(int64, int64, const std::string&));
- MOCK_METHOD1(DestinationError, void(content::DownloadInterruptReason));
- MOCK_METHOD1(DestinationCompleted, void(const std::string&));
-
- // Doesn't override any methods in the base class. Used to make sure
- // that the last DestinationUpdate before a Destination{Completed,Error}
- // had the right values.
- MOCK_METHOD3(CurrentUpdateStatus,
- void(int64, int64, const std::string&));
+ MOCK_METHOD4(CurrentUpdateStatus,
+ void(int32, int64, int64, const std::string&));
+ protected:
+ virtual ~LocalMockDownloadManager() {}
};
-
MATCHER(IsNullCallback, "") { return (arg.is_null()); }
} // namespace
@@ -80,9 +73,12 @@ class DownloadFileTest : public testing::Test {
static const int kDummyChildId;
static const int kDummyRequestId;
+ // We need a UI |BrowserThread| in order to destruct |download_manager_|,
+ // which has trait |BrowserThread::DeleteOnUIThread|. Without this,
+ // calling Release() on |download_manager_| won't ever result in its
+ // destructor being called and we get a leak.
DownloadFileTest() :
- observer_(new StrictMock<MockDownloadDestinationObserver>),
- observer_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(observer_.get())),
+ update_download_id_(-1),
bytes_(-1),
bytes_per_sec_(-1),
hash_state_("xyzzy"),
@@ -93,35 +89,44 @@ class DownloadFileTest : public testing::Test {
~DownloadFileTest() {
}
- void SetUpdateDownloadInfo(int64 bytes, int64 bytes_per_sec,
+ void SetUpdateDownloadInfo(int32 id, int64 bytes, int64 bytes_per_sec,
const std::string& hash_state) {
+ update_download_id_ = id;
bytes_ = bytes;
bytes_per_sec_ = bytes_per_sec;
hash_state_ = hash_state;
}
void ConfirmUpdateDownloadInfo() {
- observer_->CurrentUpdateStatus(bytes_, bytes_per_sec_, hash_state_);
+ download_manager_->CurrentUpdateStatus(
+ update_download_id_, bytes_, bytes_per_sec_, hash_state_);
}
virtual void SetUp() {
- EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))
+ download_manager_ = new StrictMock<LocalMockDownloadManager>;
+ EXPECT_CALL(*(download_manager_.get()),
+ UpdateDownload(
+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
+ _, _, _))
.Times(AnyNumber())
.WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));
}
+ virtual void TearDown() {
+ // When a DownloadManager's reference count drops to 0, it is not
+ // deleted immediately. Instead, a task is posted to the UI thread's
+ // message loop to delete it.
+ // So, drop the reference count to 0 and run the message loop once
+ // to ensure that all resources are cleaned up before the test exits.
+ download_manager_ = NULL;
+ ui_thread_.message_loop()->RunAllPending();
+ }
+
// Mock calls to this function are forwarded here.
void RegisterCallback(base::Closure sink_callback) {
sink_callback_ = sink_callback;
}
- void SetInterruptReasonCallback(bool* was_called,
- content::DownloadInterruptReason* reason_p,
- content::DownloadInterruptReason reason) {
- *was_called = true;
- *reason_p = reason;
- }
-
virtual bool CreateDownloadFile(int offset, bool calculate_hash) {
// There can be only one.
DCHECK(!download_file_.get());
@@ -134,36 +139,30 @@ class DownloadFileTest : public testing::Test {
.WillOnce(Invoke(this, &DownloadFileTest::RegisterCallback))
.RetiresOnSaturation();
+ DownloadCreateInfo info;
+ // info.request_handle default constructed to null.
+ info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);
+ info.save_info.file_stream = file_stream_;
download_file_.reset(
new DownloadFileImpl(
- content::DownloadSaveInfo(),
- GURL(), // Source
- GURL(), // Referrer
- 0, // Received bytes
- calculate_hash,
- scoped_ptr<content::ByteStreamReader>(input_stream_),
- net::BoundNetLog(),
+ &info,
+ scoped_ptr<content::ByteStreamReader>(input_stream_).Pass(),
+ new DownloadRequestHandle(),
+ download_manager_, calculate_hash,
scoped_ptr<content::PowerSaveBlocker>(NULL).Pass(),
- observer_factory_.GetWeakPtr()));
+ net::BoundNetLog()));
EXPECT_CALL(*input_stream_, Read(_, _))
.WillOnce(Return(content::ByteStreamReader::STREAM_EMPTY))
.RetiresOnSaturation();
-
- base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);
- bool called = false;
- content::DownloadInterruptReason result;
- download_file_->Initialize(base::Bind(
- &DownloadFileTest::SetInterruptReasonCallback,
- weak_ptr_factory.GetWeakPtr(), &called, &result));
- loop_.RunAllPending();
- EXPECT_TRUE(called);
-
+ content::DownloadInterruptReason result = download_file_->Initialize();
::testing::Mock::VerifyAndClearExpectations(input_stream_);
return result == content::DOWNLOAD_INTERRUPT_REASON_NONE;
}
virtual void DestroyDownloadFile(int offset) {
+ EXPECT_EQ(kDummyDownloadId + offset, download_file_->Id());
+ EXPECT_EQ(download_manager_, download_file_->GetDownloadManager());
EXPECT_FALSE(download_file_->InProgress());
EXPECT_EQ(static_cast<int64>(expected_data_.size()),
download_file_->BytesSoFar());
@@ -233,16 +232,19 @@ class DownloadFileTest : public testing::Test {
}
void FinishStream(content::DownloadInterruptReason interrupt_reason,
- bool check_observer) {
+ bool check_download_manager) {
::testing::Sequence s1;
SetupFinishStream(interrupt_reason, s1);
sink_callback_.Run();
VerifyStreamAndSize();
- if (check_observer) {
- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
+ if (check_download_manager) {
+ EXPECT_CALL(*download_manager_, OnResponseCompleted(_, _, _));
loop_.RunAllPending();
- ::testing::Mock::VerifyAndClearExpectations(observer_.get());
- EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))
+ ::testing::Mock::VerifyAndClearExpectations(download_manager_.get());
+ EXPECT_CALL(*(download_manager_.get()),
+ UpdateDownload(
+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
+ _, _, _))
.Times(AnyNumber())
.WillRepeatedly(Invoke(this,
&DownloadFileTest::SetUpdateDownloadInfo));
@@ -270,9 +272,7 @@ class DownloadFileTest : public testing::Test {
}
protected:
- scoped_ptr<StrictMock<MockDownloadDestinationObserver> > observer_;
- base::WeakPtrFactory<content::DownloadDestinationObserver>
- observer_factory_;
+ scoped_refptr<StrictMock<LocalMockDownloadManager> > download_manager_;
linked_ptr<net::FileStream> file_stream_;
@@ -286,7 +286,8 @@ class DownloadFileTest : public testing::Test {
// Sink callback data for stream.
base::Closure sink_callback_;
- // Latest update sent to the observer.
+ // Latest update sent to the download manager.
+ int32 update_download_id_;
int64 bytes_;
int64 bytes_per_sec_;
std::string hash_state_;
@@ -487,12 +488,21 @@ TEST_F(DownloadFileTest, StreamEmptySuccess) {
// Test that calling the sink_callback_ on an empty stream shouldn't
// do anything.
AppendDataToFile(NULL, 0);
+ ::testing::Mock::VerifyAndClearExpectations(download_manager_.get());
+ EXPECT_CALL(*(download_manager_.get()),
+ UpdateDownload(
+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
+ _, _, _))
+ .Times(AnyNumber())
+ .WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));
// Finish the download this way and make sure we see it on the
- // observer.
- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
+ // DownloadManager.
+ EXPECT_CALL(*(download_manager_.get()),
+ OnResponseCompleted(DownloadId(kValidIdDomain,
+ kDummyDownloadId + 0).local(),
+ 0, _));
FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, false);
- loop_.RunAllPending();
DestroyDownloadFile(0);
}
@@ -503,9 +513,10 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
EXPECT_TRUE(file_util::PathExists(initial_path));
// Finish the download in error and make sure we see it on the
- // observer.
- EXPECT_CALL(*(observer_.get()),
- DestinationError(
+ // DownloadManager.
+ EXPECT_CALL(*(download_manager_.get()),
+ OnDownloadInterrupted(
+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
.WillOnce(InvokeWithoutArgs(
this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
@@ -515,7 +526,8 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
// the last one may have the correct information even if the failure
// doesn't produce an update, as the timer update may have triggered at the
// same time.
- EXPECT_CALL(*(observer_.get()), CurrentUpdateStatus(0, _, _));
+ EXPECT_CALL(*(download_manager_.get()),
+ CurrentUpdateStatus(kDummyDownloadId + 0, 0, _, _));
FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false);
@@ -533,10 +545,13 @@ TEST_F(DownloadFileTest, StreamNonEmptySuccess) {
::testing::Sequence s1;
SetupDataAppend(chunks1, 2, s1);
SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, s1);
- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));
+ EXPECT_CALL(*(download_manager_.get()),
+ OnResponseCompleted(DownloadId(kValidIdDomain,
+ kDummyDownloadId + 0).local(),
+ strlen(kTestData1) + strlen(kTestData2),
+ _));
sink_callback_.Run();
VerifyStreamAndSize();
- loop_.RunAllPending();
DestroyDownloadFile(0);
}
@@ -551,8 +566,9 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED,
s1);
- EXPECT_CALL(*(observer_.get()),
- DestinationError(
+ EXPECT_CALL(*(download_manager_.get()),
+ OnDownloadInterrupted(
+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))
.WillOnce(InvokeWithoutArgs(
this, &DownloadFileTest::ConfirmUpdateDownloadInfo));
@@ -562,8 +578,9 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
// the last one may have the correct information even if the failure
// doesn't produce an update, as the timer update may have triggered at the
// same time.
- EXPECT_CALL(*(observer_.get()),
- CurrentUpdateStatus(strlen(kTestData1) + strlen(kTestData2),
+ EXPECT_CALL(*(download_manager_.get()),
+ CurrentUpdateStatus(kDummyDownloadId + 0,
+ strlen(kTestData1) + strlen(kTestData2),
_, _));
sink_callback_.Run();
@@ -573,7 +590,7 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
}
// Send some data, wait 3/4s of a second, run the message loop, and
-// confirm the values the observer received are correct.
+// confirm the values the DownloadManager received are correct.
TEST_F(DownloadFileTest, ConfirmUpdate) {
CreateDownloadFile(0, true);
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 329717d..2d0482c 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -19,6 +19,7 @@
#include "base/utf_string_conversions.h"
#include "content/browser/download/download_create_info.h"
#include "content/browser/download/download_file.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_interrupt_reasons_impl.h"
#include "content/browser/download/download_item_impl_delegate.h"
#include "content/browser/download/download_request_handle.h"
@@ -118,19 +119,6 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
}
};
-// Wrapper around DownloadFile::Detach and DownloadFile::Cancel that
-// takes ownership of the DownloadFile and hence implicitly destroys it
-// at the end of the function.
-static void DownloadFileDetach(scoped_ptr<DownloadFile> download_file) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
- download_file->Detach();
-}
-
-static void DownloadFileCancel(scoped_ptr<DownloadFile> download_file) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
- download_file->Cancel();
-}
-
} // namespace
namespace content {
@@ -304,22 +292,11 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
DownloadItemImpl::~DownloadItemImpl() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
- // Should always have been nuked before now, at worst in
- // DownloadManager shutdown.
- DCHECK(!download_file_.get());
-
FOR_EACH_OBSERVER(Observer, observers_, OnDownloadDestroyed(this));
delegate_->AssertStateConsistent(this);
delegate_->Detach();
}
-base::WeakPtr<content::DownloadDestinationObserver>
-DownloadItemImpl::DestinationObserverAsWeakPtr() {
- // Return does private downcast.
- return weak_ptr_factory_.GetWeakPtr();
-}
-
void DownloadItemImpl::AddObserver(Observer* observer) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -408,6 +385,21 @@ void DownloadItemImpl::DangerousDownloadValidated() {
delegate_->MaybeCompleteDownload(this);
}
+void DownloadItemImpl::ProgressComplete(int64 bytes_so_far,
+ const std::string& final_hash) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ hash_ = final_hash;
+ hash_state_ = "";
+
+ received_bytes_ = bytes_so_far;
+
+ // If we've received more data than we were expecting (bad server info?),
+ // revert to 'unknown size mode'.
+ if (received_bytes_ > total_bytes_)
+ total_bytes_ = 0;
+}
+
// Updates from the download thread may have been posted while this download
// was being cancelled in the UI thread, so we'll accept them unless we're
// complete.
@@ -464,35 +456,10 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
TransitionTo(CANCELLED);
-
- // Cancel and remove the download file.
- DCHECK(download_file_.get());
- BrowserThread::PostTask(
- BrowserThread::FILE, FROM_HERE,
- // Will be deleted at end of task execution.
- base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
-
- // Cancel the originating URL request.
- request_handle_->CancelRequest();
-
if (user_cancel)
delegate_->DownloadStopped(this);
}
-// We're starting the download.
-void DownloadItemImpl::Start(scoped_ptr<content::DownloadFile> download_file) {
- DCHECK(!download_file_.get());
- download_file_ = download_file.Pass();
-
- BrowserThread::PostTask(
- BrowserThread::FILE, FROM_HERE,
- base::Bind(&DownloadFile::Initialize,
- // Safe because we control download file lifetime.
- base::Unretained(download_file_.get()),
- base::Bind(&DownloadItemImpl::OnDownloadFileInitialized,
- weak_ptr_factory_.GetWeakPtr())));
-}
-
// An error occurred somewhere.
void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
// Somewhat counter-intuitively, it is possible for us to receive an
@@ -510,17 +477,6 @@ void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) {
last_reason_ = reason;
TransitionTo(INTERRUPTED);
-
- // Cancel and remove the download file.
- DCHECK(download_file_.get());
- BrowserThread::PostTask(
- BrowserThread::FILE, FROM_HERE,
- // Will be deleted at end of task execution.
- base::Bind(&DownloadFileCancel, base::Passed(download_file_.Pass())));
-
- // Cancel the originating URL request.
- request_handle_->CancelRequest();
-
download_stats::RecordDownloadInterrupted(
reason, received_bytes_, total_bytes_);
delegate_->DownloadStopped(this);
@@ -540,17 +496,12 @@ void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) {
}
void DownloadItemImpl::OnAllDataSaved(
- const std::string& final_hash) {
+ int64 size, const std::string& final_hash) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- DCHECK_EQ(IN_PROGRESS, state_);
DCHECK(!all_data_saved_);
all_data_saved_ = true;
-
- // Store final hash and null out intermediate serialized hash state.
- hash_ = final_hash;
- hash_state_ = "";
-
+ ProgressComplete(size, final_hash);
UpdateObservers();
}
@@ -745,32 +696,29 @@ void DownloadItemImpl::TogglePause() {
void DownloadItemImpl::OnDownloadCompleting() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- if (!IsInProgress())
- return;
-
VLOG(20) << __FUNCTION__ << "()"
<< " needs rename = " << NeedsRename()
<< " " << DebugString(true);
DCHECK(!GetTargetName().empty());
DCHECK_NE(DANGEROUS, GetSafetyState());
- DCHECK(download_file_.get());
if (NeedsRename()) {
- content::DownloadFile::RenameCompletionCallback callback =
+ DownloadFileManager::RenameCompletionCallback callback =
base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName,
weak_ptr_factory_.GetWeakPtr());
BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
- base::Bind(&DownloadFile::Rename,
- base::Unretained(download_file_.get()),
+ base::Bind(&DownloadFileManager::RenameDownloadFile,
+ delegate_->GetDownloadFileManager(), GetGlobalId(),
GetTargetFilePath(), true, callback));
} else {
// Complete the download and release the DownloadFile.
- BrowserThread::PostTaskAndReply(
+ BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
- base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
- base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
- weak_ptr_factory_.GetWeakPtr()));
+ base::Bind(&DownloadFileManager::CompleteDownload,
+ delegate_->GetDownloadFileManager(), GetGlobalId(),
+ base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
+ weak_ptr_factory_.GetWeakPtr())));
}
}
@@ -779,9 +727,6 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
const FilePath& full_path) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- if (!IsInProgress())
- return;
-
VLOG(20) << __FUNCTION__ << "()"
<< " full_path = \"" << full_path.value() << "\""
<< " needed rename = " << NeedsRename()
@@ -800,33 +745,12 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
delegate_->DownloadRenamedToFinalName(this);
// Complete the download and release the DownloadFile.
- // TODO(rdsmith): Unify this path with the !NeedsRename() path in
- // OnDownloadCompleting above. This can happen easily after history
- // is made into an observer and the path accessors are cleaned up;
- // that should allow OnDownloadCompleting to simply call
- // OnDownloadRenamedToFinalName directly.
- DCHECK(download_file_.get());
- BrowserThread::PostTaskAndReply(
+ BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
- base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())),
- base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
- weak_ptr_factory_.GetWeakPtr()));
-}
-
-void DownloadItemImpl::OnDownloadFileInitialized(
- content::DownloadInterruptReason result) {
- if (result != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
- Interrupt(result);
- // TODO(rdsmith): It makes no sense to continue along the
- // regular download path after we've gotten an error. But it's
- // the way the code has historically worked, and this allows us
- // to get the download persisted and observers of the download manager
- // notified, so tests work. When we execute all side effects of cancel
- // (including queue removal) immedately rather than waiting for
- // persistence we should replace this comment with a "return;".
- }
-
- delegate_->DelegateStart(this);
+ base::Bind(&DownloadFileManager::CompleteDownload,
+ delegate_->GetDownloadFileManager(), GetGlobalId(),
+ base::Bind(&DownloadItemImpl::OnDownloadFileReleased,
+ weak_ptr_factory_.GetWeakPtr())));
}
void DownloadItemImpl::OnDownloadFileReleased() {
@@ -957,30 +881,18 @@ void DownloadItemImpl::OnDownloadTargetDetermined(
// space/permission/availability constraints.
DCHECK(intermediate_path.DirName() == target_path.DirName());
- if (!IsInProgress()) {
- // If we've been cancelled or interrupted while the target was being
- // determined, continue the cascade with a null name.
- // The error doesn't matter as the cause of download stoppaged
- // will already have been recorded.
- OnDownloadRenamedToIntermediateName(
- content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED, FilePath());
- return;
- }
-
// Rename to intermediate name.
// TODO(asanka): Skip this rename if AllDataSaved() is true. This avoids a
// spurious rename when we can just rename to the final
// filename. Unnecessary renames may cause bugs like
// http://crbug.com/74187.
- DCHECK(download_file_.get());
- DownloadFile::RenameCompletionCallback callback =
+ DownloadFileManager::RenameCompletionCallback callback =
base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName,
weak_ptr_factory_.GetWeakPtr());
BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
- base::Bind(&DownloadFile::Rename,
- // Safe because we control download file lifetime.
- base::Unretained(download_file_.get()),
+ base::Bind(&DownloadFileManager::RenameDownloadFile,
+ delegate_->GetDownloadFileManager(), GetGlobalId(),
intermediate_path, false, callback));
}
@@ -1011,53 +923,14 @@ FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
GetTargetFilePath() : GetFullPath();
}
-void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
- int64 bytes_per_sec,
- const std::string& hash_state) {
+void DownloadItemImpl::OffThreadCancel() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ request_handle_->CancelRequest();
- if (!IsInProgress()) {
- // Ignore if we're no longer in-progress. This can happen if we race a
- // Cancel on the UI thread with an update on the FILE thread.
- //
- // TODO(rdsmith): Arguably we should let this go through, as this means
- // the download really did get further than we know before it was
- // cancelled. But the gain isn't very large, and the code is more
- // fragile if it has to support in progress updates in a non-in-progress
- // state. This issue should be readdressed when we revamp performance
- // reporting.
- return;
- }
- bytes_per_sec_ = bytes_per_sec;
- hash_state_ = hash_state;
- received_bytes_ = bytes_so_far;
-
- // If we've received more data than we were expecting (bad server info?),
- // revert to 'unknown size mode'.
- if (received_bytes_ > total_bytes_)
- total_bytes_ = 0;
-
- if (bound_net_log_.IsLoggingAllEvents()) {
- bound_net_log_.AddEvent(
- net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
- net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
- }
-
- UpdateObservers();
-}
-
-void DownloadItemImpl::DestinationError(
- content::DownloadInterruptReason reason) {
- // The DestinationError and Interrupt routines are being kept separate
- // to allow for a future merging of the Cancel and Interrupt routines..
- Interrupt(reason);
-}
-
-void DownloadItemImpl::DestinationCompleted(const std::string& final_hash) {
- if (!IsInProgress())
- return;
- OnAllDataSaved(final_hash);
- delegate_->MaybeCompleteDownload(this);
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&DownloadFileManager::CancelDownload,
+ delegate_->GetDownloadFileManager(), download_id_));
}
void DownloadItemImpl::Init(bool active,
@@ -1167,8 +1040,7 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
" etag = '%s'"
" url_chain = \n\t\"%s\"\n\t"
" full_path = \"%" PRFilePath "\""
- " target_path = \"%" PRFilePath "\""
- " has download file = %s",
+ " target_path = \"%" PRFilePath "\"",
GetDbHandle(),
GetTotalBytes(),
GetReceivedBytes(),
@@ -1179,8 +1051,7 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
GetETag().c_str(),
url_list.c_str(),
GetFullPath().value().c_str(),
- GetTargetFilePath().value().c_str(),
- download_file_.get() ? "true" : "false");
+ GetTargetFilePath().value().c_str());
} else {
description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
}
diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h
index 3b7a3aa..efd3ca2 100644
--- a/content/browser/download/download_item_impl.h
+++ b/content/browser/download/download_item_impl.h
@@ -17,7 +17,6 @@
#include "content/browser/download/download_net_log_parameters.h"
#include "content/browser/download/download_request_handle.h"
#include "content/common/content_export.h"
-#include "content/public/browser/download_destination_observer.h"
#include "content/public/browser/download_id.h"
#include "content/public/browser/download_item.h"
#include "googleurl/src/gurl.h"
@@ -26,14 +25,8 @@
class DownloadItemImplDelegate;
-namespace content {
-class DownloadFile;
-}
-
// See download_item.h for usage.
-class CONTENT_EXPORT DownloadItemImpl
- : public content::DownloadItem,
- public content::DownloadDestinationObserver {
+class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem {
public:
// Note that it is the responsibility of the caller to ensure that a
// DownloadItemImplDelegate passed to a DownloadItemImpl constructor
@@ -66,9 +59,6 @@ class CONTENT_EXPORT DownloadItemImpl
// Implementation functions (not part of the DownloadItem interface).
- // Start the download
- virtual void Start(scoped_ptr<content::DownloadFile> download_file);
-
// Called when the target path has been determined. |target_path| is the
// suggested target path. |disposition| indicates how the target path should
// be used (see TargetDisposition). |danger_type| is the danger level of
@@ -89,6 +79,14 @@ class CONTENT_EXPORT DownloadItemImpl
// Set the item's DB handle.
virtual void SetDbHandle(int64 handle);
+ // Cancels the off-thread aspects of the download.
+ // TODO(rdsmith): This should be private and only called from
+ // DownloadItem::Cancel/Interrupt; it isn't now because we can't
+ // call those functions from
+ // DownloadManager::FileSelectionCancelled() without doing some
+ // rewrites of the DownloadManager queues.
+ virtual void OffThreadCancel();
+
// Called when the downloaded file is removed.
virtual void OnDownloadedFileRemoved();
@@ -110,16 +108,11 @@ class CONTENT_EXPORT DownloadItemImpl
virtual void MarkAsComplete();
// Called when all data has been saved. Only has display effects.
- virtual void OnAllDataSaved(const std::string& final_hash);
+ virtual void OnAllDataSaved(int64 size, const std::string& final_hash);
// Called by SavePackage to set the total number of bytes on the item.
virtual void SetTotalBytes(int64 total_bytes);
- // Provide a weak pointer reference to a DownloadDestinationObserver
- // for use by download destinations.
- base::WeakPtr<content::DownloadDestinationObserver>
- DestinationObserverAsWeakPtr();
-
// Notify observers that this item is being removed by the user.
virtual void NotifyRemoved();
@@ -204,17 +197,6 @@ class CONTENT_EXPORT DownloadItemImpl
virtual void MockDownloadOpenForTesting() OVERRIDE;
private:
- // DownloadDestinationObserver
- virtual void DestinationUpdate(int64 bytes_so_far,
- int64 bytes_per_sec,
- const std::string& hash_state) OVERRIDE;
- virtual void DestinationError(
- content::DownloadInterruptReason reason) OVERRIDE;
- virtual void DestinationCompleted(const std::string& final_hash) OVERRIDE;
-
- // For weak pointer downcasting.
- friend class base::WeakPtr<content::DownloadDestinationObserver>;
-
// Construction common to all constructors. |active| should be true for new
// downloads and false for downloads from the history.
// |download_type| indicates to the net log system what kind of download
@@ -230,6 +212,12 @@ class CONTENT_EXPORT DownloadItemImpl
// downloads still may block on user acceptance after this point.)
void MaybeCompleteDownload();
+ // Internal helper for maintaining consistent received and total sizes, and
+ // setting the final hash.
+ // Should only be called from |OnAllDataSaved|.
+ void ProgressComplete(int64 bytes_so_far,
+ const std::string& final_hash);
+
// Called when the entire download operation (including renaming etc)
// is completed.
void Completed();
@@ -252,10 +240,6 @@ class CONTENT_EXPORT DownloadItemImpl
void OnDownloadRenamedToIntermediateName(
content::DownloadInterruptReason reason, const FilePath& full_path);
- // Callback from file thread when we initialize the DownloadFile.
- void OnDownloadFileInitialized(
- content::DownloadInterruptReason result);
-
// Callback from file thread when we release the DownloadFile.
void OnDownloadFileReleased();
@@ -414,12 +398,6 @@ class CONTENT_EXPORT DownloadItemImpl
// Did the delegate delay calling Complete on this download?
bool delegate_delayed_complete_;
- // DownloadFile associated with this download. Note that this
- // pointer may only be used or destroyed on the FILE thread.
- // This pointer will be non-null only while the DownloadItem is in
- // the IN_PROGRESS state.
- scoped_ptr<content::DownloadFile> download_file_;
-
// Net log to use for this download.
const net::BoundNetLog bound_net_log_;
diff --git a/content/browser/download/download_item_impl_delegate.cc b/content/browser/download/download_item_impl_delegate.cc
index 0b9ea10..4a889f7 100644
--- a/content/browser/download/download_item_impl_delegate.cc
+++ b/content/browser/download/download_item_impl_delegate.cc
@@ -2,10 +2,8 @@
// 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_item_impl_delegate.h"
-
#include "base/logging.h"
-#include "content/browser/download/download_file_factory.h"
+#include "content/browser/download/download_item_impl_delegate.h"
class DownloadItemImpl;
@@ -27,15 +25,12 @@ void DownloadItemImplDelegate::Detach() {
--count_;
}
-void DownloadItemImplDelegate::DelegateStart(
- DownloadItemImpl* download_item) {}
-
-bool DownloadItemImplDelegate::ShouldOpenDownload(DownloadItemImpl* download) {
+bool DownloadItemImplDelegate::ShouldOpenFileBasedOnExtension(
+ const FilePath& path) {
return false;
}
-bool DownloadItemImplDelegate::ShouldOpenFileBasedOnExtension(
- const FilePath& path) {
+bool DownloadItemImplDelegate::ShouldOpenDownload(DownloadItemImpl* download) {
return false;
}
diff --git a/content/browser/download/download_item_impl_delegate.h b/content/browser/download/download_item_impl_delegate.h
index a2f2db2..0092454 100644
--- a/content/browser/download/download_item_impl_delegate.h
+++ b/content/browser/download/download_item_impl_delegate.h
@@ -28,23 +28,13 @@ class CONTENT_EXPORT DownloadItemImplDelegate {
void Attach();
void Detach();
- // Start the delegate's portion of the download; called when the
- // download item is ready for the delegate to take over.
- // Pure virtual because if the delegate doesn't do something the
- // DownloadItemImpl is dead in the water.
- // TODO(rdsmith): The machinery of running the download should be
- // moved into the DownloadItem, and this should be changed into
- // a probe as to whether to start and if not, provide a callback
- // to call when it's time to start.
- virtual void DelegateStart(DownloadItemImpl* download) = 0;
+ // Tests if a file type should be opened automatically.
+ virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path);
// Allows the delegate to override the opening of a download. If it returns
// true then it's reponsible for opening the item.
virtual bool ShouldOpenDownload(DownloadItemImpl* download);
- // Tests if a file type should be opened automatically.
- virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path);
-
// Checks whether a downloaded file still exists and updates the
// file's state if the file is already removed.
// The check may or may not result in a later asynchronous call
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index ff033ca..d2ce5dd 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -7,13 +7,11 @@
#include "base/threading/thread.h"
#include "content/browser/download/byte_stream.h"
#include "content/browser/download/download_create_info.h"
-#include "content/browser/download/download_file_factory.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_item_impl.h"
#include "content/browser/download/download_item_impl_delegate.h"
#include "content/browser/download/download_request_handle.h"
-#include "content/browser/download/mock_download_file.h"
#include "content/public/browser/download_id.h"
-#include "content/public/browser/download_destination_observer.h"
#include "content/public/browser/download_interrupt_reasons.h"
#include "content/public/test/mock_download_item.h"
#include "content/public/test/test_browser_thread.h"
@@ -30,16 +28,17 @@ using ::testing::_;
using ::testing::AllOf;
using ::testing::Property;
using ::testing::Return;
-using ::testing::StrictMock;
DownloadId::Domain kValidDownloadItemIdDomain = "valid DownloadId::Domain";
namespace {
class MockDelegate : public DownloadItemImplDelegate {
public:
- MOCK_METHOD1(DelegateStart, void(DownloadItemImpl* download));
- MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItemImpl* download));
+ MockDelegate(DownloadFileManager* file_manager)
+ : file_manager_(file_manager) {
+ }
MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path));
+ MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItemImpl* download));
MOCK_METHOD1(CheckForFileRemoval, void(DownloadItemImpl* download));
MOCK_METHOD1(MaybeCompleteDownload, void(DownloadItemImpl* download));
MOCK_CONST_METHOD0(GetBrowserContext, content::BrowserContext*());
@@ -68,15 +67,53 @@ class MockRequestHandle : public DownloadRequestHandleInterface {
MOCK_CONST_METHOD0(DebugString, std::string());
};
+class MockDownloadFileFactory
+ : public DownloadFileManager::DownloadFileFactory {
+ public:
+ content::DownloadFile* CreateFile(
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream_reader,
+ DownloadManager* mgr,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) {
+ return MockCreateFile(
+ info, stream_reader.get(), info->request_handle, mgr, calculate_hash,
+ bound_net_log);
+ }
+
+ MOCK_METHOD6(MockCreateFile,
+ content::DownloadFile*(DownloadCreateInfo*,
+ content::ByteStreamReader*,
+ const DownloadRequestHandle&,
+ DownloadManager*,
+ bool,
+ const net::BoundNetLog&));
+};
+
+class MockDownloadFileManager : public DownloadFileManager {
+ public:
+ MockDownloadFileManager();
+ MOCK_METHOD0(Shutdown, void());
+ MOCK_METHOD1(CancelDownload, void(DownloadId));
+ MOCK_METHOD2(CompleteDownload, void(DownloadId, const base::Closure&));
+ MOCK_METHOD1(OnDownloadManagerShutdown, void(DownloadManager*));
+ MOCK_METHOD4(RenameDownloadFile, void(DownloadId, const FilePath&, bool,
+ const RenameCompletionCallback&));
+ MOCK_CONST_METHOD0(NumberOfActiveDownloads, int());
+ private:
+ ~MockDownloadFileManager() {}
+};
+
// Schedules a task to invoke the RenameCompletionCallback with |new_path| on
// the UI thread. Should only be used as the action for
-// MockDownloadFile::Rename as follows:
-// EXPECT_CALL(download_file, Rename(_,_,_))
+// MockDownloadFileManager::Rename*DownloadFile as follows:
+// EXPECT_CALL(mock_download_file_manager,
+// RenameDownloadFile(_,_,_,_))
// .WillOnce(ScheduleRenameCallback(new_path));
ACTION_P(ScheduleRenameCallback, new_path) {
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
- base::Bind(arg2, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path));
+ base::Bind(arg3, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path));
}
// Similarly for scheduling a completion callback.
@@ -84,6 +121,10 @@ ACTION(ScheduleCompleteCallback) {
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(arg1));
}
+MockDownloadFileManager::MockDownloadFileManager()
+ : DownloadFileManager(new MockDownloadFileFactory) {
+}
+
} // namespace
class DownloadItemTest : public testing::Test {
@@ -143,7 +184,8 @@ class DownloadItemTest : public testing::Test {
DownloadItemTest()
: ui_thread_(BrowserThread::UI, &loop_),
file_thread_(BrowserThread::FILE, &loop_),
- delegate_() {
+ file_manager_(new MockDownloadFileManager),
+ delegate_(file_manager_.get()) {
}
~DownloadItemTest() {
@@ -184,27 +226,6 @@ class DownloadItemTest : public testing::Test {
return download;
}
- // Add DownloadFile to DownloadItem
- MockDownloadFile* AddDownloadFileToDownloadItem(DownloadItemImpl* item) {
- MockDownloadFile* mock_download_file(new StrictMock<MockDownloadFile>);
- scoped_ptr<content::DownloadFile> download_file(mock_download_file);
- EXPECT_CALL(*mock_download_file, Initialize(_));
- item->Start(download_file.Pass());
- loop_.RunAllPending();
- return mock_download_file;
- }
-
- // Cleanup a download item (specifically get rid of the DownloadFile on it).
- // The item must be in the IN_PROGRESS state.
- void CleanupItem(DownloadItemImpl* item, MockDownloadFile* download_file) {
- EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
-
- EXPECT_CALL(*download_file, Cancel());
- EXPECT_CALL(delegate_, DownloadStopped(item));
- item->Cancel(true);
- loop_.RunAllPending();
- }
-
// Destroy a previously created download item.
void DestroyDownloadItem(DownloadItem* item) {
allocated_downloads_.erase(item);
@@ -219,10 +240,15 @@ class DownloadItemTest : public testing::Test {
return &delegate_;
}
+ MockDownloadFileManager* mock_file_manager() {
+ return file_manager_.get();
+ }
+
private:
MessageLoopForUI loop_;
content::TestBrowserThread ui_thread_; // UI thread
content::TestBrowserThread file_thread_; // FILE thread
+ scoped_refptr<MockDownloadFileManager> file_manager_;
testing::NiceMock<MockDelegate> delegate_;
std::set<DownloadItem*> allocated_downloads_;
};
@@ -256,30 +282,24 @@ TEST_F(DownloadItemTest, NotificationAfterUpdate) {
TEST_F(DownloadItemTest, NotificationAfterCancel) {
DownloadItemImpl* user_cancel = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(user_cancel);
- EXPECT_CALL(*download_file, Cancel());
MockObserver observer1(user_cancel);
user_cancel->Cancel(true);
ASSERT_TRUE(observer1.CheckUpdated());
- RunAllPendingInMessageLoops();
DownloadItemImpl* system_cancel =
CreateDownloadItem(DownloadItem::IN_PROGRESS);
- download_file = AddDownloadFileToDownloadItem(system_cancel);
- EXPECT_CALL(*download_file, Cancel());
MockObserver observer2(system_cancel);
system_cancel->Cancel(false);
ASSERT_TRUE(observer2.CheckUpdated());
- RunAllPendingInMessageLoops();
}
TEST_F(DownloadItemTest, NotificationAfterComplete) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
MockObserver observer(item);
- item->OnAllDataSaved(DownloadItem::kEmptyFileHash);
+ item->OnAllDataSaved(kDownloadChunkSize, DownloadItem::kEmptyFileHash);
ASSERT_TRUE(observer.CheckUpdated());
item->MarkAsComplete();
@@ -296,24 +316,18 @@ TEST_F(DownloadItemTest, NotificationAfterDownloadedFileRemoved) {
TEST_F(DownloadItemTest, NotificationAfterInterrupted) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
- EXPECT_CALL(*download_file, Cancel());
MockObserver observer(item);
item->Interrupt(content::DOWNLOAD_INTERRUPT_REASON_NONE);
ASSERT_TRUE(observer.CheckUpdated());
- RunAllPendingInMessageLoops();
}
TEST_F(DownloadItemTest, NotificationAfterDelete) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
- EXPECT_CALL(*download_file, Cancel());
MockObserver observer(item);
item->Delete(DownloadItem::DELETE_DUE_TO_BROWSER_SHUTDOWN);
ASSERT_TRUE(observer.CheckUpdated());
- RunAllPendingInMessageLoops();
}
TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
@@ -326,8 +340,6 @@ TEST_F(DownloadItemTest, NotificationAfterDestroyed) {
TEST_F(DownloadItemTest, NotificationAfterRemove) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
- EXPECT_CALL(*download_file, Cancel());
MockObserver observer(item);
item->Remove();
@@ -340,7 +352,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
DownloadItemImpl* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
MockObserver safe_observer(safe_item);
- safe_item->OnAllDataSaved("");
+ safe_item->OnAllDataSaved(1, "");
EXPECT_TRUE(safe_observer.CheckUpdated());
safe_item->OnContentCheckCompleted(
content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
@@ -351,7 +363,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
CreateDownloadItem(DownloadItem::IN_PROGRESS);
MockObserver unsafeurl_observer(unsafeurl_item);
- unsafeurl_item->OnAllDataSaved("");
+ unsafeurl_item->OnAllDataSaved(1, "");
EXPECT_TRUE(unsafeurl_observer.CheckUpdated());
unsafeurl_item->OnContentCheckCompleted(
content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL);
@@ -364,7 +376,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
CreateDownloadItem(DownloadItem::IN_PROGRESS);
MockObserver unsafefile_observer(unsafefile_item);
- unsafefile_item->OnAllDataSaved("");
+ unsafefile_item->OnAllDataSaved(1, "");
EXPECT_TRUE(unsafefile_observer.CheckUpdated());
unsafefile_item->OnContentCheckCompleted(
content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE);
@@ -375,18 +387,18 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) {
}
// DownloadItemImpl::OnDownloadTargetDetermined will schedule a task to run
-// DownloadFile::Rename(). Once the rename
+// DownloadFileManager::RenameDownloadFile(). Once the rename
// completes, DownloadItemImpl receives a notification with the new file
// name. Check that observers are updated when the new filename is available and
// not before.
TEST_F(DownloadItemTest, NotificationAfterOnDownloadTargetDetermined) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
MockObserver observer(item);
FilePath target_path(kDummyPath);
FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
FilePath new_intermediate_path(target_path.InsertBeforeExtensionASCII("y"));
- EXPECT_CALL(*download_file, Rename(intermediate_path, false, _))
+ EXPECT_CALL(*mock_file_manager(),
+ RenameDownloadFile(_,intermediate_path,false,_))
.WillOnce(ScheduleRenameCallback(new_intermediate_path));
// Currently, a notification would be generated if the danger type is anything
@@ -399,8 +411,6 @@ TEST_F(DownloadItemTest, NotificationAfterOnDownloadTargetDetermined) {
RunAllPendingInMessageLoops();
EXPECT_TRUE(observer.CheckUpdated());
EXPECT_EQ(new_intermediate_path, item->GetFullPath());
-
- CleanupItem(item, download_file);
}
TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
@@ -416,12 +426,12 @@ TEST_F(DownloadItemTest, NotificationAfterTogglePause) {
TEST_F(DownloadItemTest, DisplayName) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
FilePath target_path(FilePath(kDummyPath).AppendASCII("foo.bar"));
FilePath intermediate_path(target_path.InsertBeforeExtensionASCII("x"));
EXPECT_EQ(FILE_PATH_LITERAL(""),
item->GetFileNameToReportUser().value());
- EXPECT_CALL(*download_file, Rename(_, false, _))
+ EXPECT_CALL(*mock_file_manager(),
+ RenameDownloadFile(_,_,false,_))
.WillOnce(ScheduleRenameCallback(intermediate_path));
item->OnDownloadTargetDetermined(target_path,
DownloadItem::TARGET_DISPOSITION_OVERWRITE,
@@ -433,18 +443,6 @@ TEST_F(DownloadItemTest, DisplayName) {
item->SetDisplayName(FilePath(FILE_PATH_LITERAL("new.name")));
EXPECT_EQ(FILE_PATH_LITERAL("new.name"),
item->GetFileNameToReportUser().value());
- CleanupItem(item, download_file);
-}
-
-// Test to make sure that Start method calls DF initialize properly.
-TEST_F(DownloadItemTest, Start) {
- MockDownloadFile* mock_download_file(new MockDownloadFile);
- scoped_ptr<content::DownloadFile> download_file(mock_download_file);
- DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- EXPECT_CALL(*mock_download_file, Initialize(_));
- item->Start(download_file.Pass());
-
- CleanupItem(item, mock_download_file);
}
// Test that the delegate is invoked after the download file is renamed.
@@ -454,13 +452,13 @@ TEST_F(DownloadItemTest, Start) {
// rename.
TEST_F(DownloadItemTest, CallbackAfterRename) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
FilePath final_path(FilePath(kDummyPath).AppendASCII("foo.bar"));
FilePath intermediate_path(final_path.InsertBeforeExtensionASCII("x"));
FilePath new_intermediate_path(final_path.InsertBeforeExtensionASCII("y"));
- EXPECT_CALL(*download_file, Rename(intermediate_path, false, _))
+ EXPECT_CALL(*mock_file_manager(),
+ RenameDownloadFile(item->GetGlobalId(),
+ intermediate_path, false, _))
.WillOnce(ScheduleRenameCallback(new_intermediate_path));
-
// DownloadItemImpl should invoke this callback on the delegate once the
// download is renamed to the intermediate name. Also check that GetFullPath()
// returns the intermediate path at the time of the call.
@@ -475,12 +473,17 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
intermediate_path);
RunAllPendingInMessageLoops();
// All the callbacks should have happened by now.
- ::testing::Mock::VerifyAndClearExpectations(download_file);
+ ::testing::Mock::VerifyAndClearExpectations(mock_file_manager());
::testing::Mock::VerifyAndClearExpectations(mock_delegate());
- item->OnAllDataSaved("");
- EXPECT_CALL(*download_file, Rename(final_path, true, _))
+ item->OnAllDataSaved(10, "");
+ EXPECT_CALL(*mock_file_manager(),
+ RenameDownloadFile(item->GetGlobalId(),
+ final_path, true, _))
.WillOnce(ScheduleRenameCallback(final_path));
+ EXPECT_CALL(*mock_file_manager(),
+ CompleteDownload(item->GetGlobalId(), _))
+ .WillOnce(ScheduleCompleteCallback());
// DownloadItemImpl should invoke this callback on the delegate after the
// final rename has completed. Also check that GetFullPath() and
// GetTargetFilePath() return the final path at the time of the call.
@@ -493,24 +496,20 @@ TEST_F(DownloadItemTest, CallbackAfterRename) {
EXPECT_CALL(*mock_delegate(), DownloadCompleted(item));
EXPECT_CALL(*mock_delegate(), ShouldOpenDownload(item))
.WillOnce(Return(true));
- EXPECT_CALL(*download_file, Detach());
item->OnDownloadCompleting();
RunAllPendingInMessageLoops();
- ::testing::Mock::VerifyAndClearExpectations(download_file);
+ ::testing::Mock::VerifyAndClearExpectations(mock_file_manager());
::testing::Mock::VerifyAndClearExpectations(mock_delegate());
}
TEST_F(DownloadItemTest, Interrupted) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
const content::DownloadInterruptReason reason(
content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
// Confirm interrupt sets state properly.
- EXPECT_CALL(*download_file, Cancel());
item->Interrupt(reason);
- RunAllPendingInMessageLoops();
EXPECT_EQ(DownloadItem::INTERRUPTED, item->GetState());
EXPECT_EQ(reason, item->GetLastReason());
@@ -523,14 +522,11 @@ TEST_F(DownloadItemTest, Interrupted) {
TEST_F(DownloadItemTest, Canceled) {
DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
// Confirm cancel sets state properly.
EXPECT_CALL(*mock_delegate(), DownloadStopped(item));
- EXPECT_CALL(*download_file, Cancel());
item->Cancel(true);
EXPECT_EQ(DownloadItem::CANCELLED, item->GetState());
- RunAllPendingInMessageLoops();
}
TEST_F(DownloadItemTest, FileRemoved) {
@@ -541,89 +537,6 @@ TEST_F(DownloadItemTest, FileRemoved) {
EXPECT_TRUE(item->GetFileExternallyRemoved());
}
-TEST_F(DownloadItemTest, DestinationUpdate) {
- DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- base::WeakPtr<content::DownloadDestinationObserver> as_observer(
- item->DestinationObserverAsWeakPtr());
- MockObserver observer(item);
-
- EXPECT_EQ(0l, item->CurrentSpeed());
- EXPECT_EQ("", item->GetHashState());
- EXPECT_EQ(0l, item->GetReceivedBytes());
- EXPECT_EQ(0l, item->GetTotalBytes());
- EXPECT_FALSE(observer.CheckUpdated());
- item->SetTotalBytes(100l);
- EXPECT_EQ(100l, item->GetTotalBytes());
-
- as_observer->DestinationUpdate(10, 20, "deadbeef");
- EXPECT_EQ(20l, item->CurrentSpeed());
- EXPECT_EQ("deadbeef", item->GetHashState());
- EXPECT_EQ(10l, item->GetReceivedBytes());
- EXPECT_EQ(100l, item->GetTotalBytes());
- EXPECT_TRUE(observer.CheckUpdated());
-
- as_observer->DestinationUpdate(200, 20, "livebeef");
- EXPECT_EQ(20l, item->CurrentSpeed());
- EXPECT_EQ("livebeef", item->GetHashState());
- EXPECT_EQ(200l, item->GetReceivedBytes());
- EXPECT_EQ(0l, item->GetTotalBytes());
- EXPECT_TRUE(observer.CheckUpdated());
-}
-
-TEST_F(DownloadItemTest, DestinationError) {
- DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item);
- base::WeakPtr<content::DownloadDestinationObserver> as_observer(
- item->DestinationObserverAsWeakPtr());
- MockObserver observer(item);
-
- EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
- EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, item->GetLastReason());
- EXPECT_FALSE(observer.CheckUpdated());
-
- EXPECT_CALL(*mock_delegate(), DownloadStopped(item));
- EXPECT_CALL(*download_file, Cancel());
- as_observer->DestinationError(
- content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED);
- ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
- EXPECT_TRUE(observer.CheckUpdated());
- EXPECT_EQ(content::DownloadItem::INTERRUPTED, item->GetState());
- EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED,
- item->GetLastReason());
- RunAllPendingInMessageLoops();
-}
-
-TEST_F(DownloadItemTest, DestinationCompleted) {
- DownloadItemImpl* item = CreateDownloadItem(DownloadItem::IN_PROGRESS);
- base::WeakPtr<content::DownloadDestinationObserver> as_observer(
- item->DestinationObserverAsWeakPtr());
- MockObserver observer(item);
-
- EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
- EXPECT_EQ("", item->GetHash());
- EXPECT_EQ("", item->GetHashState());
- EXPECT_FALSE(item->AllDataSaved());
- EXPECT_FALSE(observer.CheckUpdated());
-
- as_observer->DestinationUpdate(10, 20, "deadbeef");
- EXPECT_TRUE(observer.CheckUpdated());
- EXPECT_FALSE(observer.CheckUpdated()); // Confirm reset.
- EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
- EXPECT_EQ("", item->GetHash());
- EXPECT_EQ("deadbeef", item->GetHashState());
- EXPECT_FALSE(item->AllDataSaved());
-
- EXPECT_CALL(*mock_delegate(), MaybeCompleteDownload(item));
- as_observer->DestinationCompleted("livebeef");
- ::testing::Mock::VerifyAndClearExpectations(mock_delegate());
- EXPECT_EQ(content::DownloadItem::IN_PROGRESS, item->GetState());
- EXPECT_TRUE(observer.CheckUpdated());
- EXPECT_EQ("livebeef", item->GetHash());
- EXPECT_EQ("", item->GetHashState());
- EXPECT_TRUE(item->AllDataSaved());
-}
-
TEST(MockDownloadItem, Compiles) {
MockDownloadItem mock_item;
}
-
diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc
index 88a4982..e779760 100644
--- a/content/browser/download/download_manager_impl.cc
+++ b/content/browser/download/download_manager_impl.cc
@@ -21,7 +21,7 @@
#include "build/build_config.h"
#include "content/browser/download/byte_stream.h"
#include "content/browser/download/download_create_info.h"
-#include "content/browser/download/download_file_factory.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_item_impl.h"
#include "content/browser/download/download_stats.h"
#include "content/browser/renderer_host/render_view_host_impl.h"
@@ -144,13 +144,23 @@ class MapValueIteratorAdapter {
// Allow copy and assign.
};
-void EnsureNoPendingDownloadJobsOnFile(bool* result) {
- DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
- *result = (content::DownloadFile::GetNumberOfDownloadFiles() == 0);
+void EnsureNoPendingDownloadsOnFile(scoped_refptr<DownloadFileManager> dfm,
+ bool* result) {
+ if (dfm->NumberOfActiveDownloads())
+ *result = false;
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE, MessageLoop::QuitClosure());
}
+void EnsureNoPendingDownloadJobsOnIO(bool* result) {
+ scoped_refptr<DownloadFileManager> download_file_manager =
+ ResourceDispatcherHostImpl::Get()->download_file_manager();
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&EnsureNoPendingDownloadsOnFile,
+ download_file_manager, result));
+}
+
class DownloadItemFactoryImpl : public content::DownloadItemFactory {
public:
DownloadItemFactoryImpl() {}
@@ -193,8 +203,8 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
bool result = true;
BrowserThread::PostTask(
- BrowserThread::FILE, FROM_HERE,
- base::Bind(&EnsureNoPendingDownloadJobsOnFile, &result));
+ BrowserThread::IO, FROM_HERE,
+ base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result));
MessageLoop::current()->Run();
return result;
}
@@ -202,17 +212,19 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() {
} // namespace content
DownloadManagerImpl::DownloadManagerImpl(
- scoped_ptr<content::DownloadItemFactory> item_factory,
+ DownloadFileManager* file_manager,
+ scoped_ptr<content::DownloadItemFactory> factory,
net::NetLog* net_log)
- : item_factory_(item_factory.Pass()),
+ : factory_(factory.Pass()),
history_size_(0),
shutdown_needed_(false),
browser_context_(NULL),
+ file_manager_(file_manager),
delegate_(NULL),
net_log_(net_log) {
- if (!item_factory_.get())
- item_factory_.reset(new DownloadItemFactoryImpl());
- file_factory_.reset(new content::DownloadFileFactory());
+ DCHECK(file_manager);
+ if (!factory_.get())
+ factory_.reset(new DownloadItemFactoryImpl());
}
DownloadManagerImpl::~DownloadManagerImpl() {
@@ -231,18 +243,8 @@ DownloadId DownloadManagerImpl::GetNextId() {
return id;
}
-void DownloadManagerImpl::DelegateStart(DownloadItemImpl* item) {
- content::DownloadTargetCallback callback =
- base::Bind(&DownloadManagerImpl::OnDownloadTargetDetermined,
- this, item->GetId());
- if (!delegate_ || !delegate_->DetermineDownloadTarget(item, callback)) {
- FilePath target_path = item->GetForcedFilePath();
- // TODO(asanka): Determine a useful path if |target_path| is empty.
- callback.Run(target_path,
- DownloadItem::TARGET_DISPOSITION_OVERWRITE,
- content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
- target_path);
- }
+DownloadFileManager* DownloadManagerImpl::GetDownloadFileManager() {
+ return file_manager_;
}
bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) {
@@ -278,7 +280,11 @@ void DownloadManagerImpl::Shutdown() {
FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this));
// TODO(benjhayden): Consider clearing observers_.
- // The DownloadFiles will be canceled and deleted by their DownloadItems.
+ DCHECK(file_manager_);
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&DownloadFileManager::OnDownloadManagerShutdown,
+ file_manager_, make_scoped_refptr(this)));
AssertContainersConsistent();
@@ -321,6 +327,7 @@ void DownloadManagerImpl::Shutdown() {
// We'll have nothing more to report to the observers after this point.
observers_.Clear();
+ file_manager_ = NULL;
if (delegate_)
delegate_->Shutdown();
delegate_ = NULL;
@@ -384,28 +391,65 @@ bool DownloadManagerImpl::Init(content::BrowserContext* browser_context) {
return true;
}
+// We have received a message from DownloadFileManager about a new download.
content::DownloadId DownloadManagerImpl::StartDownload(
scoped_ptr<DownloadCreateInfo> info,
scoped_ptr<content::ByteStreamReader> stream) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- net::BoundNetLog bound_net_log =
- net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
+ // |bound_net_log| will be used for logging both the download item's and
+ // the download file's events.
+ net::BoundNetLog bound_net_log = CreateDownloadItem(info.get());
+
+ // If info->download_id was unknown on entry to this function, it was
+ // assigned in CreateDownloadItem.
+ DownloadId download_id = info->download_id;
- // We create the DownloadItem before the DownloadFile because the
- // DownloadItem already needs to handle a state in which there is
- // no associated DownloadFile (history downloads, !IN_PROGRESS downloads)
- DownloadItemImpl* download =
- CreateDownloadItem(info.get(), bound_net_log);
- scoped_ptr<content::DownloadFile> download_file(
- file_factory_->CreateFile(
- info->save_info, info->url(), info->referrer_url,
- info->received_bytes, GenerateFileHash(),
- stream.Pass(), bound_net_log,
- download->DestinationObserverAsWeakPtr()));
- download->Start(download_file.Pass());
+ DownloadFileManager::CreateDownloadFileCallback callback(
+ base::Bind(&DownloadManagerImpl::OnDownloadFileCreated,
+ this, download_id.local()));
- return download->GetGlobalId();
+ BrowserThread::PostTask(
+ BrowserThread::FILE, FROM_HERE,
+ base::Bind(&DownloadFileManager::CreateDownloadFile,
+ file_manager_, base::Passed(info.Pass()),
+ base::Passed(stream.Pass()),
+ make_scoped_refptr(this),
+ GenerateFileHash(), bound_net_log,
+ callback));
+
+ return download_id;
+}
+
+void DownloadManagerImpl::OnDownloadFileCreated(
+ int32 download_id, content::DownloadInterruptReason reason) {
+ if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) {
+ OnDownloadInterrupted(download_id, reason);
+ // TODO(rdsmith): It makes no sense to continue along the
+ // regular download path after we've gotten an error. But it's
+ // the way the code has historically worked, and this allows us
+ // to get the download persisted and observers of the download manager
+ // notified, so tests work. When we execute all side effects of cancel
+ // (including queue removal) immedately rather than waiting for
+ // persistence we should replace this comment with a "return;".
+ }
+
+ DownloadMap::iterator download_iter = active_downloads_.find(download_id);
+ if (download_iter == active_downloads_.end())
+ return;
+
+ DownloadItemImpl* download = download_iter->second;
+ content::DownloadTargetCallback callback =
+ base::Bind(&DownloadManagerImpl::OnDownloadTargetDetermined,
+ this, download_id);
+ if (!delegate_ || !delegate_->DetermineDownloadTarget(download, callback)) {
+ FilePath target_path = download->GetForcedFilePath();
+ // TODO(asanka): Determine a useful path if |target_path| is empty.
+ callback.Run(target_path,
+ DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+ content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+ target_path);
+ }
}
void DownloadManagerImpl::OnDownloadTargetDetermined(
@@ -469,13 +513,15 @@ content::BrowserContext* DownloadManagerImpl::GetBrowserContext() const {
return browser_context_;
}
-DownloadItemImpl* DownloadManagerImpl::CreateDownloadItem(
- DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log) {
+net::BoundNetLog DownloadManagerImpl::CreateDownloadItem(
+ DownloadCreateInfo* info) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ net::BoundNetLog bound_net_log =
+ net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
if (!info->download_id.IsValid())
info->download_id = GetNextId();
- DownloadItemImpl* download = item_factory_->CreateActiveItem(
+ DownloadItemImpl* download = factory_->CreateActiveItem(
this, *info,
scoped_ptr<DownloadRequestHandleInterface>(
new DownloadRequestHandle(info->request_handle)).Pass(),
@@ -487,7 +533,7 @@ DownloadItemImpl* DownloadManagerImpl::CreateDownloadItem(
active_downloads_[download->GetId()] = download;
FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download));
- return download;
+ return bound_net_log;
}
DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
@@ -497,7 +543,7 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
DownloadItem::Observer* observer) {
net::BoundNetLog bound_net_log =
net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
- DownloadItemImpl* download = item_factory_->CreateSavePageItem(
+ DownloadItemImpl* download = factory_->CreateSavePageItem(
this,
main_file_path,
page_url,
@@ -524,6 +570,40 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem(
return download;
}
+void DownloadManagerImpl::UpdateDownload(int32 download_id,
+ int64 bytes_so_far,
+ int64 bytes_per_sec,
+ const std::string& hash_state) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ DownloadMap::iterator it = active_downloads_.find(download_id);
+ if (it != active_downloads_.end()) {
+ DownloadItemImpl* download = it->second;
+ if (download->IsInProgress()) {
+ download->UpdateProgress(bytes_so_far, bytes_per_sec, hash_state);
+ if (delegate_)
+ delegate_->UpdateItemInPersistentStore(download);
+ }
+ }
+}
+
+void DownloadManagerImpl::OnResponseCompleted(int32 download_id,
+ int64 size,
+ const std::string& hash) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ VLOG(20) << __FUNCTION__ << "()" << " download_id = " << download_id
+ << " size = " << size;
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ // If it's not in active_downloads_, that means it was cancelled; just
+ // ignore the notification.
+ if (active_downloads_.count(download_id) == 0)
+ return;
+
+ DownloadItemImpl* download = active_downloads_[download_id];
+ download->OnAllDataSaved(size, hash);
+ MaybeCompleteDownload(download);
+}
+
void DownloadManagerImpl::AssertStateConsistent(
DownloadItemImpl* download) const {
CHECK(ContainsKey(downloads_, download->GetId()));
@@ -645,6 +725,19 @@ void DownloadManagerImpl::DownloadStopped(DownloadItemImpl* download) {
// This function is called from the DownloadItem, so DI state
// should already have been updated.
AssertStateConsistent(download);
+
+ DCHECK(file_manager_);
+ download->OffThreadCancel();
+}
+
+void DownloadManagerImpl::OnDownloadInterrupted(
+ int32 download_id,
+ content::DownloadInterruptReason reason) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ if (!ContainsKey(active_downloads_, download_id))
+ return;
+ active_downloads_[download_id]->Interrupt(reason);
}
void DownloadManagerImpl::RemoveFromActiveList(DownloadItemImpl* download) {
@@ -664,16 +757,6 @@ bool DownloadManagerImpl::GenerateFileHash() {
return delegate_ && delegate_->GenerateFileHash();
}
-void DownloadManagerImpl::SetDownloadFileFactoryForTesting(
- scoped_ptr<content::DownloadFileFactory> file_factory) {
- file_factory_ = file_factory.Pass();
-}
-
-content::DownloadFileFactory*
-DownloadManagerImpl::GetDownloadFileFactoryForTesting() {
- return file_factory_.get();
-}
-
int DownloadManagerImpl::RemoveDownloadItems(
const DownloadItemImplVector& pending_deletes) {
if (pending_deletes.empty())
@@ -781,7 +864,7 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete(
net::BoundNetLog bound_net_log =
net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD);
- DownloadItemImpl* download = item_factory_->CreatePersistedItem(
+ DownloadItemImpl* download = factory_->CreatePersistedItem(
this, GetNextId(), entries->at(i), bound_net_log);
DCHECK(!ContainsKey(downloads_, download->GetId()));
downloads_[download->GetId()] = download;
@@ -996,7 +1079,7 @@ void DownloadManagerImpl::DownloadRenamedToIntermediateName(
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
// download->GetFullPath() is only expected to be meaningful after this
// callback is received. Therefore we can now add the download to a persistent
- // store. If the rename failed, we processed an interrupt
+ // store. If the rename failed, we receive an OnDownloadInterrupted() call
// before we receive the DownloadRenamedToIntermediateName() call.
if (delegate_) {
delegate_->AddItemToPersistentStore(download);
@@ -1009,7 +1092,8 @@ void DownloadManagerImpl::DownloadRenamedToIntermediateName(
void DownloadManagerImpl::DownloadRenamedToFinalName(
DownloadItemImpl* download) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- // If the rename failed, we processed an interrupt before we get here.
+ // If the rename failed, we receive an OnDownloadInterrupted() call before we
+ // receive the DownloadRenamedToFinalName() call.
if (delegate_) {
delegate_->UpdatePathForItemInPersistentStore(
download, download->GetFullPath());
diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h
index 26faf9a..8fab016f 100644
--- a/content/browser/download/download_manager_impl.h
+++ b/content/browser/download/download_manager_impl.h
@@ -23,24 +23,17 @@
class DownloadFileManager;
class DownloadItemImpl;
-namespace content {
-class DownloadFileFactory;
-}
-
-namespace net {
-class BoundNetLog;
-}
-
class CONTENT_EXPORT DownloadManagerImpl
: public content::DownloadManager,
private DownloadItemImplDelegate {
public:
- // Caller guarantees that |net_log| will remain valid
+ // Caller guarantees that |file_manager| and |net_log| will remain valid
// for the lifetime of DownloadManagerImpl (until Shutdown() is called).
// |factory| may be a default constructed (null) scoped_ptr; if so,
// the DownloadManagerImpl creates and takes ownership of the
// default DownloadItemFactory.
- DownloadManagerImpl(scoped_ptr<content::DownloadItemFactory> item_factory,
+ DownloadManagerImpl(DownloadFileManager* file_manager,
+ scoped_ptr<content::DownloadItemFactory> factory,
net::NetLog* net_log);
// Implementation functions (not part of the DownloadManager interface).
@@ -68,7 +61,16 @@ class CONTENT_EXPORT DownloadManagerImpl
virtual content::DownloadId StartDownload(
scoped_ptr<DownloadCreateInfo> info,
scoped_ptr<content::ByteStreamReader> stream) OVERRIDE;
+ virtual void UpdateDownload(int32 download_id,
+ int64 bytes_so_far,
+ int64 bytes_per_sec,
+ const std::string& hash_state) OVERRIDE;
+ virtual void OnResponseCompleted(int32 download_id, int64 size,
+ const std::string& hash) OVERRIDE;
virtual void CancelDownload(int32 download_id) OVERRIDE;
+ virtual void OnDownloadInterrupted(
+ int32 download_id,
+ content::DownloadInterruptReason reason) OVERRIDE;
virtual int RemoveDownloadsBetween(base::Time remove_begin,
base::Time remove_end) OVERRIDE;
virtual int RemoveDownloads(base::Time remove_begin) OVERRIDE;
@@ -91,11 +93,6 @@ class CONTENT_EXPORT DownloadManagerImpl
virtual content::DownloadItem* GetActiveDownloadItem(int id) OVERRIDE;
virtual bool GenerateFileHash() OVERRIDE;
- // For testing; specifically, accessed from TestFileErrorInjector.
- virtual void SetDownloadFileFactoryForTesting(
- scoped_ptr<content::DownloadFileFactory> file_factory);
- virtual content::DownloadFileFactory* GetDownloadFileFactoryForTesting();
-
private:
typedef std::set<content::DownloadItem*> DownloadSet;
typedef base::hash_map<int32, DownloadItemImpl*> DownloadMap;
@@ -110,8 +107,8 @@ class CONTENT_EXPORT DownloadManagerImpl
virtual ~DownloadManagerImpl();
// Creates the download item. Must be called on the UI thread.
- virtual DownloadItemImpl* CreateDownloadItem(
- DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log);
+ // Returns the |BoundNetLog| used by the |DownloadItem|.
+ virtual net::BoundNetLog CreateDownloadItem(DownloadCreateInfo* info);
// Does nothing if |download_id| is not an active download.
void MaybeCompleteDownloadById(int download_id);
@@ -153,6 +150,12 @@ class CONTENT_EXPORT DownloadManagerImpl
// Remove from internal maps.
int RemoveDownloadItems(const DownloadItemImplVector& pending_deletes);
+ // Called in response to our request to the DownloadFileManager to
+ // create a DownloadFile. A |reason| of
+ // content::DOWNLOAD_INTERRUPT_REASON_NONE indicates success.
+ void OnDownloadFileCreated(
+ int32 download_id, content::DownloadInterruptReason reason);
+
// Called when the delegate has completed determining the download target.
// Arguments following |download_id| are as per
// content::DownloadTargetCallback.
@@ -171,7 +174,7 @@ class CONTENT_EXPORT DownloadManagerImpl
// Overridden from DownloadItemImplDelegate
// (Note that |GetBrowserContext| are present in both interfaces.)
- virtual void DelegateStart(DownloadItemImpl* item) OVERRIDE;
+ virtual DownloadFileManager* GetDownloadFileManager() OVERRIDE;
virtual bool ShouldOpenDownload(DownloadItemImpl* item) OVERRIDE;
virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) OVERRIDE;
virtual void CheckForFileRemoval(DownloadItemImpl* download_item) OVERRIDE;
@@ -186,10 +189,7 @@ class CONTENT_EXPORT DownloadManagerImpl
virtual void AssertStateConsistent(DownloadItemImpl* download) const OVERRIDE;
// Factory for creation of downloads items.
- scoped_ptr<content::DownloadItemFactory> item_factory_;
-
- // Factory for the creation of download files.
- scoped_ptr<content::DownloadFileFactory> file_factory_;
+ scoped_ptr<content::DownloadItemFactory> factory_;
// |downloads_| is the owning set for all downloads known to the
// DownloadManager. This includes downloads started by the user in
@@ -202,7 +202,8 @@ class CONTENT_EXPORT DownloadManagerImpl
// until destruction.
//
// |active_downloads_| is a map of all downloads that are currently being
- // processed.
+ // processed. The key is the ID assigned by the DownloadFileManager,
+ // which is unique for the current session.
//
// When a download is created through a user action, the corresponding
// DownloadItem* is placed in |active_downloads_| and remains there until the
@@ -226,6 +227,9 @@ class CONTENT_EXPORT DownloadManagerImpl
// The current active browser context.
content::BrowserContext* browser_context_;
+ // Non-owning pointer for handling file writing on the download_thread_.
+ DownloadFileManager* file_manager_;
+
// Allows an embedder to control behavior. Guaranteed to outlive this object.
content::DownloadManagerDelegate* delegate_;
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index 1cbbe9d..1b40112 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -18,6 +18,7 @@
#include "build/build_config.h"
#include "content/browser/download/byte_stream.h"
#include "content/browser/download/download_create_info.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_item_factory.h"
#include "content/browser/download/download_item_impl.h"
#include "content/browser/download/download_item_impl_delegate.h"
@@ -31,7 +32,6 @@
#include "content/public/test/mock_download_item.h"
#include "content/public/test/test_browser_context.h"
#include "content/public/test/test_browser_thread.h"
-#include "net/base/net_log.h"
#include "net/base/net_util.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gmock_mutant.h"
@@ -80,16 +80,9 @@ class MockDownloadItemImpl : public DownloadItemImpl {
MOCK_METHOD1(Cancel, void(bool));
MOCK_METHOD0(MarkAsComplete, void());
MOCK_METHOD1(DelayedDownloadOpened, void(bool));
- MOCK_METHOD1(OnAllDataSaved, void(const std::string&));
+ MOCK_METHOD2(OnAllDataSaved, void(int64, const std::string&));
MOCK_METHOD0(OnDownloadedFileRemoved, void());
MOCK_METHOD0(MaybeCompleteDownload, void());
- virtual void Start(
- scoped_ptr<content::DownloadFile> download_file) OVERRIDE {
- MockStart(download_file.get());
- }
-
- MOCK_METHOD1(MockStart, void(content::DownloadFile*));
-
MOCK_METHOD1(Interrupt, void(DownloadInterruptReason));
MOCK_METHOD1(Delete, void(DeleteReason));
MOCK_METHOD0(Remove, void());
@@ -160,6 +153,7 @@ class MockDownloadItemImpl : public DownloadItemImpl {
MOCK_CONST_METHOD0(GetFileNameToReportUser, FilePath());
MOCK_METHOD1(SetDisplayName, void(const FilePath&));
MOCK_CONST_METHOD0(GetUserVerifiedFilePath, FilePath());
+ MOCK_METHOD0(OffThreadCancel, void());
MOCK_CONST_METHOD1(DebugString, std::string(bool));
MOCK_METHOD0(MockDownloadOpenForTesting, void());
};
@@ -197,17 +191,52 @@ MockDownloadManagerDelegate::MockDownloadManagerDelegate() {}
MockDownloadManagerDelegate::~MockDownloadManagerDelegate() {}
-class NullDownloadItemImplDelegate : public DownloadItemImplDelegate {
+class MockDownloadFileManager : public DownloadFileManager {
public:
- // Safe to null this out even if it doesn't do anything because none
- // of these functions will ever be called; this class just exists
- // to have something to pass to the DownloadItemImpl base class
- // of MockDownloadItemImpl.
- virtual void DelegateStart(DownloadItemImpl* download) OVERRIDE {
- NOTREACHED();
+ MockDownloadFileManager();
+
+ void CreateDownloadFile(
+ scoped_ptr<DownloadCreateInfo> info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ scoped_refptr<content::DownloadManager> download_manager,
+ bool hash_needed,
+ const net::BoundNetLog& bound_net_log,
+ const CreateDownloadFileCallback& callback) OVERRIDE {
+ // Note that scoped_refptr<> on download manager is also stripped
+ // to make mock comparisons easier. Comparing the scoped_refptr<>
+ // works, but holds a reference to the DownloadManager until
+ // MockDownloadFileManager destruction, which messes up destruction
+ // testing.
+ MockCreateDownloadFile(info.get(), stream.get(), download_manager.get(),
+ hash_needed, bound_net_log, callback);
}
+
+ MOCK_METHOD6(MockCreateDownloadFile, void(
+ DownloadCreateInfo* info,
+ content::ByteStreamReader* stream,
+ content::DownloadManager* download_manager,
+ bool hash_needed,
+ const net::BoundNetLog& bound_net_log,
+ const CreateDownloadFileCallback& callback));
+ MOCK_METHOD0(Shutdown, void());
+ MOCK_METHOD1(CancelDownload, void(content::DownloadId));
+ MOCK_METHOD2(CompleteDownload, void(content::DownloadId,
+ const base::Closure&));
+ MOCK_METHOD1(OnDownloadManagerShutdown, void(content::DownloadManager*));
+ MOCK_METHOD4(RenameDownloadFile, void(content::DownloadId,
+ const FilePath&,
+ bool,
+ const RenameCompletionCallback&));
+ MOCK_CONST_METHOD0(NumberOfActiveDownloads, int());
+ protected:
+ virtual ~MockDownloadFileManager();
};
+MockDownloadFileManager::MockDownloadFileManager()
+ : DownloadFileManager(NULL) {}
+
+MockDownloadFileManager::~MockDownloadFileManager() {}
+
class MockDownloadItemFactory
: public content::DownloadItemFactory,
public base::SupportsWeakPtr<MockDownloadItemFactory> {
@@ -251,7 +280,7 @@ class MockDownloadItemFactory
private:
std::map<int32, MockDownloadItemImpl*> items_;
- NullDownloadItemImplDelegate item_delegate_;
+ DownloadItemImplDelegate item_delegate_;
DISALLOW_COPY_AND_ASSIGN(MockDownloadItemFactory);
};
@@ -311,14 +340,8 @@ DownloadItemImpl* MockDownloadItemFactory::CreateActiveItem(
new StrictMock<MockDownloadItemImpl>(&item_delegate_);
EXPECT_CALL(*result, GetId())
.WillRepeatedly(Return(local_id));
- EXPECT_CALL(*result, GetGlobalId())
- .WillRepeatedly(Return(content::DownloadId(delegate, local_id)));
items_[local_id] = result;
- // Active items are created and then immediately are called to start
- // the download.
- EXPECT_CALL(*result, MockStart(_));
-
return result;
}
@@ -390,8 +413,8 @@ class DownloadManagerTest : public testing::Test {
// We tear down everything in TearDown().
~DownloadManagerTest() {}
- // Create a MockDownloadItemFactory and MockDownloadManagerDelegate,
- // then create a DownloadManager that points
+ // Create a MockDownloadItemFactory, MockDownloadManagerDelegate,
+ // and MockDownloadFileManager, then create a DownloadManager that points
// at all of those.
virtual void SetUp() {
DCHECK(!download_manager_.get());
@@ -401,11 +424,15 @@ class DownloadManagerTest : public testing::Test {
new StrictMock<MockDownloadManagerDelegate>);
EXPECT_CALL(*mock_download_manager_delegate_.get(), Shutdown())
.WillOnce(Return());
+ mock_download_file_manager_ = new StrictMock<MockDownloadFileManager>;
+ EXPECT_CALL(*mock_download_file_manager_.get(),
+ OnDownloadManagerShutdown(_));
mock_browser_context_.reset(new StrictMock<MockBrowserContext>);
EXPECT_CALL(*mock_browser_context_.get(), IsOffTheRecord())
.WillRepeatedly(Return(false));
download_manager_ = new DownloadManagerImpl(
+ mock_download_file_manager_.get(),
scoped_ptr<content::DownloadItemFactory>(
mock_download_item_factory_.get()).Pass(), NULL);
observer_.reset(new MockDownloadManagerObserver());
@@ -433,6 +460,7 @@ class DownloadManagerTest : public testing::Test {
ASSERT_EQ(NULL, mock_download_item_factory_.get());
message_loop_.RunAllPending();
mock_download_manager_delegate_.reset();
+ mock_download_file_manager_ = NULL;
mock_browser_context_.reset();
}
@@ -448,16 +476,12 @@ class DownloadManagerTest : public testing::Test {
++next_download_id_;
info.download_id = content::DownloadId(kDownloadIdDomain, id);
info.request_handle = DownloadRequestHandle();
- EXPECT_CALL(GetMockObserver(),
- OnDownloadCreated(download_manager_.get(), _));
- download_manager_->CreateDownloadItem(&info, net::BoundNetLog());
+ download_manager_->CreateDownloadItem(&info);
DCHECK(mock_download_item_factory_->GetItem(id));
MockDownloadItemImpl& item(*mock_download_item_factory_->GetItem(id));
- // Satisfy expectation. If the item is created in StartDownload(),
- // we call Start on it immediately, so we need to set that expectation
- // in the factory.
- item.Start(scoped_ptr<content::DownloadFile>());
+ ON_CALL(item, GetId())
+ .WillByDefault(Return(id));
return item;
}
@@ -478,6 +502,10 @@ class DownloadManagerTest : public testing::Test {
return *mock_download_manager_delegate_;
}
+ MockDownloadFileManager& GetMockDownloadFileManager() {
+ return *mock_download_file_manager_;
+ }
+
MockDownloadManagerObserver& GetMockObserver() {
return *observer_;
}
@@ -487,10 +515,6 @@ class DownloadManagerTest : public testing::Test {
download_manager_->DownloadStopped(item);
}
- void DelegateStart(DownloadItemImpl* item) {
- download_manager_->DelegateStart(item);
- }
-
void AddItemToHistory(MockDownloadItemImpl& item, int64 db_handle) {
// For DCHECK in AddDownloadItemToHistory. Don't want to use
// WillRepeatedly as it may have to return true after this.
@@ -530,6 +554,7 @@ class DownloadManagerTest : public testing::Test {
content::TestBrowserThread file_thread_;
base::WeakPtr<MockDownloadItemFactory> mock_download_item_factory_;
scoped_ptr<MockDownloadManagerDelegate> mock_download_manager_delegate_;
+ scoped_refptr<MockDownloadFileManager> mock_download_file_manager_;
scoped_ptr<MockBrowserContext> mock_browser_context_;
scoped_ptr<MockDownloadManagerObserver> observer_;
int next_download_id_;
@@ -551,44 +576,36 @@ TEST_F(DownloadManagerTest, StartDownload) {
.WillOnce(Return(content::DownloadId(this, local_id)));
EXPECT_CALL(GetMockDownloadManagerDelegate(), GenerateFileHash())
.WillOnce(Return(true));
+ EXPECT_CALL(GetMockDownloadFileManager(), MockCreateDownloadFile(
+ info.get(), static_cast<content::ByteStreamReader*>(NULL),
+ download_manager_.get(), true, _, _));
download_manager_->StartDownload(info.Pass(), stream.Pass());
EXPECT_TRUE(download_manager_->GetActiveDownloadItem(local_id));
}
-// Confirm that calling DelegateStart behaves properly if the delegate
-// blocks starting.
-TEST_F(DownloadManagerTest, DelegateStart_True) {
+// Do the results of an OnDownloadInterrupted get passed through properly
+// to the DownloadItem?
+TEST_F(DownloadManagerTest, OnDownloadInterrupted) {
+ EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+ .WillOnce(Return());
// Put a mock we have a handle to on the download manager.
MockDownloadItemImpl& item(AddItemToManager());
+ int download_id = item.GetId();
- EXPECT_CALL(GetMockDownloadManagerDelegate(),
- DetermineDownloadTarget(&item, _))
- .WillOnce(Return(true));
- DelegateStart(&item);
-}
+ content::DownloadInterruptReason reason(
+ content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
-// Confirm that calling DelegateStart behaves properly if the delegate
-// allows starting. This also tests OnDownloadTargetDetermined.
-TEST_F(DownloadManagerTest, DelegateStart_False) {
- // Put a mock we have a handle to on the download manager.
- MockDownloadItemImpl& item(AddItemToManager());
-
- EXPECT_CALL(GetMockDownloadManagerDelegate(),
- DetermineDownloadTarget(&item, _))
- .WillOnce(Return(false));
- EXPECT_CALL(item, OnDownloadTargetDetermined(
- _, content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
- content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, _));
- FilePath null_path;
- EXPECT_CALL(item, GetForcedFilePath())
- .WillOnce(ReturnRef(null_path));
- DelegateStart(&item);
+ EXPECT_CALL(item, Interrupt(reason));
+ download_manager_->OnDownloadInterrupted(download_id, reason);
+ EXPECT_EQ(&item, download_manager_->GetActiveDownloadItem(download_id));
}
// Does DownloadStopped remove Download from appropriate queues?
// This test tests non-persisted downloads.
TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) {
+ EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+ .WillOnce(Return());
// Put a mock we have a handle to on the download manager.
MockDownloadItemImpl& item(AddItemToManager());
@@ -599,6 +616,7 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) {
EXPECT_CALL(item, GetDbHandle())
.WillRepeatedly(Return(DownloadItem::kUninitializedHandle));
+ EXPECT_CALL(item, OffThreadCancel());
DownloadStopped(&item);
// TODO(rdsmith): Confirm that the download item is no longer on the
// active list by calling download_manager_->GetActiveDownloadItem(id).
@@ -609,6 +627,8 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) {
// Does DownloadStopped remove Download from appropriate queues?
// This test tests persisted downloads.
TEST_F(DownloadManagerTest, OnDownloadStopped_Persisted) {
+ EXPECT_CALL(GetMockObserver(), OnDownloadCreated(download_manager_.get(), _))
+ .WillOnce(Return());
// Put a mock we have a handle to on the download manager.
MockDownloadItemImpl& item(AddItemToManager());
int download_id = item.GetId();
@@ -626,6 +646,7 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_Persisted) {
EXPECT_CALL(item, GetDbHandle())
.WillRepeatedly(Return(db_handle));
+ EXPECT_CALL(item, OffThreadCancel());
DownloadStopped(&item);
EXPECT_EQ(NULL, download_manager_->GetActiveDownloadItem(download_id));
}
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 96834c6..e42d0b4 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -13,6 +13,7 @@
#include "base/metrics/stats_counters.h"
#include "base/stringprintf.h"
#include "content/browser/download/download_create_info.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_interrupt_reasons_impl.h"
#include "content/browser/download/download_manager_impl.h"
#include "content/browser/download/download_request_handle.h"
diff --git a/content/browser/download/mock_download_file.cc b/content/browser/download/mock_download_file.cc
index 9d02372..d5c7781 100644
--- a/content/browser/download/mock_download_file.cc
+++ b/content/browser/download/mock_download_file.cc
@@ -7,19 +7,11 @@
using ::testing::_;
using ::testing::Return;
-namespace {
-
-void SuccessRun(content::DownloadFile::InitializeCallback callback) {
- callback.Run(content::DOWNLOAD_INTERRUPT_REASON_NONE);
-}
-
-} // namespace
-
MockDownloadFile::MockDownloadFile() {
// This is here because |Initialize()| is normally called right after
// construction.
- ON_CALL(*this, Initialize(_))
- .WillByDefault(::testing::Invoke(SuccessRun));
+ ON_CALL(*this, Initialize())
+ .WillByDefault(Return(content::DOWNLOAD_INTERRUPT_REASON_NONE));
}
MockDownloadFile::~MockDownloadFile() {
diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h
index 9fb83bd..34bcb54 100644
--- a/content/browser/download/mock_download_file.h
+++ b/content/browser/download/mock_download_file.h
@@ -24,7 +24,7 @@ class MockDownloadFile : virtual public content::DownloadFile {
virtual ~MockDownloadFile();
// DownloadFile functions.
- MOCK_METHOD1(Initialize, void(const InitializeCallback&));
+ MOCK_METHOD0(Initialize, content::DownloadInterruptReason());
MOCK_METHOD2(AppendDataToFile, content::DownloadInterruptReason(
const char* data, size_t data_len));
MOCK_METHOD1(Rename, content::DownloadInterruptReason(
@@ -42,6 +42,7 @@ class MockDownloadFile : virtual public content::DownloadFile {
MOCK_CONST_METHOD0(CurrentSpeed, int64());
MOCK_METHOD1(GetHash, bool(std::string* hash));
MOCK_METHOD0(GetHashState, std::string());
+ MOCK_METHOD0(CancelDownloadRequest, void());
MOCK_METHOD0(SendUpdate, void());
MOCK_CONST_METHOD0(Id, int());
MOCK_METHOD0(GetDownloadManager, content::DownloadManager*());
diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc
index 6c941f7..0ef878e 100644
--- a/content/browser/download/save_package.cc
+++ b/content/browser/download/save_package.cc
@@ -18,6 +18,7 @@
#include "base/sys_string_conversions.h"
#include "base/threading/thread.h"
#include "base/utf_string_conversions.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_item_impl.h"
#include "content/browser/download/download_manager_impl.h"
#include "content/browser/download/download_stats.h"
@@ -337,8 +338,7 @@ void SavePackage::OnMHTMLGenerated(const FilePath& path, int64 size) {
// Must call OnAllDataSaved here in order for
// GDataDownloadObserver::ShouldUpload() to return true.
// ShouldCompleteDownload() may depend on the gdata uploader to finish.
- download_->UpdateProgress(size, CurrentSpeed(), "");
- download_->OnAllDataSaved(DownloadItem::kEmptyFileHash);
+ download_->OnAllDataSaved(size, DownloadItem::kEmptyFileHash);
if (!download_manager_->GetDelegate() ||
download_manager_->GetDelegate()->ShouldCompleteDownload(
download_, base::Bind(&SavePackage::Finish, this))) {
@@ -741,10 +741,9 @@ void SavePackage::Finish() {
save_ids));
if (download_) {
- if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML) {
- download_->UpdateProgress(all_save_items_count_, CurrentSpeed(), "");
- download_->OnAllDataSaved(DownloadItem::kEmptyFileHash);
- }
+ if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML)
+ download_->OnAllDataSaved(all_save_items_count_,
+ DownloadItem::kEmptyFileHash);
download_->MarkAsComplete();
FinalizeDownloadEntry();
}
diff --git a/content/browser/renderer_host/resource_dispatcher_host_impl.cc b/content/browser/renderer_host/resource_dispatcher_host_impl.cc
index 75d9bdd..052e5ce 100644
--- a/content/browser/renderer_host/resource_dispatcher_host_impl.cc
+++ b/content/browser/renderer_host/resource_dispatcher_host_impl.cc
@@ -26,6 +26,7 @@
#include "content/browser/cert_store_impl.h"
#include "content/browser/child_process_security_policy_impl.h"
#include "content/browser/cross_site_request_manager.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_resource_handler.h"
#include "content/browser/download/save_file_manager.h"
#include "content/browser/download/save_file_resource_handler.h"
@@ -349,7 +350,8 @@ ResourceDispatcherHost* ResourceDispatcherHost::Get() {
}
ResourceDispatcherHostImpl::ResourceDispatcherHostImpl()
- : save_file_manager_(new SaveFileManager()),
+ : download_file_manager_(new DownloadFileManager(NULL)),
+ save_file_manager_(new SaveFileManager()),
request_id_(-1),
is_shutdown_(false),
max_outstanding_requests_cost_per_process_(
diff --git a/content/browser/renderer_host/resource_dispatcher_host_impl.h b/content/browser/renderer_host/resource_dispatcher_host_impl.h
index 8604d2f..8b82c10 100644
--- a/content/browser/renderer_host/resource_dispatcher_host_impl.h
+++ b/content/browser/renderer_host/resource_dispatcher_host_impl.h
@@ -35,6 +35,7 @@
#include "net/url_request/url_request.h"
#include "webkit/glue/resource_type.h"
+class DownloadFileManager;
class ResourceHandler;
class SaveFileManager;
class WebContentsImpl;
@@ -143,6 +144,10 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
// request. Experimentally obtained.
static const int kAvgBytesPerOutstandingRequest = 4400;
+ DownloadFileManager* download_file_manager() const {
+ return download_file_manager_;
+ }
+
SaveFileManager* save_file_manager() const {
return save_file_manager_;
}
@@ -351,6 +356,9 @@ class CONTENT_EXPORT ResourceDispatcherHostImpl
scoped_ptr<base::RepeatingTimer<ResourceDispatcherHostImpl> >
update_load_states_timer_;
+ // We own the download file writing thread and manager
+ scoped_refptr<DownloadFileManager> download_file_manager_;
+
// We own the save file manager.
scoped_refptr<SaveFileManager> save_file_manager_;
diff --git a/content/content_browser.gypi b/content/content_browser.gypi
index 3fcf64d..ac06a9d 100644
--- a/content/content_browser.gypi
+++ b/content/content_browser.gypi
@@ -69,7 +69,6 @@
'public/browser/devtools_manager.h',
'public/browser/dom_operation_notification_details.h',
'public/browser/dom_storage_context.h',
- 'public/browser/download_destination_observer.h',
'public/browser/download_interrupt_reason_values.h',
'public/browser/download_interrupt_reasons.h',
'public/browser/download_item.h',
@@ -317,10 +316,10 @@
'browser/download/download_create_info.cc',
'browser/download/download_create_info.h',
'browser/download/download_file.h',
- 'browser/download/download_file_factory.cc',
- 'browser/download/download_file_factory.h',
'browser/download/download_file_impl.cc',
'browser/download/download_file_impl.h',
+ 'browser/download/download_file_manager.cc',
+ 'browser/download/download_file_manager.h',
'browser/download/download_interrupt_reasons_impl.cc',
'browser/download/download_interrupt_reasons_impl.h',
'browser/download/download_item_factory.h',
diff --git a/content/content_tests.gypi b/content/content_tests.gypi
index c78eaad..7c46308 100644
--- a/content/content_tests.gypi
+++ b/content/content_tests.gypi
@@ -251,6 +251,7 @@
'browser/device_orientation/provider_unittest.cc',
'browser/download/base_file_unittest.cc',
'browser/download/byte_stream_unittest.cc',
+ 'browser/download/download_file_manager_unittest.cc',
'browser/download/download_file_unittest.cc',
'browser/download/download_id_unittest.cc',
'browser/download/download_item_impl_unittest.cc',
diff --git a/content/public/browser/download_destination_observer.h b/content/public/browser/download_destination_observer.h
deleted file mode 100644
index dbe6cce..0000000
--- a/content/public/browser/download_destination_observer.h
+++ /dev/null
@@ -1,37 +0,0 @@
-// 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.
-
-#ifndef CONTENT_PUBLIC_BROWSER_DOWNLOAD_DESTINATION_OBSERVER_H_
-#define CONTENT_PUBLIC_BROWSER_DOWNLOAD_DESTINATION_OBSERVER_H_
-
-#include <string>
-
-#include "content/public/browser/download_interrupt_reasons.h"
-
-namespace content {
-
-// Class that receives asynchronous events from a DownloadDestination about
-// downloading progress and completion. These should report status when the
-// data arrives at its final location; i.e. DestinationUpdate should be
-// called after the destination is finished with whatever operation it
-// is doing on the data described by |bytes_so_far| and DestinationCompleted
-// should only be called once that is true for all data.
-//
-// All methods are invoked on the UI thread.
-//
-// Note that this interface does not deal with cross-thread lifetime issues.
-class DownloadDestinationObserver {
- public:
- virtual void DestinationUpdate(int64 bytes_so_far,
- int64 bytes_per_sec,
- const std::string& hash_state) = 0;
-
- virtual void DestinationError(DownloadInterruptReason reason) = 0;
-
- virtual void DestinationCompleted(const std::string& final_hash) = 0;
-};
-
-} // namespace content
-
-#endif // CONTENT_PUBLIC_BROWSER_DOWNLOAD_DESTINATION_OBSERVER_H_
diff --git a/content/public/browser/download_manager.h b/content/public/browser/download_manager.h
index ea9ae73..7501f0e 100644
--- a/content/public/browser/download_manager.h
+++ b/content/public/browser/download_manager.h
@@ -132,10 +132,33 @@ class CONTENT_EXPORT DownloadManager
scoped_ptr<DownloadCreateInfo> info,
scoped_ptr<ByteStreamReader> stream) = 0;
+ // Notifications sent from the download thread to the UI thread
+ virtual void UpdateDownload(int32 download_id,
+ int64 bytes_so_far,
+ int64 bytes_per_sec,
+ const std::string& hash_state) = 0;
+
+ // |download_id| is the ID of the download.
+ // |size| is the number of bytes that have been downloaded.
+ // |hash| is sha256 hash for the downloaded file. It is empty when the hash
+ // is not available.
+ virtual void OnResponseCompleted(int32 download_id, int64 size,
+ const std::string& hash) = 0;
+
// Offthread target for cancelling a particular download. Will be a no-op
// if the download has already been cancelled.
virtual void CancelDownload(int32 download_id) = 0;
+ // Called when there is an error in the download.
+ // |download_id| is the ID of the download.
+ // |size| is the number of bytes that are currently downloaded.
+ // |hash_state| is the current state of the hash of the data that has been
+ // downloaded.
+ // |reason| is a download interrupt reason code.
+ virtual void OnDownloadInterrupted(
+ int32 download_id,
+ DownloadInterruptReason reason) = 0;
+
// Remove downloads after remove_begin (inclusive) and before remove_end
// (exclusive). You may pass in null Time values to do an unbounded delete
// in either direction.
diff --git a/content/public/test/mock_download_manager.h b/content/public/test/mock_download_manager.h
index 0e3a1ab..dd009dd 100644
--- a/content/public/test/mock_download_manager.h
+++ b/content/public/test/mock_download_manager.h
@@ -39,9 +39,19 @@ class MockDownloadManager : public DownloadManager {
scoped_ptr<ByteStreamReader> stream) OVERRIDE;
MOCK_METHOD2(MockStartDownload,
- content::DownloadId(DownloadCreateInfo*,
- ByteStreamReader*));
+ DownloadId(DownloadCreateInfo*,
+ ByteStreamReader*));
+ MOCK_METHOD4(UpdateDownload, void(int32 download_id,
+ int64 bytes_so_far,
+ int64 bytes_per_sec,
+ const std::string& hash_state));
+ MOCK_METHOD3(OnResponseCompleted, void(int32 download_id,
+ int64 size,
+ const std::string& hash));
MOCK_METHOD1(CancelDownload, void(int32 download_id));
+ MOCK_METHOD2(OnDownloadInterrupted,
+ void(int32 download_id,
+ DownloadInterruptReason reason));
MOCK_METHOD2(RemoveDownloadsBetween, int(base::Time remove_begin,
base::Time remove_end));
MOCK_METHOD1(RemoveDownloads, int(base::Time remove_begin));
diff --git a/content/public/test/test_file_error_injector.cc b/content/public/test/test_file_error_injector.cc
index 4aecd1b..26eaf8e 100644
--- a/content/public/test/test_file_error_injector.cc
+++ b/content/public/test/test_file_error_injector.cc
@@ -8,13 +8,14 @@
#include "base/compiler_specific.h"
#include "base/logging.h"
+#include "content/browser/download/download_create_info.h"
#include "content/browser/download/download_file_impl.h"
-#include "content/browser/download/download_file_factory.h"
+#include "content/browser/download/download_file_manager.h"
#include "content/browser/download/download_interrupt_reasons_impl.h"
-#include "content/browser/download/download_manager_impl.h"
#include "content/browser/power_save_blocker.h"
#include "content/browser/renderer_host/resource_dispatcher_host_impl.h"
#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/download_id.h"
#include "googleurl/src/gurl.h"
namespace content {
@@ -23,31 +24,35 @@ class ByteStreamReader;
namespace {
+DownloadFileManager* GetDownloadFileManager() {
+ content::ResourceDispatcherHostImpl* rdh =
+ content::ResourceDispatcherHostImpl::Get();
+ DCHECK(rdh != NULL);
+ return rdh->download_file_manager();
+}
+
// A class that performs file operations and injects errors.
class DownloadFileWithErrors: public DownloadFileImpl {
public:
- typedef base::Callback<void(const GURL& url)> ConstructionCallback;
+ typedef base::Callback<void(const GURL& url, content::DownloadId id)>
+ ConstructionCallback;
typedef base::Callback<void(const GURL& url)> DestructionCallback;
DownloadFileWithErrors(
- const content::DownloadSaveInfo& save_info,
- const GURL& url,
- const GURL& referrer_url,
- int64 received_bytes,
- bool calculate_hash,
+ const DownloadCreateInfo* info,
scoped_ptr<content::ByteStreamReader> stream,
+ DownloadRequestHandleInterface* request_handle,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
const net::BoundNetLog& bound_net_log,
- scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
- base::WeakPtr<content::DownloadDestinationObserver> observer,
const content::TestFileErrorInjector::FileErrorInfo& error_info,
const ConstructionCallback& ctor_callback,
const DestructionCallback& dtor_callback);
~DownloadFileWithErrors();
- virtual void Initialize(const InitializeCallback& callback) OVERRIDE;
-
// DownloadFile interface.
+ virtual content::DownloadInterruptReason Initialize() OVERRIDE;
virtual content::DownloadInterruptReason AppendDataToFile(
const char* data, size_t data_len) OVERRIDE;
virtual void Rename(const FilePath& full_path,
@@ -60,15 +65,12 @@ class DownloadFileWithErrors: public DownloadFileImpl {
content::TestFileErrorInjector::FileOperationCode code,
content::DownloadInterruptReason original_error);
- // Determine whether to overwrite an operation with the given code
- // with a substitute error; if returns true, |*original_error| is
- // written with the error to use for overwriting.
- // NOTE: This routine changes state; specifically, it increases the
- // operations counts for the specified code. It should only be called
- // once per operation.
- bool OverwriteError(
- content::TestFileErrorInjector::FileOperationCode code,
- content::DownloadInterruptReason* output_error);
+ // Used in place of original rename callback to intercept with
+ // ShouldReturnError.
+ void RenameErrorCallback(
+ const RenameCompletionCallback& original_callback,
+ content::DownloadInterruptReason original_error,
+ const FilePath& path_result);
// Source URL for the file being downloaded.
GURL source_url_;
@@ -84,66 +86,37 @@ class DownloadFileWithErrors: public DownloadFileImpl {
DestructionCallback destruction_callback_;
};
-static void InitializeErrorCallback(
- const content::DownloadFile::InitializeCallback original_callback,
- content::DownloadInterruptReason overwrite_error,
- content::DownloadInterruptReason original_error) {
- original_callback.Run(overwrite_error);
-}
-
-static void RenameErrorCallback(
- const content::DownloadFile::RenameCompletionCallback original_callback,
- content::DownloadInterruptReason overwrite_error,
- content::DownloadInterruptReason original_error,
- const FilePath& path_result) {
- original_callback.Run(
- overwrite_error,
- overwrite_error == content::DOWNLOAD_INTERRUPT_REASON_NONE ?
- path_result : FilePath());
-}
-
DownloadFileWithErrors::DownloadFileWithErrors(
- const content::DownloadSaveInfo& save_info,
- const GURL& url,
- const GURL& referrer_url,
- int64 received_bytes,
- bool calculate_hash,
+ const DownloadCreateInfo* info,
scoped_ptr<content::ByteStreamReader> stream,
+ DownloadRequestHandleInterface* request_handle,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
const net::BoundNetLog& bound_net_log,
- scoped_ptr<content::PowerSaveBlocker> power_save_blocker,
- base::WeakPtr<content::DownloadDestinationObserver> observer,
const content::TestFileErrorInjector::FileErrorInfo& error_info,
const ConstructionCallback& ctor_callback,
const DestructionCallback& dtor_callback)
- : DownloadFileImpl(
- save_info, url, referrer_url, received_bytes, calculate_hash,
- stream.Pass(), bound_net_log, power_save_blocker.Pass(),
- observer),
- source_url_(url),
+ : DownloadFileImpl(info,
+ stream.Pass(),
+ request_handle,
+ download_manager,
+ calculate_hash,
+ scoped_ptr<content::PowerSaveBlocker>(NULL).Pass(),
+ bound_net_log),
+ source_url_(info->url()),
error_info_(error_info),
destruction_callback_(dtor_callback) {
- ctor_callback.Run(source_url_);
+ ctor_callback.Run(source_url_, info->download_id);
}
DownloadFileWithErrors::~DownloadFileWithErrors() {
destruction_callback_.Run(source_url_);
}
-void DownloadFileWithErrors::Initialize(
- const InitializeCallback& callback) {
- content::DownloadInterruptReason error_to_return =
- content::DOWNLOAD_INTERRUPT_REASON_NONE;
- InitializeCallback callback_to_use = callback;
-
- // Replace callback if the error needs to be overwritten.
- if (OverwriteError(
+content::DownloadInterruptReason DownloadFileWithErrors::Initialize() {
+ return ShouldReturnError(
content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
- &error_to_return)) {
- callback_to_use = base::Bind(&InitializeErrorCallback, callback,
- error_to_return);
- }
-
- DownloadFileImpl::Initialize(callback_to_use);
+ DownloadFileImpl::Initialize());
}
content::DownloadInterruptReason DownloadFileWithErrors::AppendDataToFile(
@@ -157,42 +130,48 @@ void DownloadFileWithErrors::Rename(
const FilePath& full_path,
bool overwrite_existing_file,
const RenameCompletionCallback& callback) {
- content::DownloadInterruptReason error_to_return =
- content::DOWNLOAD_INTERRUPT_REASON_NONE;
- RenameCompletionCallback callback_to_use = callback;
-
- // Replace callback if the error needs to be overwritten.
- if (OverwriteError(
- content::TestFileErrorInjector::FILE_OPERATION_RENAME,
- &error_to_return)) {
- callback_to_use = base::Bind(&RenameErrorCallback, callback,
- error_to_return);
- }
-
- DownloadFileImpl::Rename(full_path, overwrite_existing_file, callback_to_use);
+ DownloadFileImpl::Rename(
+ full_path, overwrite_existing_file,
+ base::Bind(&DownloadFileWithErrors::RenameErrorCallback,
+ // Unretained since this'll only be called from
+ // the DownloadFileImpl slice of the same object.
+ base::Unretained(this), callback));
}
-bool DownloadFileWithErrors::OverwriteError(
+content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
content::TestFileErrorInjector::FileOperationCode code,
- content::DownloadInterruptReason* output_error) {
- int counter = operation_counter_[code]++;
+ content::DownloadInterruptReason original_error) {
+ int counter = operation_counter_[code];
+ ++operation_counter_[code];
if (code != error_info_.code)
- return false;
+ return original_error;
if (counter != error_info_.operation_instance)
- return false;
-
- *output_error = error_info_.error;
- return true;
+ return original_error;
+
+ VLOG(1) << " " << __FUNCTION__ << "()"
+ << " url = '" << source_url_.spec() << "'"
+ << " code = " << content::TestFileErrorInjector::DebugString(code)
+ << " (" << code << ")"
+ << " counter = " << counter
+ << " original_error = "
+ << content::InterruptReasonDebugString(original_error)
+ << " (" << original_error << ")"
+ << " new error = "
+ << content::InterruptReasonDebugString(error_info_.error)
+ << " (" << error_info_.error << ")";
+
+ return error_info_.error;
}
-content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
- content::TestFileErrorInjector::FileOperationCode code,
- content::DownloadInterruptReason original_error) {
- content::DownloadInterruptReason output_error = original_error;
- OverwriteError(code, &output_error);
- return output_error;
+void DownloadFileWithErrors::RenameErrorCallback(
+ const RenameCompletionCallback& original_callback,
+ content::DownloadInterruptReason original_error,
+ const FilePath& path_result) {
+ original_callback.Run(ShouldReturnError(
+ content::TestFileErrorInjector::FILE_OPERATION_RENAME,
+ original_error), path_result);
}
} // namespace
@@ -200,7 +179,8 @@ content::DownloadInterruptReason DownloadFileWithErrors::ShouldReturnError(
namespace content {
// A factory for constructing DownloadFiles that inject errors.
-class DownloadFileWithErrorsFactory : public DownloadFileFactory {
+class DownloadFileWithErrorsFactory
+ : public DownloadFileManager::DownloadFileFactory {
public:
DownloadFileWithErrorsFactory(
@@ -210,14 +190,11 @@ class DownloadFileWithErrorsFactory : public DownloadFileFactory {
// DownloadFileFactory interface.
virtual DownloadFile* CreateFile(
- const content::DownloadSaveInfo& save_info,
- GURL url,
- GURL referrer_url,
- int64 received_bytes,
- bool calculate_hash,
- scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
- base::WeakPtr<content::DownloadDestinationObserver> observer);
+ DownloadCreateInfo* info,
+ scoped_ptr<content::ByteStreamReader> stream,
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log);
bool AddError(
const TestFileErrorInjector::FileErrorInfo& error_info);
@@ -244,40 +221,32 @@ DownloadFileWithErrorsFactory::~DownloadFileWithErrorsFactory() {
}
content::DownloadFile* DownloadFileWithErrorsFactory::CreateFile(
- const content::DownloadSaveInfo& save_info,
- GURL url,
- GURL referrer_url,
- int64 received_bytes,
- bool calculate_hash,
+ DownloadCreateInfo* info,
scoped_ptr<content::ByteStreamReader> stream,
- const net::BoundNetLog& bound_net_log,
- base::WeakPtr<content::DownloadDestinationObserver> observer) {
- if (injected_errors_.find(url.spec()) == injected_errors_.end()) {
+ content::DownloadManager* download_manager,
+ bool calculate_hash,
+ const net::BoundNetLog& bound_net_log) {
+ std::string url = info->url().spec();
+
+ if (injected_errors_.find(url) == injected_errors_.end()) {
// Have to create entry, because FileErrorInfo is not a POD type.
TestFileErrorInjector::FileErrorInfo err_info = {
- url.spec(),
+ url,
TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
-1,
content::DOWNLOAD_INTERRUPT_REASON_NONE
};
- injected_errors_[url.spec()] = err_info;
+ injected_errors_[url] = err_info;
}
- scoped_ptr<content::PowerSaveBlocker> psb(
- new content::PowerSaveBlocker(
- content::PowerSaveBlocker::kPowerSaveBlockPreventAppSuspension,
- "Download in progress"));
return new DownloadFileWithErrors(
- save_info,
- url,
- referrer_url,
- received_bytes,
- calculate_hash,
+ info,
stream.Pass(),
+ new DownloadRequestHandle(info->request_handle),
+ download_manager,
+ calculate_hash,
bound_net_log,
- psb.Pass(),
- observer,
- injected_errors_[url.spec()],
+ injected_errors_[url],
construction_callback_,
destruction_callback_);
}
@@ -294,32 +263,47 @@ void DownloadFileWithErrorsFactory::ClearErrors() {
injected_errors_.clear();
}
-TestFileErrorInjector::TestFileErrorInjector(
- scoped_refptr<content::DownloadManager> download_manager)
- : created_factory_(NULL),
- // This code is only used for browser_tests, so a
- // DownloadManager is always a DownloadManagerImpl.
- download_manager_(
- static_cast<DownloadManagerImpl*>(download_manager.release())) {
+TestFileErrorInjector::TestFileErrorInjector()
+ : created_factory_(NULL) {
// Record the value of the pointer, for later validation.
created_factory_ =
new DownloadFileWithErrorsFactory(
- base::Bind(&TestFileErrorInjector::RecordDownloadFileConstruction,
+ base::Bind(&TestFileErrorInjector::
+ RecordDownloadFileConstruction,
this),
- base::Bind(&TestFileErrorInjector::RecordDownloadFileDestruction,
+ base::Bind(&TestFileErrorInjector::
+ RecordDownloadFileDestruction,
this));
- // We will transfer ownership of the factory to the download manager.
- scoped_ptr<DownloadFileFactory> download_file_factory(
+ // We will transfer ownership of the factory to the download file manager.
+ scoped_ptr<DownloadFileWithErrorsFactory> download_file_factory(
created_factory_);
- download_manager_->SetDownloadFileFactoryForTesting(
- download_file_factory.Pass());
+ content::BrowserThread::PostTask(
+ content::BrowserThread::FILE,
+ FROM_HERE,
+ base::Bind(&TestFileErrorInjector::AddFactory,
+ this,
+ base::Passed(&download_file_factory)));
}
TestFileErrorInjector::~TestFileErrorInjector() {
}
+void TestFileErrorInjector::AddFactory(
+ scoped_ptr<DownloadFileWithErrorsFactory> factory) {
+ DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
+
+ DownloadFileManager* download_file_manager = GetDownloadFileManager();
+ DCHECK(download_file_manager);
+
+ // Convert to base class pointer, for GCC.
+ scoped_ptr<DownloadFileManager::DownloadFileFactory> plain_factory(
+ factory.release());
+
+ download_file_manager->SetFileFactoryForTesting(plain_factory.Pass());
+}
+
bool TestFileErrorInjector::AddError(const FileErrorInfo& error_info) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK_LE(0, error_info.operation_instance);
@@ -341,16 +325,37 @@ bool TestFileErrorInjector::InjectErrors() {
ClearFoundFiles();
- DCHECK_EQ(static_cast<content::DownloadFileFactory*>(created_factory_),
- download_manager_->GetDownloadFileFactoryForTesting());
+ content::BrowserThread::PostTask(
+ content::BrowserThread::FILE,
+ FROM_HERE,
+ base::Bind(&TestFileErrorInjector::InjectErrorsOnFileThread,
+ this,
+ injected_errors_,
+ created_factory_));
- created_factory_->ClearErrors();
+ return true;
+}
- for (ErrorMap::const_iterator it = injected_errors_.begin();
- it != injected_errors_.end(); ++it)
- created_factory_->AddError(it->second);
+void TestFileErrorInjector::InjectErrorsOnFileThread(
+ ErrorMap map, DownloadFileWithErrorsFactory* factory) {
+ DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
- return true;
+ // Validate that our factory is in use.
+ DownloadFileManager* download_file_manager = GetDownloadFileManager();
+ DCHECK(download_file_manager);
+
+ DownloadFileManager::DownloadFileFactory* file_factory =
+ download_file_manager->GetFileFactoryForTesting();
+
+ // Validate that we still have the same factory.
+ DCHECK_EQ(static_cast<DownloadFileManager::DownloadFileFactory*>(factory),
+ file_factory);
+
+ // We want to replace all existing injection errors.
+ factory->ClearErrors();
+
+ for (ErrorMap::const_iterator it = map.begin(); it != map.end(); ++it)
+ factory->AddError(it->second);
}
size_t TestFileErrorInjector::CurrentFileCount() const {
@@ -370,16 +375,28 @@ bool TestFileErrorInjector::HadFile(const GURL& url) const {
return (found_files_.find(url) != found_files_.end());
}
+const content::DownloadId TestFileErrorInjector::GetId(
+ const GURL& url) const {
+ DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+ FileMap::const_iterator it = found_files_.find(url);
+ if (it == found_files_.end())
+ return content::DownloadId::Invalid();
+
+ return it->second;
+}
+
void TestFileErrorInjector::ClearFoundFiles() {
found_files_.clear();
}
-void TestFileErrorInjector::DownloadFileCreated(GURL url) {
+void TestFileErrorInjector::DownloadFileCreated(GURL url,
+ content::DownloadId id) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
DCHECK(files_.find(url) == files_.end());
- files_.insert(url);
- found_files_.insert(url);
+ files_[url] = id;
+ found_files_[url] = id;
}
void TestFileErrorInjector::DestroyingDownloadFile(GURL url) {
@@ -389,13 +406,15 @@ void TestFileErrorInjector::DestroyingDownloadFile(GURL url) {
files_.erase(url);
}
-void TestFileErrorInjector::RecordDownloadFileConstruction(const GURL& url) {
+void TestFileErrorInjector::RecordDownloadFileConstruction(
+ const GURL& url, content::DownloadId id) {
content::BrowserThread::PostTask(
content::BrowserThread::UI,
FROM_HERE,
base::Bind(&TestFileErrorInjector::DownloadFileCreated,
this,
- url));
+ url,
+ id));
}
void TestFileErrorInjector::RecordDownloadFileDestruction(const GURL& url) {
@@ -408,14 +427,13 @@ void TestFileErrorInjector::RecordDownloadFileDestruction(const GURL& url) {
}
// static
-scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create(
- scoped_refptr<content::DownloadManager> download_manager) {
+scoped_refptr<TestFileErrorInjector> TestFileErrorInjector::Create() {
static bool visited = false;
DCHECK(!visited); // Only allowed to be called once.
visited = true;
scoped_refptr<TestFileErrorInjector> single_injector(
- new TestFileErrorInjector(download_manager));
+ new TestFileErrorInjector);
return single_injector;
}
diff --git a/content/public/test/test_file_error_injector.h b/content/public/test/test_file_error_injector.h
index 91daf2b..a762343 100644
--- a/content/public/test/test_file_error_injector.h
+++ b/content/public/test/test_file_error_injector.h
@@ -6,22 +6,18 @@
#define CONTENT_PUBLIC_TEST_TEST_FILE_ERROR_INJECTOR_H_
#include <map>
-#include <set>
#include <string>
#include "base/memory/ref_counted.h"
#include "base/memory/scoped_ptr.h"
-#include "base/memory/ref_counted.h"
#include "content/public/browser/download_interrupt_reasons.h"
-class DownloadManagerImpl;
class GURL;
namespace content {
class DownloadId;
class DownloadFileWithErrorsFactory;
-class DownloadManager;
// Test helper for injecting errors into download file operations.
// All errors for a download must be injected before it starts.
@@ -40,7 +36,7 @@ class DownloadManager;
// FileErrorInfo b = { url2, ... };
//
// scoped_refptr<TestFileErrorInjector> injector =
-// TestFileErrorInjector::Create(download_manager);
+// TestFileErrorInjector::Create();
//
// injector->AddError(a);
// injector->AddError(b);
@@ -71,9 +67,7 @@ class TestFileErrorInjector
// Creates an instance. May only be called once.
// Lives until all callbacks (in the implementation) are complete and the
// creator goes out of scope.
- // TODO(rdsmith): Allow multiple calls for different download managers.
- static scoped_refptr<TestFileErrorInjector> Create(
- scoped_refptr<content::DownloadManager> download_manager);
+ static scoped_refptr<TestFileErrorInjector> Create();
// Adds an error.
// Must be called before |InjectErrors()| for a particular download file.
@@ -101,6 +95,9 @@ class TestFileErrorInjector
// Returns whether or not a file matching |url| has been created.
bool HadFile(const GURL& url) const;
+ // Gets the download ID associated with the file matching |url|.
+ const DownloadId GetId(const GURL& url) const;
+
// Resets the found file list.
void ClearFoundFiles();
@@ -109,20 +106,24 @@ class TestFileErrorInjector
private:
friend class base::RefCountedThreadSafe<TestFileErrorInjector>;
- typedef std::set<GURL> FileSet;
+ typedef std::map<GURL, DownloadId> FileMap;
- TestFileErrorInjector(
- scoped_refptr<content::DownloadManager> download_manager);
+ TestFileErrorInjector();
virtual ~TestFileErrorInjector();
+ void AddFactory(scoped_ptr<DownloadFileWithErrorsFactory> factory);
+
+ void InjectErrorsOnFileThread(ErrorMap map,
+ DownloadFileWithErrorsFactory* factory);
+
// Callbacks from the download file, to record lifetimes.
// These may be called on any thread.
- void RecordDownloadFileConstruction(const GURL& url);
+ void RecordDownloadFileConstruction(const GURL& url, DownloadId id);
void RecordDownloadFileDestruction(const GURL& url);
// These run on the UI thread.
- void DownloadFileCreated(GURL url);
+ void DownloadFileCreated(GURL url, DownloadId id);
void DestroyingDownloadFile(GURL url);
// All the data is used on the UI thread.
@@ -130,17 +131,14 @@ class TestFileErrorInjector
ErrorMap injected_errors_;
// Keep track of active DownloadFiles.
- FileSet files_;
+ FileMap files_;
// Keep track of found DownloadFiles.
- FileSet found_files_;
+ FileMap found_files_;
// The factory we created. May outlive this class.
DownloadFileWithErrorsFactory* created_factory_;
- // The download manager we set the factory on.
- scoped_refptr<DownloadManagerImpl> download_manager_;
-
DISALLOW_COPY_AND_ASSIGN(TestFileErrorInjector);
};