diff options
author | rdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-30 22:42:28 +0000 |
---|---|---|
committer | rdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-30 22:42:28 +0000 |
commit | 70f522d19798c26334e68e0b6d8c3d373ebda453 (patch) | |
tree | 8c82deb7c1ed7024f31df2a040d14c6cc38f7139 /content/browser/download | |
parent | fa9e3003a4304c441f0879c6742edb74ff2d10ee (diff) | |
download | chromium_src-70f522d19798c26334e68e0b6d8c3d373ebda453.zip chromium_src-70f522d19798c26334e68e0b6d8c3d373ebda453.tar.gz chromium_src-70f522d19798c26334e68e0b6d8c3d373ebda453.tar.bz2 |
Remove DownloadFileManager in favor of direct ownership of DownloadFiles.
This CL is equivalent to CLs
* http://codereview.chromium.org/10799005,
* http://codereview.chromium.org/10836293,
* https://chromiumcodereview.appspot.com/10823406, and
* http://codereview.chromium.org/10867065
which were previous attempts to land the same functionality.
R=benjhayden@chromium.org
TBR=jam@chromium.org
BUG=123998
BUG=144751
Review URL: https://chromiumcodereview.appspot.com/10900010
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@154294 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'content/browser/download')
23 files changed, 919 insertions, 1452 deletions
diff --git a/content/browser/download/base_file.cc b/content/browser/download/base_file.cc index 1d5d1e2..debf0fb 100644 --- a/content/browser/download/base_file.cc +++ b/content/browser/download/base_file.cc @@ -220,7 +220,6 @@ 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 7ada03c..c488c34 100644 --- a/content/browser/download/base_file.h +++ b/content/browser/download/base_file.h @@ -29,6 +29,8 @@ 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 e707a26..e53602d 100644 --- a/content/browser/download/download_file.h +++ b/content/browser/download/download_file.h @@ -11,7 +11,6 @@ #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 { @@ -24,6 +23,12 @@ 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 @@ -33,10 +38,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. - virtual DownloadInterruptReason Initialize() = 0; + // 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; // Rename the download file to |full_path|. If that file exists and // |overwrite_existing_file| is false, |full_path| will be uniquified by @@ -68,14 +73,11 @@ class CONTENT_EXPORT DownloadFile { // Returns the current (intermediate) state of the hash as a byte string. virtual std::string GetHashState() = 0; - // 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; + // For testing. Must be called on FILE thread. + static int GetNumberOfDownloadFiles(); - virtual std::string DebugString() const = 0; + protected: + static int number_active_objects_; }; } // namespace content diff --git a/content/browser/download/download_file_factory.cc b/content/browser/download/download_file_factory.cc new file mode 100644 index 0000000..8678607 --- /dev/null +++ b/content/browser/download/download_file_factory.cc @@ -0,0 +1,32 @@ +// 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 new file mode 100644 index 0000000..97cb22b --- /dev/null +++ b/content/browser/download/download_file_factory.h @@ -0,0 +1,44 @@ +// 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 a1ab62a..5bd2412 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_manager.h" +#include "content/public/browser/download_destination_observer.h" #include "content/browser/download/download_stats.h" #include "net/base/io_buffer.h" @@ -27,57 +27,72 @@ using content::DownloadManager; const int kUpdatePeriodMs = 500; const int kMaxTimeBlockingFileThreadMs = 1000; +int content::DownloadFile::number_active_objects_ = 0; + DownloadFileImpl::DownloadFileImpl( - const DownloadCreateInfo* info, - scoped_ptr<content::ByteStreamReader> stream, - DownloadRequestHandleInterface* request_handle, - scoped_refptr<DownloadManager> download_manager, + 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, - const net::BoundNetLog& bound_net_log) - : file_(info->save_info.file_path, - info->url(), - info->referrer_url, - info->received_bytes, + base::WeakPtr<content::DownloadDestinationObserver> observer) + : file_(save_info.file_path, + url, + referrer_url, + received_bytes, calculate_hash, - info->save_info.hash_state, - info->save_info.file_stream, + save_info.hash_state, + 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_; } -content::DownloadInterruptReason DownloadFileImpl::Initialize() { +void DownloadFileImpl::Initialize(const InitializeCallback& callback) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + update_timer_.reset(new base::RepeatingTimer<DownloadFileImpl>()); - net::Error result = file_.Initialize(); - if (result != net::OK) { - return content::ConvertNetErrorToInterruptReason( - result, content::DOWNLOAD_INTERRUPT_FROM_DISK); + + 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; } stream_reader_->RegisterCallback( base::Bind(&DownloadFileImpl::StreamActive, weak_factory_.GetWeakPtr())); download_start_ = base::TimeTicks::Now(); + // Initial pull from the straw. StreamActive(); - return content::DOWNLOAD_INTERRUPT_REASON_NONE; + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, base::Bind( + callback, content::DOWNLOAD_INTERRUPT_REASON_NONE)); + + ++number_active_objects_; } 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), @@ -91,6 +106,8 @@ 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. @@ -126,6 +143,14 @@ 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(); } @@ -163,36 +188,6 @@ 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; @@ -261,18 +256,18 @@ void DownloadFileImpl::StreamActive() { download_stats::RecordContiguousWriteTime(now - start); - // Take care of communication with our controller. + // Take care of communication with our observer. 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 controller. - // Our controller will clean us up. + // Shut down processing and signal an error to our observer. + // Our observer 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(&DownloadManager::OnDownloadInterrupted, - download_manager_, id_.local(), reason)); + base::Bind(&content::DownloadDestinationObserver::DestinationError, + observer_, reason)); } else if (state == content::ByteStreamReader::STREAM_COMPLETE) { // Signal successful completion and shut down processing. stream_reader_->RegisterCallback(base::Closure()); @@ -280,11 +275,12 @@ void DownloadFileImpl::StreamActive() { std::string hash; if (!GetHash(&hash) || file_.IsEmptyHash(hash)) hash.clear(); + SendUpdate(); BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, - base::Bind(&DownloadManager::OnResponseCompleted, - download_manager_, id_.local(), - BytesSoFar(), hash)); + base::Bind( + &content::DownloadDestinationObserver::DestinationCompleted, + observer_, hash)); } if (bound_net_log_.IsLoggingAllEvents()) { bound_net_log_.AddEvent( @@ -297,7 +293,12 @@ void DownloadFileImpl::StreamActive() { void DownloadFileImpl::SendUpdate() { BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, - base::Bind(&DownloadManager::UpdateDownload, - download_manager_, id_.local(), - BytesSoFar(), CurrentSpeed(), GetHashState())); + base::Bind(&content::DownloadDestinationObserver::DestinationUpdate, + observer_, BytesSoFar(), CurrentSpeed(), GetHashState())); +} + +// static +int content::DownloadFile::GetNumberOfDownloadFiles() { + return number_active_objects_; } + diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h index b3630e5..64ae527 100644 --- a/content/browser/download/download_file_impl.h +++ b/content/browser/download/download_file_impl.h @@ -14,13 +14,14 @@ #include "base/timer.h" #include "content/browser/download/base_file.h" #include "content/browser/download/byte_stream.h" -#include "content/browser/download/download_request_handle.h" +#include "content/public/browser/download_save_info.h" #include "net/base/net_log.h" struct DownloadCreateInfo; namespace content { class ByteStreamReader; +class DownloadDestinationObserver; class DownloadManager; class PowerSaveBlocker; } @@ -29,17 +30,27 @@ 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. - 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); + // 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); + virtual ~DownloadFileImpl(); // DownloadFile functions. - virtual content::DownloadInterruptReason Initialize() OVERRIDE; + virtual void Initialize(const InitializeCallback& callback) OVERRIDE; virtual void Rename(const FilePath& full_path, bool overwrite_existing_file, const RenameCompletionCallback& callback) OVERRIDE; @@ -52,11 +63,6 @@ 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. @@ -80,20 +86,9 @@ 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_; @@ -101,6 +96,8 @@ 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 deleted file mode 100644 index 7bebd92..0000000 --- a/content/browser/download/download_file_manager.cc +++ /dev/null @@ -1,227 +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_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 deleted file mode 100644 index 919f1c4..0000000 --- a/content/browser/download/download_file_manager.h +++ /dev/null @@ -1,180 +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. -// -// 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 deleted file mode 100644 index 52ce433..0000000 --- a/content/browser/download/download_file_manager_unittest.cc +++ /dev/null @@ -1,413 +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_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 24eaade..78da487 100644 --- a/content/browser/download/download_file_unittest.cc +++ b/content/browser/download/download_file_unittest.cc @@ -12,6 +12,7 @@ #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" @@ -25,7 +26,6 @@ using content::BrowserThread; using content::BrowserThreadImpl; using content::DownloadFile; using content::DownloadId; -using content::DownloadManager; using ::testing::_; using ::testing::AnyNumber; using ::testing::DoAll; @@ -48,14 +48,21 @@ class MockByteStreamReader : public content::ByteStreamReader { MOCK_METHOD1(RegisterCallback, void(const base::Closure&)); }; -class LocalMockDownloadManager : public content::MockDownloadManager { +class MockDownloadDestinationObserver + : public content::DownloadDestinationObserver { public: - MOCK_METHOD4(CurrentUpdateStatus, - void(int32, int64, int64, const std::string&)); - protected: - virtual ~LocalMockDownloadManager() {} + 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&)); }; + MATCHER(IsNullCallback, "") { return (arg.is_null()); } } // namespace @@ -73,12 +80,9 @@ 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() : - update_download_id_(-1), + observer_(new StrictMock<MockDownloadDestinationObserver>), + observer_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(observer_.get())), bytes_(-1), bytes_per_sec_(-1), hash_state_("xyzzy"), @@ -89,44 +93,35 @@ class DownloadFileTest : public testing::Test { ~DownloadFileTest() { } - void SetUpdateDownloadInfo(int32 id, int64 bytes, int64 bytes_per_sec, + void SetUpdateDownloadInfo(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() { - download_manager_->CurrentUpdateStatus( - update_download_id_, bytes_, bytes_per_sec_, hash_state_); + observer_->CurrentUpdateStatus(bytes_, bytes_per_sec_, hash_state_); } virtual void SetUp() { - download_manager_ = new StrictMock<LocalMockDownloadManager>; - EXPECT_CALL(*(download_manager_.get()), - UpdateDownload( - DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), - _, _, _)) + EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _)) .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()); @@ -139,30 +134,36 @@ 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( - &info, - scoped_ptr<content::ByteStreamReader>(input_stream_).Pass(), - new DownloadRequestHandle(), - download_manager_, calculate_hash, + content::DownloadSaveInfo(), + GURL(), // Source + GURL(), // Referrer + 0, // Received bytes + calculate_hash, + scoped_ptr<content::ByteStreamReader>(input_stream_), + net::BoundNetLog(), scoped_ptr<content::PowerSaveBlocker>(NULL).Pass(), - net::BoundNetLog())); + observer_factory_.GetWeakPtr())); EXPECT_CALL(*input_stream_, Read(_, _)) .WillOnce(Return(content::ByteStreamReader::STREAM_EMPTY)) .RetiresOnSaturation(); - content::DownloadInterruptReason result = download_file_->Initialize(); + + 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); + ::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()); @@ -232,19 +233,16 @@ class DownloadFileTest : public testing::Test { } void FinishStream(content::DownloadInterruptReason interrupt_reason, - bool check_download_manager) { + bool check_observer) { ::testing::Sequence s1; SetupFinishStream(interrupt_reason, s1); sink_callback_.Run(); VerifyStreamAndSize(); - if (check_download_manager) { - EXPECT_CALL(*download_manager_, OnResponseCompleted(_, _, _)); + if (check_observer) { + EXPECT_CALL(*(observer_.get()), DestinationCompleted(_)); loop_.RunAllPending(); - ::testing::Mock::VerifyAndClearExpectations(download_manager_.get()); - EXPECT_CALL(*(download_manager_.get()), - UpdateDownload( - DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), - _, _, _)) + ::testing::Mock::VerifyAndClearExpectations(observer_.get()); + EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _)) .Times(AnyNumber()) .WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo)); @@ -272,7 +270,9 @@ class DownloadFileTest : public testing::Test { } protected: - scoped_refptr<StrictMock<LocalMockDownloadManager> > download_manager_; + scoped_ptr<StrictMock<MockDownloadDestinationObserver> > observer_; + base::WeakPtrFactory<content::DownloadDestinationObserver> + observer_factory_; linked_ptr<net::FileStream> file_stream_; @@ -286,8 +286,7 @@ class DownloadFileTest : public testing::Test { // Sink callback data for stream. base::Closure sink_callback_; - // Latest update sent to the download manager. - int32 update_download_id_; + // Latest update sent to the observer. int64 bytes_; int64 bytes_per_sec_; std::string hash_state_; @@ -488,21 +487,12 @@ 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 - // DownloadManager. - EXPECT_CALL(*(download_manager_.get()), - OnResponseCompleted(DownloadId(kValidIdDomain, - kDummyDownloadId + 0).local(), - 0, _)); + // observer. + EXPECT_CALL(*(observer_.get()), DestinationCompleted(_)); FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, false); + loop_.RunAllPending(); DestroyDownloadFile(0); } @@ -513,10 +503,9 @@ TEST_F(DownloadFileTest, StreamEmptyError) { EXPECT_TRUE(file_util::PathExists(initial_path)); // Finish the download in error and make sure we see it on the - // DownloadManager. - EXPECT_CALL(*(download_manager_.get()), - OnDownloadInterrupted( - DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), + // observer. + EXPECT_CALL(*(observer_.get()), + DestinationError( content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)) .WillOnce(InvokeWithoutArgs( this, &DownloadFileTest::ConfirmUpdateDownloadInfo)); @@ -526,8 +515,7 @@ 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(*(download_manager_.get()), - CurrentUpdateStatus(kDummyDownloadId + 0, 0, _, _)); + EXPECT_CALL(*(observer_.get()), CurrentUpdateStatus(0, _, _)); FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false); @@ -545,13 +533,10 @@ TEST_F(DownloadFileTest, StreamNonEmptySuccess) { ::testing::Sequence s1; SetupDataAppend(chunks1, 2, s1); SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, s1); - EXPECT_CALL(*(download_manager_.get()), - OnResponseCompleted(DownloadId(kValidIdDomain, - kDummyDownloadId + 0).local(), - strlen(kTestData1) + strlen(kTestData2), - _)); + EXPECT_CALL(*(observer_.get()), DestinationCompleted(_)); sink_callback_.Run(); VerifyStreamAndSize(); + loop_.RunAllPending(); DestroyDownloadFile(0); } @@ -566,9 +551,8 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) { SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, s1); - EXPECT_CALL(*(download_manager_.get()), - OnDownloadInterrupted( - DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), + EXPECT_CALL(*(observer_.get()), + DestinationError( content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)) .WillOnce(InvokeWithoutArgs( this, &DownloadFileTest::ConfirmUpdateDownloadInfo)); @@ -578,9 +562,8 @@ 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(*(download_manager_.get()), - CurrentUpdateStatus(kDummyDownloadId + 0, - strlen(kTestData1) + strlen(kTestData2), + EXPECT_CALL(*(observer_.get()), + CurrentUpdateStatus(strlen(kTestData1) + strlen(kTestData2), _, _)); sink_callback_.Run(); @@ -590,7 +573,7 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) { } // Send some data, wait 3/4s of a second, run the message loop, and -// confirm the values the DownloadManager received are correct. +// confirm the values the observer 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 2d0482c..97b9978 100644 --- a/content/browser/download/download_item_impl.cc +++ b/content/browser/download/download_item_impl.cc @@ -19,7 +19,6 @@ #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" @@ -119,6 +118,19 @@ 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 { @@ -144,7 +156,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, DownloadId download_id, const DownloadPersistentStoreInfo& info, const net::BoundNetLog& bound_net_log) - : download_id_(download_id), + : is_save_package_download_(false), + download_id_(download_id), current_path_(info.path), target_path_(info.path), target_disposition_(TARGET_DISPOSITION_OVERWRITE), @@ -191,7 +204,8 @@ DownloadItemImpl::DownloadItemImpl( const DownloadCreateInfo& info, scoped_ptr<DownloadRequestHandleInterface> request_handle, const net::BoundNetLog& bound_net_log) - : request_handle_(request_handle.Pass()), + : is_save_package_download_(false), + request_handle_(request_handle.Pass()), download_id_(info.download_id), target_disposition_( (info.prompt_user_for_save_location) ? @@ -251,7 +265,8 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate, DownloadId download_id, const std::string& mime_type, const net::BoundNetLog& bound_net_log) - : request_handle_(new NullDownloadRequestHandle()), + : is_save_package_download_(true), + request_handle_(new NullDownloadRequestHandle()), download_id_(download_id), current_path_(path), target_path_(path), @@ -292,11 +307,22 @@ 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)); @@ -385,21 +411,6 @@ 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. @@ -456,10 +467,39 @@ void DownloadItemImpl::Cancel(bool user_cancel) { download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT); TransitionTo(CANCELLED); + + // Cancel and remove the download file. + // TODO(rdsmith/benjhayden): Remove condition as part of + // SavePackage integration. + if (!is_save_package_download_) { + CHECK(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 @@ -477,6 +517,21 @@ void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) { last_reason_ = reason; TransitionTo(INTERRUPTED); + + // Cancel and remove the download file. + // TODO(rdsmith/benjhayden): Remove condition as part of + // SavePackage integration. + if (!is_save_package_download_) { + CHECK(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); @@ -496,12 +551,17 @@ void DownloadItemImpl::DelayedDownloadOpened(bool auto_opened) { } void DownloadItemImpl::OnAllDataSaved( - int64 size, const std::string& final_hash) { + const std::string& final_hash) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK_EQ(IN_PROGRESS, state_); DCHECK(!all_data_saved_); all_data_saved_ = true; - ProgressComplete(size, final_hash); + + // Store final hash and null out intermediate serialized hash state. + hash_ = final_hash; + hash_state_ = ""; + UpdateObservers(); } @@ -666,6 +726,10 @@ bool DownloadItemImpl::TimeRemaining(base::TimeDelta* remaining) const { return true; } +bool DownloadItemImpl::IsSavePackageDownload() const { + return is_save_package_download_; +} + int64 DownloadItemImpl::CurrentSpeed() const { if (is_paused_) return 0; @@ -696,29 +760,40 @@ 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()); + // TODO(rdsmith/benjhayden): Remove as part of SavePackage integration. + if (is_save_package_download_) { + // Avoid doing anything on the file thread; there's nothing we control + // there. + OnDownloadFileReleased(); + return; + } + + CHECK(download_file_.get()); if (NeedsRename()) { - DownloadFileManager::RenameCompletionCallback callback = + content::DownloadFile::RenameCompletionCallback callback = base::Bind(&DownloadItemImpl::OnDownloadRenamedToFinalName, weak_ptr_factory_.GetWeakPtr()); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::RenameDownloadFile, - delegate_->GetDownloadFileManager(), GetGlobalId(), + base::Bind(&DownloadFile::Rename, + base::Unretained(download_file_.get()), GetTargetFilePath(), true, callback)); } else { // Complete the download and release the DownloadFile. - BrowserThread::PostTask( + BrowserThread::PostTaskAndReply( BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::CompleteDownload, - delegate_->GetDownloadFileManager(), GetGlobalId(), - base::Bind(&DownloadItemImpl::OnDownloadFileReleased, - weak_ptr_factory_.GetWeakPtr()))); + base::Bind(&DownloadFileDetach, base::Passed(download_file_.Pass())), + base::Bind(&DownloadItemImpl::OnDownloadFileReleased, + weak_ptr_factory_.GetWeakPtr())); } } @@ -727,6 +802,9 @@ 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() @@ -745,12 +823,34 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( delegate_->DownloadRenamedToFinalName(this); // Complete the download and release the DownloadFile. - BrowserThread::PostTask( + // 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(!is_save_package_download_); + CHECK(download_file_.get()); + BrowserThread::PostTaskAndReply( BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::CompleteDownload, - delegate_->GetDownloadFileManager(), GetGlobalId(), - base::Bind(&DownloadItemImpl::OnDownloadFileReleased, - weak_ptr_factory_.GetWeakPtr()))); + 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); } void DownloadItemImpl::OnDownloadFileReleased() { @@ -881,18 +981,31 @@ 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. - DownloadFileManager::RenameCompletionCallback callback = + DCHECK(!is_save_package_download_); + CHECK(download_file_.get()); + DownloadFile::RenameCompletionCallback callback = base::Bind(&DownloadItemImpl::OnDownloadRenamedToIntermediateName, weak_ptr_factory_.GetWeakPtr()); BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::RenameDownloadFile, - delegate_->GetDownloadFileManager(), GetGlobalId(), + base::Bind(&DownloadFile::Rename, + // Safe because we control download file lifetime. + base::Unretained(download_file_.get()), intermediate_path, false, callback)); } @@ -923,14 +1036,53 @@ FilePath DownloadItemImpl::GetUserVerifiedFilePath() const { GetTargetFilePath() : GetFullPath(); } -void DownloadItemImpl::OffThreadCancel() { +void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far, + int64 bytes_per_sec, + const std::string& hash_state) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - request_handle_->CancelRequest(); - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::CancelDownload, - delegate_->GetDownloadFileManager(), download_id_)); + 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); } void DownloadItemImpl::Init(bool active, @@ -1040,7 +1192,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { " etag = '%s'" " url_chain = \n\t\"%s\"\n\t" " full_path = \"%" PRFilePath "\"" - " target_path = \"%" PRFilePath "\"", + " target_path = \"%" PRFilePath "\"" + " has download file = %s", GetDbHandle(), GetTotalBytes(), GetReceivedBytes(), @@ -1051,7 +1204,8 @@ std::string DownloadItemImpl::DebugString(bool verbose) const { GetETag().c_str(), url_list.c_str(), GetFullPath().value().c_str(), - GetTargetFilePath().value().c_str()); + GetTargetFilePath().value().c_str(), + download_file_.get() ? "true" : "false"); } else { description += base::StringPrintf(" url = \"%s\"", url_list.c_str()); } @@ -1104,6 +1258,7 @@ base::Time DownloadItemImpl::GetEndTime() const { return end_time_; } void DownloadItemImpl::SetIsPersisted() { is_persisted_ = true; + UpdateObservers(); } bool DownloadItemImpl::IsPersisted() const { diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h index efd3ca2..40b1f7c 100644 --- a/content/browser/download/download_item_impl.h +++ b/content/browser/download/download_item_impl.h @@ -17,6 +17,7 @@ #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" @@ -25,8 +26,14 @@ class DownloadItemImplDelegate; +namespace content { +class DownloadFile; +} + // See download_item.h for usage. -class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { +class CONTENT_EXPORT DownloadItemImpl + : public content::DownloadItem, + public content::DownloadDestinationObserver { public: // Note that it is the responsibility of the caller to ensure that a // DownloadItemImplDelegate passed to a DownloadItemImpl constructor @@ -59,6 +66,9 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // 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 @@ -79,14 +89,6 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // 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(); @@ -108,14 +110,22 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { virtual void MarkAsComplete(); // Called when all data has been saved. Only has display effects. - virtual void OnAllDataSaved(int64 size, const std::string& final_hash); + virtual void OnAllDataSaved(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(); + // For dispatching on whether we're dealing with a SavePackage download. + virtual bool IsSavePackageDownload() const; + // Overridden from DownloadItem. virtual void AddObserver(DownloadItem::Observer* observer) OVERRIDE; virtual void RemoveObserver(DownloadItem::Observer* observer) OVERRIDE; @@ -197,6 +207,17 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { 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 @@ -212,12 +233,6 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // 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(); @@ -240,9 +255,17 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { 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(); + // Will be false for save package downloads retrieved from the history. + // TODO(rdsmith): Replace with a generalized enum for "download source". + const bool is_save_package_download_; + // The handle to the request information. Used for operations outside the // download system. scoped_ptr<DownloadRequestHandleInterface> request_handle_; @@ -398,6 +421,12 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // 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 4a889f7..0b9ea10 100644 --- a/content/browser/download/download_item_impl_delegate.cc +++ b/content/browser/download/download_item_impl_delegate.cc @@ -2,9 +2,11 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "base/logging.h" #include "content/browser/download/download_item_impl_delegate.h" +#include "base/logging.h" +#include "content/browser/download/download_file_factory.h" + class DownloadItemImpl; // Infrastructure in DownloadItemImplDelegate to assert invariant that @@ -25,12 +27,15 @@ void DownloadItemImplDelegate::Detach() { --count_; } -bool DownloadItemImplDelegate::ShouldOpenFileBasedOnExtension( - const FilePath& path) { +void DownloadItemImplDelegate::DelegateStart( + DownloadItemImpl* download_item) {} + +bool DownloadItemImplDelegate::ShouldOpenDownload(DownloadItemImpl* download) { return false; } -bool DownloadItemImplDelegate::ShouldOpenDownload(DownloadItemImpl* download) { +bool DownloadItemImplDelegate::ShouldOpenFileBasedOnExtension( + const FilePath& path) { return false; } diff --git a/content/browser/download/download_item_impl_delegate.h b/content/browser/download/download_item_impl_delegate.h index 0092454..a2f2db2 100644 --- a/content/browser/download/download_item_impl_delegate.h +++ b/content/browser/download/download_item_impl_delegate.h @@ -28,13 +28,23 @@ class CONTENT_EXPORT DownloadItemImplDelegate { void Attach(); void Detach(); - // Tests if a file type should be opened automatically. - virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path); + // 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; // 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 d2ce5dd..ff033ca 100644 --- a/content/browser/download/download_item_impl_unittest.cc +++ b/content/browser/download/download_item_impl_unittest.cc @@ -7,11 +7,13 @@ #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_manager.h" +#include "content/browser/download/download_file_factory.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" @@ -28,17 +30,16 @@ 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: - MockDelegate(DownloadFileManager* file_manager) - : file_manager_(file_manager) { - } - MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path)); + MOCK_METHOD1(DelegateStart, void(DownloadItemImpl* download)); MOCK_METHOD1(ShouldOpenDownload, bool(DownloadItemImpl* download)); + MOCK_METHOD1(ShouldOpenFileBasedOnExtension, bool(const FilePath& path)); MOCK_METHOD1(CheckForFileRemoval, void(DownloadItemImpl* download)); MOCK_METHOD1(MaybeCompleteDownload, void(DownloadItemImpl* download)); MOCK_CONST_METHOD0(GetBrowserContext, content::BrowserContext*()); @@ -67,53 +68,15 @@ 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 -// MockDownloadFileManager::Rename*DownloadFile as follows: -// EXPECT_CALL(mock_download_file_manager, -// RenameDownloadFile(_,_,_,_)) +// MockDownloadFile::Rename as follows: +// EXPECT_CALL(download_file, Rename(_,_,_)) // .WillOnce(ScheduleRenameCallback(new_path)); ACTION_P(ScheduleRenameCallback, new_path) { BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, - base::Bind(arg3, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path)); + base::Bind(arg2, content::DOWNLOAD_INTERRUPT_REASON_NONE, new_path)); } // Similarly for scheduling a completion callback. @@ -121,10 +84,6 @@ ACTION(ScheduleCompleteCallback) { BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(arg1)); } -MockDownloadFileManager::MockDownloadFileManager() - : DownloadFileManager(new MockDownloadFileFactory) { -} - } // namespace class DownloadItemTest : public testing::Test { @@ -184,8 +143,7 @@ class DownloadItemTest : public testing::Test { DownloadItemTest() : ui_thread_(BrowserThread::UI, &loop_), file_thread_(BrowserThread::FILE, &loop_), - file_manager_(new MockDownloadFileManager), - delegate_(file_manager_.get()) { + delegate_() { } ~DownloadItemTest() { @@ -226,6 +184,27 @@ 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); @@ -240,15 +219,10 @@ 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_; }; @@ -282,24 +256,30 @@ 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(kDownloadChunkSize, DownloadItem::kEmptyFileHash); + item->OnAllDataSaved(DownloadItem::kEmptyFileHash); ASSERT_TRUE(observer.CheckUpdated()); item->MarkAsComplete(); @@ -316,18 +296,24 @@ 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) { @@ -340,6 +326,8 @@ 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(); @@ -352,7 +340,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { DownloadItemImpl* safe_item = CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver safe_observer(safe_item); - safe_item->OnAllDataSaved(1, ""); + safe_item->OnAllDataSaved(""); EXPECT_TRUE(safe_observer.CheckUpdated()); safe_item->OnContentCheckCompleted( content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS); @@ -363,7 +351,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver unsafeurl_observer(unsafeurl_item); - unsafeurl_item->OnAllDataSaved(1, ""); + unsafeurl_item->OnAllDataSaved(""); EXPECT_TRUE(unsafeurl_observer.CheckUpdated()); unsafeurl_item->OnContentCheckCompleted( content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL); @@ -376,7 +364,7 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { CreateDownloadItem(DownloadItem::IN_PROGRESS); MockObserver unsafefile_observer(unsafefile_item); - unsafefile_item->OnAllDataSaved(1, ""); + unsafefile_item->OnAllDataSaved(""); EXPECT_TRUE(unsafefile_observer.CheckUpdated()); unsafefile_item->OnContentCheckCompleted( content::DOWNLOAD_DANGER_TYPE_DANGEROUS_FILE); @@ -387,18 +375,18 @@ TEST_F(DownloadItemTest, NotificationAfterOnContentCheckCompleted) { } // DownloadItemImpl::OnDownloadTargetDetermined will schedule a task to run -// DownloadFileManager::RenameDownloadFile(). Once the rename +// DownloadFile::Rename(). 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(*mock_file_manager(), - RenameDownloadFile(_,intermediate_path,false,_)) + EXPECT_CALL(*download_file, Rename(intermediate_path, false, _)) .WillOnce(ScheduleRenameCallback(new_intermediate_path)); // Currently, a notification would be generated if the danger type is anything @@ -411,6 +399,8 @@ TEST_F(DownloadItemTest, NotificationAfterOnDownloadTargetDetermined) { RunAllPendingInMessageLoops(); EXPECT_TRUE(observer.CheckUpdated()); EXPECT_EQ(new_intermediate_path, item->GetFullPath()); + + CleanupItem(item, download_file); } TEST_F(DownloadItemTest, NotificationAfterTogglePause) { @@ -426,12 +416,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(*mock_file_manager(), - RenameDownloadFile(_,_,false,_)) + EXPECT_CALL(*download_file, Rename(_, false, _)) .WillOnce(ScheduleRenameCallback(intermediate_path)); item->OnDownloadTargetDetermined(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE, @@ -443,6 +433,18 @@ 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. @@ -452,13 +454,13 @@ TEST_F(DownloadItemTest, DisplayName) { // 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(*mock_file_manager(), - RenameDownloadFile(item->GetGlobalId(), - intermediate_path, false, _)) + EXPECT_CALL(*download_file, Rename(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. @@ -473,17 +475,12 @@ TEST_F(DownloadItemTest, CallbackAfterRename) { intermediate_path); RunAllPendingInMessageLoops(); // All the callbacks should have happened by now. - ::testing::Mock::VerifyAndClearExpectations(mock_file_manager()); + ::testing::Mock::VerifyAndClearExpectations(download_file); ::testing::Mock::VerifyAndClearExpectations(mock_delegate()); - item->OnAllDataSaved(10, ""); - EXPECT_CALL(*mock_file_manager(), - RenameDownloadFile(item->GetGlobalId(), - final_path, true, _)) + item->OnAllDataSaved(""); + EXPECT_CALL(*download_file, Rename(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. @@ -496,20 +493,24 @@ 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(mock_file_manager()); + ::testing::Mock::VerifyAndClearExpectations(download_file); ::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()); @@ -522,11 +523,14 @@ 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) { @@ -537,6 +541,89 @@ 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 8f2b175..e1f31b0 100644 --- a/content/browser/download/download_manager_impl.cc +++ b/content/browser/download/download_manager_impl.cc @@ -21,7 +21,8 @@ #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_file_factory.h" +#include "content/browser/download/download_item_factory.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" @@ -52,28 +53,6 @@ using content::WebContents; namespace { -// This is just used to remember which DownloadItems come from SavePage. -class SavePageData : public base::SupportsUserData::Data { - public: - // A spoonful of syntactic sugar. - static bool Get(DownloadItem* item) { - return item->GetUserData(kKey) != NULL; - } - - explicit SavePageData(DownloadItem* item) { - item->SetUserData(kKey, this); - } - - virtual ~SavePageData() {} - - private: - static const char kKey[]; - - DISALLOW_COPY_AND_ASSIGN(SavePageData); -}; - -const char SavePageData::kKey[] = "DownloadItem SavePageData"; - void BeginDownload(content::DownloadUrlParameters* params) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); // ResourceDispatcherHost{Base} is-not-a URLRequest::Delegate, and @@ -144,23 +123,13 @@ class MapValueIteratorAdapter { // Allow copy and assign. }; -void EnsureNoPendingDownloadsOnFile(scoped_refptr<DownloadFileManager> dfm, - bool* result) { - if (dfm->NumberOfActiveDownloads()) - *result = false; +void EnsureNoPendingDownloadJobsOnFile(bool* result) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); + *result = (content::DownloadFile::GetNumberOfDownloadFiles() == 0); 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() {} @@ -203,8 +172,8 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); bool result = true; BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - base::Bind(&EnsureNoPendingDownloadJobsOnIO, &result)); + BrowserThread::FILE, FROM_HERE, + base::Bind(&EnsureNoPendingDownloadJobsOnFile, &result)); MessageLoop::current()->Run(); return result; } @@ -212,19 +181,20 @@ bool DownloadManager::EnsureNoPendingDownloadsForTesting() { } // namespace content DownloadManagerImpl::DownloadManagerImpl( - DownloadFileManager* file_manager, - scoped_ptr<content::DownloadItemFactory> factory, + scoped_ptr<content::DownloadItemFactory> item_factory, + scoped_ptr<content::DownloadFileFactory> file_factory, net::NetLog* net_log) - : factory_(factory.Pass()), + : item_factory_(item_factory.Pass()), + file_factory_(file_factory.Pass()), history_size_(0), shutdown_needed_(false), browser_context_(NULL), - file_manager_(file_manager), delegate_(NULL), net_log_(net_log) { - DCHECK(file_manager); - if (!factory_.get()) - factory_.reset(new DownloadItemFactoryImpl()); + if (!item_factory_.get()) + item_factory_.reset(new DownloadItemFactoryImpl()); + if (!file_factory_.get()) + file_factory_.reset(new content::DownloadFileFactory()); } DownloadManagerImpl::~DownloadManagerImpl() { @@ -243,8 +213,18 @@ DownloadId DownloadManagerImpl::GetNextId() { return id; } -DownloadFileManager* DownloadManagerImpl::GetDownloadFileManager() { - return file_manager_; +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); + } } bool DownloadManagerImpl::ShouldOpenDownload(DownloadItemImpl* item) { @@ -280,11 +260,7 @@ void DownloadManagerImpl::Shutdown() { FOR_EACH_OBSERVER(Observer, observers_, ManagerGoingDown(this)); // TODO(benjhayden): Consider clearing observers_. - DCHECK(file_manager_); - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(&DownloadFileManager::OnDownloadManagerShutdown, - file_manager_, make_scoped_refptr(this))); + // The DownloadFiles will be canceled and deleted by their DownloadItems. AssertContainersConsistent(); @@ -327,7 +303,6 @@ 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; @@ -391,65 +366,28 @@ 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)); - // |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; + net::BoundNetLog bound_net_log = + net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD); - DownloadFileManager::CreateDownloadFileCallback callback( - base::Bind(&DownloadManagerImpl::OnDownloadFileCreated, - this, download_id.local())); + // 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()); - 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); - } + return download->GetGlobalId(); } void DownloadManagerImpl::OnDownloadTargetDetermined( @@ -513,15 +451,13 @@ content::BrowserContext* DownloadManagerImpl::GetBrowserContext() const { return browser_context_; } -net::BoundNetLog DownloadManagerImpl::CreateDownloadItem( - DownloadCreateInfo* info) { +DownloadItemImpl* DownloadManagerImpl::CreateDownloadItem( + DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log) { 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 = factory_->CreateActiveItem( + DownloadItemImpl* download = item_factory_->CreateActiveItem( this, *info, scoped_ptr<DownloadRequestHandleInterface>( new DownloadRequestHandle(info->request_handle)).Pass(), @@ -533,7 +469,7 @@ net::BoundNetLog DownloadManagerImpl::CreateDownloadItem( active_downloads_[download->GetId()] = download; FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download)); - return bound_net_log; + return download; } DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem( @@ -543,7 +479,7 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem( DownloadItem::Observer* observer) { net::BoundNetLog bound_net_log = net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD); - DownloadItemImpl* download = factory_->CreateSavePageItem( + DownloadItemImpl* download = item_factory_->CreateSavePageItem( this, main_file_path, page_url, @@ -555,9 +491,6 @@ DownloadItemImpl* DownloadManagerImpl::CreateSavePackageDownloadItem( DCHECK(!ContainsKey(downloads_, download->GetId())); downloads_[download->GetId()] = download; - DCHECK(!SavePageData::Get(download)); - new SavePageData(download); - DCHECK(SavePageData::Get(download)); FOR_EACH_OBSERVER(Observer, observers_, OnDownloadCreated(this, download)); @@ -568,40 +501,6 @@ 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())); @@ -723,19 +622,6 @@ 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) { @@ -755,6 +641,16 @@ 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()) @@ -862,7 +758,7 @@ void DownloadManagerImpl::OnPersistentStoreQueryComplete( net::BoundNetLog bound_net_log = net::BoundNetLog::Make(net_log_, net::NetLog::SOURCE_DOWNLOAD); - DownloadItemImpl* download = factory_->CreatePersistedItem( + DownloadItemImpl* download = item_factory_->CreatePersistedItem( this, GetNextId(), entries->at(i), bound_net_log); DCHECK(!ContainsKey(downloads_, download->GetId())); downloads_[download->GetId()] = download; @@ -894,7 +790,6 @@ void DownloadManagerImpl::AddDownloadItemToHistory(DownloadItemImpl* download, NotifyModelChanged(); } - void DownloadManagerImpl::OnItemAddedToPersistentStore(int32 download_id, int64 db_handle) { // It's valid that we don't find a matching item, i.e. on shutdown. @@ -903,7 +798,7 @@ void DownloadManagerImpl::OnItemAddedToPersistentStore(int32 download_id, DownloadItemImpl* item = downloads_[download_id]; AddDownloadItemToHistory(item, db_handle); - if (SavePageData::Get(item)) { + if (item->IsSavePackageDownload()) { OnSavePageItemAddedToPersistentStore(item); } else { OnDownloadItemAddedToPersistentStore(item); @@ -1050,11 +945,6 @@ void DownloadManagerImpl::SavePageDownloadFinished( if (download->IsPersisted()) { if (delegate_) delegate_->UpdateItemInPersistentStore(download); - if (download->IsComplete()) - content::NotificationService::current()->Notify( - content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED, - content::Source<DownloadManager>(this), - content::Details<DownloadItem>(download)); } } @@ -1077,7 +967,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 receive an OnDownloadInterrupted() call + // store. If the rename failed, we processed an interrupt // before we receive the DownloadRenamedToIntermediateName() call. if (delegate_) { delegate_->AddItemToPersistentStore(download); @@ -1090,8 +980,7 @@ void DownloadManagerImpl::DownloadRenamedToIntermediateName( void DownloadManagerImpl::DownloadRenamedToFinalName( DownloadItemImpl* download) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - // If the rename failed, we receive an OnDownloadInterrupted() call before we - // receive the DownloadRenamedToFinalName() call. + // If the rename failed, we processed an interrupt before we get here. 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 8fab016f..5797c0a 100644 --- a/content/browser/download/download_manager_impl.h +++ b/content/browser/download/download_manager_impl.h @@ -15,7 +15,6 @@ #include "base/observer_list.h" #include "base/sequenced_task_runner_helpers.h" #include "base/synchronization/lock.h" -#include "content/browser/download/download_item_factory.h" #include "content/browser/download/download_item_impl_delegate.h" #include "content/common/content_export.h" #include "content/public/browser/download_manager.h" @@ -23,17 +22,26 @@ class DownloadFileManager; class DownloadItemImpl; +namespace content { +class DownloadItemFactory; +class DownloadFileFactory; +} + +namespace net { +class BoundNetLog; +} + class CONTENT_EXPORT DownloadManagerImpl : public content::DownloadManager, private DownloadItemImplDelegate { public: - // Caller guarantees that |file_manager| and |net_log| will remain valid + // Caller guarantees that |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(DownloadFileManager* file_manager, - scoped_ptr<content::DownloadItemFactory> factory, + DownloadManagerImpl(scoped_ptr<content::DownloadItemFactory> item_factory, + scoped_ptr<content::DownloadFileFactory> file_factory, net::NetLog* net_log); // Implementation functions (not part of the DownloadManager interface). @@ -61,16 +69,7 @@ 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; @@ -93,6 +92,11 @@ 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; @@ -107,8 +111,8 @@ class CONTENT_EXPORT DownloadManagerImpl virtual ~DownloadManagerImpl(); // Creates the download item. Must be called on the UI thread. - // Returns the |BoundNetLog| used by the |DownloadItem|. - virtual net::BoundNetLog CreateDownloadItem(DownloadCreateInfo* info); + virtual DownloadItemImpl* CreateDownloadItem( + DownloadCreateInfo* info, const net::BoundNetLog& bound_net_log); // Does nothing if |download_id| is not an active download. void MaybeCompleteDownloadById(int download_id); @@ -150,12 +154,6 @@ 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. @@ -174,7 +172,7 @@ class CONTENT_EXPORT DownloadManagerImpl // Overridden from DownloadItemImplDelegate // (Note that |GetBrowserContext| are present in both interfaces.) - virtual DownloadFileManager* GetDownloadFileManager() OVERRIDE; + virtual void DelegateStart(DownloadItemImpl* item) OVERRIDE; virtual bool ShouldOpenDownload(DownloadItemImpl* item) OVERRIDE; virtual bool ShouldOpenFileBasedOnExtension(const FilePath& path) OVERRIDE; virtual void CheckForFileRemoval(DownloadItemImpl* download_item) OVERRIDE; @@ -189,7 +187,10 @@ class CONTENT_EXPORT DownloadManagerImpl virtual void AssertStateConsistent(DownloadItemImpl* download) const OVERRIDE; // Factory for creation of downloads items. - scoped_ptr<content::DownloadItemFactory> factory_; + scoped_ptr<content::DownloadItemFactory> item_factory_; + + // Factory for the creation of download files. + scoped_ptr<content::DownloadFileFactory> file_factory_; // |downloads_| is the owning set for all downloads known to the // DownloadManager. This includes downloads started by the user in @@ -202,8 +203,7 @@ class CONTENT_EXPORT DownloadManagerImpl // until destruction. // // |active_downloads_| is a map of all downloads that are currently being - // processed. The key is the ID assigned by the DownloadFileManager, - // which is unique for the current session. + // processed. // // When a download is created through a user action, the corresponding // DownloadItem* is placed in |active_downloads_| and remains there until the @@ -227,9 +227,6 @@ 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 56bd819..4fc1a41 100644 --- a/content/browser/download/download_manager_impl_unittest.cc +++ b/content/browser/download/download_manager_impl_unittest.cc @@ -18,7 +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_file_factory.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" @@ -32,6 +32,7 @@ #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,9 +81,16 @@ class MockDownloadItemImpl : public DownloadItemImpl { MOCK_METHOD1(Cancel, void(bool)); MOCK_METHOD0(MarkAsComplete, void()); MOCK_METHOD1(DelayedDownloadOpened, void(bool)); - MOCK_METHOD2(OnAllDataSaved, void(int64, const std::string&)); + MOCK_METHOD1(OnAllDataSaved, void(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()); @@ -153,7 +161,6 @@ 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()); }; @@ -191,52 +198,17 @@ MockDownloadManagerDelegate::MockDownloadManagerDelegate() {} MockDownloadManagerDelegate::~MockDownloadManagerDelegate() {} -class MockDownloadFileManager : public DownloadFileManager { +class NullDownloadItemImplDelegate : public DownloadItemImplDelegate { public: - 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); + // 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(); } - - 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> { @@ -280,7 +252,7 @@ class MockDownloadItemFactory private: std::map<int32, MockDownloadItemImpl*> items_; - DownloadItemImplDelegate item_delegate_; + NullDownloadItemImplDelegate item_delegate_; DISALLOW_COPY_AND_ASSIGN(MockDownloadItemFactory); }; @@ -340,8 +312,14 @@ 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; } @@ -364,6 +342,36 @@ DownloadItemImpl* MockDownloadItemFactory::CreateSavePageItem( return result; } +class MockDownloadFileFactory + : public content::DownloadFileFactory, + public base::SupportsWeakPtr<MockDownloadFileFactory> { + public: + MockDownloadFileFactory() {} + virtual ~MockDownloadFileFactory() {} + + // Overridden method from DownloadFileFactory + MOCK_METHOD8(MockCreateFile, content::DownloadFile*( + const content::DownloadSaveInfo&, + GURL, GURL, int64, bool, + content::ByteStreamReader*, + const net::BoundNetLog&, + base::WeakPtr<content::DownloadDestinationObserver>)); + + 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) { + return MockCreateFile(save_info, url, referrer_url, received_bytes, + calculate_hash, stream.get(), bound_net_log, + observer); + } +}; + class MockBrowserContext : public content::BrowserContext { public: MockBrowserContext() {} @@ -416,28 +424,27 @@ class DownloadManagerTest : public testing::Test { // We tear down everything in TearDown(). ~DownloadManagerTest() {} - // Create a MockDownloadItemFactory, MockDownloadManagerDelegate, - // and MockDownloadFileManager, then create a DownloadManager that points + // Create a MockDownloadItemFactory and MockDownloadManagerDelegate, + // then create a DownloadManager that points // at all of those. virtual void SetUp() { DCHECK(!download_manager_.get()); mock_download_item_factory_ = (new MockDownloadItemFactory())->AsWeakPtr(); + mock_download_file_factory_ = (new MockDownloadFileFactory())->AsWeakPtr(); mock_download_manager_delegate_.reset( 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); + mock_download_item_factory_.get()).Pass(), + scoped_ptr<content::DownloadFileFactory>( + mock_download_file_factory_.get()).Pass(), NULL); observer_.reset(new MockDownloadManagerObserver()); EXPECT_CALL(GetMockObserver(), ModelChanged(download_manager_.get())) .WillOnce(Return()); @@ -461,9 +468,9 @@ class DownloadManagerTest : public testing::Test { download_manager_ = NULL; message_loop_.RunAllPending(); ASSERT_EQ(NULL, mock_download_item_factory_.get()); + ASSERT_EQ(NULL, mock_download_file_factory_.get()); message_loop_.RunAllPending(); mock_download_manager_delegate_.reset(); - mock_download_file_manager_ = NULL; mock_browser_context_.reset(); } @@ -479,12 +486,16 @@ class DownloadManagerTest : public testing::Test { ++next_download_id_; info.download_id = content::DownloadId(kDownloadIdDomain, id); info.request_handle = DownloadRequestHandle(); - download_manager_->CreateDownloadItem(&info); + EXPECT_CALL(GetMockObserver(), + OnDownloadCreated(download_manager_.get(), _)); + download_manager_->CreateDownloadItem(&info, net::BoundNetLog()); DCHECK(mock_download_item_factory_->GetItem(id)); MockDownloadItemImpl& item(*mock_download_item_factory_->GetItem(id)); - ON_CALL(item, GetId()) - .WillByDefault(Return(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>()); return item; } @@ -505,10 +516,6 @@ class DownloadManagerTest : public testing::Test { return *mock_download_manager_delegate_; } - MockDownloadFileManager& GetMockDownloadFileManager() { - return *mock_download_file_manager_; - } - MockDownloadManagerObserver& GetMockObserver() { return *observer_; } @@ -518,6 +525,10 @@ 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. @@ -550,6 +561,7 @@ class DownloadManagerTest : public testing::Test { protected: // Key test variable; we'll keep it available to sub-classes. scoped_refptr<DownloadManagerImpl> download_manager_; + base::WeakPtr<MockDownloadFileFactory> mock_download_file_factory_; private: MessageLoopForUI message_loop_; @@ -557,7 +569,6 @@ 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_; @@ -579,36 +590,47 @@ 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, _, _)); + EXPECT_CALL(*mock_download_file_factory_.get(), + MockCreateFile(Ref(info->save_info), _, _, 0, true, stream.get(), + _, _)); download_manager_->StartDownload(info.Pass(), stream.Pass()); EXPECT_TRUE(download_manager_->GetActiveDownloadItem(local_id)); } -// 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()); +// Confirm that calling DelegateStart behaves properly if the delegate +// blocks starting. +TEST_F(DownloadManagerTest, DelegateStart_True) { // Put a mock we have a handle to on the download manager. MockDownloadItemImpl& item(AddItemToManager()); - int download_id = item.GetId(); - content::DownloadInterruptReason reason( - content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED); + EXPECT_CALL(GetMockDownloadManagerDelegate(), + DetermineDownloadTarget(&item, _)) + .WillOnce(Return(true)); + DelegateStart(&item); +} - EXPECT_CALL(item, Interrupt(reason)); - download_manager_->OnDownloadInterrupted(download_id, reason); - EXPECT_EQ(&item, download_manager_->GetActiveDownloadItem(download_id)); +// 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); } // 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()); @@ -619,7 +641,6 @@ 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). @@ -630,8 +651,6 @@ 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(); @@ -649,7 +668,6 @@ 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 e42d0b4..96834c6 100644 --- a/content/browser/download/download_resource_handler.cc +++ b/content/browser/download/download_resource_handler.cc @@ -13,7 +13,6 @@ #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 d5c7781..9d02372 100644 --- a/content/browser/download/mock_download_file.cc +++ b/content/browser/download/mock_download_file.cc @@ -7,11 +7,19 @@ 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(Return(content::DOWNLOAD_INTERRUPT_REASON_NONE)); + ON_CALL(*this, Initialize(_)) + .WillByDefault(::testing::Invoke(SuccessRun)); } MockDownloadFile::~MockDownloadFile() { diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h index 34bcb54..9fb83bd 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_METHOD0(Initialize, content::DownloadInterruptReason()); + MOCK_METHOD1(Initialize, void(const InitializeCallback&)); MOCK_METHOD2(AppendDataToFile, content::DownloadInterruptReason( const char* data, size_t data_len)); MOCK_METHOD1(Rename, content::DownloadInterruptReason( @@ -42,7 +42,6 @@ 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 0ef878e..c043bc1 100644 --- a/content/browser/download/save_package.cc +++ b/content/browser/download/save_package.cc @@ -18,7 +18,6 @@ #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" @@ -35,6 +34,8 @@ #include "content/public/browser/content_browser_client.h" #include "content/public/browser/download_manager_delegate.h" #include "content/public/browser/navigation_entry.h" +#include "content/public/browser/notification_service.h" +#include "content/public/browser/notification_types.h" #include "content/public/browser/resource_context.h" #include "content/public/browser/web_contents.h" #include "content/public/common/url_constants.h" @@ -333,12 +334,19 @@ void SavePackage::OnMHTMLGenerated(const FilePath& path, int64 size) { return; } wrote_to_completed_file_ = true; - download_->SetTotalBytes(size); - download_->UpdateProgress(size, size, DownloadItem::kEmptyFileHash); - // Must call OnAllDataSaved here in order for - // GDataDownloadObserver::ShouldUpload() to return true. - // ShouldCompleteDownload() may depend on the gdata uploader to finish. - download_->OnAllDataSaved(size, DownloadItem::kEmptyFileHash); + + // Hack to avoid touching download_ after user cancel. + // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem + // with SavePackage flow. + if (download_->IsInProgress()) { + download_->SetTotalBytes(size); + download_->UpdateProgress(size, size, DownloadItem::kEmptyFileHash); + // Must call OnAllDataSaved here in order for + // GDataDownloadObserver::ShouldUpload() to return true. + // ShouldCompleteDownload() may depend on the gdata uploader to finish. + download_->OnAllDataSaved(DownloadItem::kEmptyFileHash); + } + if (!download_manager_->GetDelegate() || download_manager_->GetDelegate()->ShouldCompleteDownload( download_, base::Bind(&SavePackage::Finish, this))) { @@ -740,13 +748,17 @@ void SavePackage::Finish() { file_manager_, save_ids)); - if (download_) { - if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML) - download_->OnAllDataSaved(all_save_items_count_, - DownloadItem::kEmptyFileHash); + // Hack to avoid touching download_ after user cancel. + // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem + // with SavePackage flow. + if (download_ && download_->IsInProgress()) { + if (save_type_ != content::SAVE_PAGE_TYPE_AS_MHTML) { + download_->UpdateProgress(all_save_items_count_, CurrentSpeed(), ""); + download_->OnAllDataSaved(DownloadItem::kEmptyFileHash); + } download_->MarkAsComplete(); - FinalizeDownloadEntry(); } + FinalizeDownloadEntry(); } // Called for updating end state. @@ -766,7 +778,10 @@ void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) { // Inform the DownloadItem to update UI. // We use the received bytes as number of saved files. - if (download_) + // Hack to avoid touching download_ after user cancel. + // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem + // with SavePackage flow. + if (download_ && download_->IsInProgress()) download_->UpdateProgress(completed_count(), CurrentSpeed(), ""); if (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM && @@ -808,7 +823,10 @@ void SavePackage::SaveFailed(const GURL& save_url) { // Inform the DownloadItem to update UI. // We use the received bytes as number of saved files. - if (download_) + // Hack to avoid touching download_ after user cancel. + // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem + // with SavePackage flow. + if (download_ && download_->IsInProgress()) download_->UpdateProgress(completed_count(), CurrentSpeed(), ""); if ((save_type_ == content::SAVE_PAGE_TYPE_AS_ONLY_HTML) || @@ -1096,7 +1114,10 @@ void SavePackage::OnReceivedSavableResourceLinksForCurrentPage( static_cast<int>(frames_list.size()); // We use total bytes as the total number of files we want to save. - if (download_) + // Hack to avoid touching download_ after user cancel. + // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem + // with SavePackage flow. + if (download_ && download_->IsInProgress()) download_->SetTotalBytes(all_save_items_count_); if (all_save_items_count_) { @@ -1365,6 +1386,16 @@ void SavePackage::FinalizeDownloadEntry() { DCHECK(download_); DCHECK(download_manager_); + content::NotificationService::current()->Notify( + content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED, + // We use the DownloadManager as the source as that's a + // central SavePackage related location that observers can + // get to if they want to wait for notifications for a + // particular BrowserContext. Alternatively, we could make + // it come from the WebContents, which would be more specific + // but less useful to (current) customers. + content::Source<content::DownloadManager>(download_manager_), + content::Details<content::DownloadItem>(download_)); download_manager_->SavePageDownloadFinished(download_); StopObservation(); } |