diff options
author | rdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-07-11 19:53:26 +0000 |
---|---|---|
committer | rdsmith@chromium.org <rdsmith@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-07-11 19:53:26 +0000 |
commit | 7f02c4fb8c61bded5f386a7e4d056ec017d93d34 (patch) | |
tree | 13d380ee175d06ee81052fe40e193910bac42221 /content/browser/download | |
parent | 313ae0800d5a79af370ba29a4b2ec19fae9d3a97 (diff) | |
download | chromium_src-7f02c4fb8c61bded5f386a7e4d056ec017d93d34.zip chromium_src-7f02c4fb8c61bded5f386a7e4d056ec017d93d34.tar.gz chromium_src-7f02c4fb8c61bded5f386a7e4d056ec017d93d34.tar.bz2 |
Revert 146162 - Move Rename functionality from DownloadFileManager to DownloadFileImple.
BUG=123998
R=asanka@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10689093
TBR=rdsmith@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10702151
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@146177 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'content/browser/download')
-rw-r--r-- | content/browser/download/download_file.h | 19 | ||||
-rw-r--r-- | content/browser/download/download_file_impl.cc | 46 | ||||
-rw-r--r-- | content/browser/download/download_file_impl.h | 11 | ||||
-rw-r--r-- | content/browser/download/download_file_manager.cc | 38 | ||||
-rw-r--r-- | content/browser/download/download_file_manager.h | 5 | ||||
-rw-r--r-- | content/browser/download/download_file_manager_unittest.cc | 120 | ||||
-rw-r--r-- | content/browser/download/download_file_unittest.cc | 184 | ||||
-rw-r--r-- | content/browser/download/download_item_impl.cc | 29 | ||||
-rw-r--r-- | content/browser/download/download_item_impl.h | 4 | ||||
-rw-r--r-- | content/browser/download/download_item_impl_unittest.cc | 5 | ||||
-rw-r--r-- | content/browser/download/download_manager_impl.cc | 5 | ||||
-rw-r--r-- | content/browser/download/download_manager_impl.h | 2 | ||||
-rw-r--r-- | content/browser/download/download_manager_impl_unittest.cc | 6 | ||||
-rw-r--r-- | content/browser/download/mock_download_file.h | 4 |
14 files changed, 177 insertions, 301 deletions
diff --git a/content/browser/download/download_file.h b/content/browser/download/download_file.h index e707a26..59e8b9b 100644 --- a/content/browser/download/download_file.h +++ b/content/browser/download/download_file.h @@ -8,7 +8,6 @@ #include <string> #include "base/basictypes.h" -#include "base/callback_forward.h" #include "base/file_path.h" #include "content/common/content_export.h" #include "content/public/browser/download_id.h" @@ -24,13 +23,6 @@ class DownloadManager; // cancelled, the DownloadFile is destroyed. class CONTENT_EXPORT DownloadFile { public: - // 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 - // error. - typedef base::Callback<void(content::DownloadInterruptReason reason, - const FilePath& path)> RenameCompletionCallback; - virtual ~DownloadFile() {} // If calculate_hash is true, sha256 hash will be calculated. @@ -38,14 +30,9 @@ class CONTENT_EXPORT DownloadFile { // error code on failure. virtual DownloadInterruptReason Initialize() = 0; - // Rename the download file to |full_path|. If that file exists and - // |overwrite_existing_file| is false, |full_path| will be uniquified by - // suffixing " (<number>)" to the file name before the extension. - // Upon completion, |callback| will be called on the UI thread - // as per the comment above. - virtual void Rename(const FilePath& full_path, - bool overwrite_existing_file, - const RenameCompletionCallback& callback) = 0; + // Rename the download file. + // Returns net::OK on success, or a network error code on failure. + virtual DownloadInterruptReason Rename(const FilePath& full_path) = 0; // Detach the file so it is not deleted on destruction. virtual void Detach() = 0; diff --git a/content/browser/download/download_file_impl.cc b/content/browser/download/download_file_impl.cc index 8c42515..9d4a6880 100644 --- a/content/browser/download/download_file_impl.cc +++ b/content/browser/download/download_file_impl.cc @@ -88,39 +88,11 @@ content::DownloadInterruptReason DownloadFileImpl::AppendDataToFile( content::DOWNLOAD_INTERRUPT_FROM_DISK); } -void DownloadFileImpl::Rename(const FilePath& full_path, - bool overwrite_existing_file, - const RenameCompletionCallback& callback) { - FilePath new_path(full_path); - if (!overwrite_existing_file) { - // Make the file unique if requested. - int uniquifier = - file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL("")); - if (uniquifier > 0) { - new_path = new_path.InsertBeforeExtensionASCII( - StringPrintf(" (%d)", uniquifier)); - } - } - - net::Error rename_error = file_.Rename(new_path); - content::DownloadInterruptReason reason( - content::DOWNLOAD_INTERRUPT_REASON_NONE); - if (net::OK != rename_error) { - // Make sure our information is updated, since we're about to - // error out. - SendUpdate(); - - reason = - content::ConvertNetErrorToInterruptReason( - rename_error, - content::DOWNLOAD_INTERRUPT_FROM_DISK); - - new_path.clear(); - } - - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(callback, reason, new_path)); +content::DownloadInterruptReason DownloadFileImpl::Rename( + const FilePath& full_path) { + return content::ConvertNetErrorToInterruptReason( + file_.Rename(full_path), + content::DOWNLOAD_INTERRUPT_FROM_DISK); } void DownloadFileImpl::Detach() { @@ -218,10 +190,6 @@ void DownloadFileImpl::StreamActive() { base::TimeTicks write_start(base::TimeTicks::Now()); reason = AppendDataToFile( incoming_data.get()->data(), incoming_data_size); - // Note that if we're after a rename failure but before any - // cancel that our owner generates based on that rename failure, - // we'll get an ERR_UNEXPECTED from the above. Our consumers - // need to handle this situation. disk_writes_time_ += (base::TimeTicks::Now() - write_start); bytes_seen_ += incoming_data_size; total_incoming_data_size += incoming_data_size; @@ -270,11 +238,11 @@ void DownloadFileImpl::StreamActive() { // Our controller will clean us up. stream_reader_->RegisterCallback(base::Closure()); weak_factory_.InvalidateWeakPtrs(); - SendUpdate(); // Make info up to date before error. BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&DownloadManager::OnDownloadInterrupted, - download_manager_, id_.local(), reason)); + download_manager_, id_.local(), + BytesSoFar(), GetHashState(), reason)); } else if (state == content::ByteStreamReader::STREAM_COMPLETE) { // Signal successful completion and shut down processing. stream_reader_->RegisterCallback(base::Closure()); diff --git a/content/browser/download/download_file_impl.h b/content/browser/download/download_file_impl.h index b3630e5..1083132 100644 --- a/content/browser/download/download_file_impl.h +++ b/content/browser/download/download_file_impl.h @@ -40,9 +40,8 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile { // DownloadFile functions. virtual content::DownloadInterruptReason Initialize() OVERRIDE; - virtual void Rename(const FilePath& full_path, - bool overwrite_existing_file, - const RenameCompletionCallback& callback) OVERRIDE; + virtual content::DownloadInterruptReason Rename( + const FilePath& full_path) OVERRIDE; virtual void Detach() OVERRIDE; virtual void Cancel() OVERRIDE; virtual void AnnotateWithSourceInformation() OVERRIDE; @@ -64,13 +63,13 @@ class CONTENT_EXPORT DownloadFileImpl : virtual public content::DownloadFile { const char* data, size_t data_len); private: - // Send an update on our progress. - void SendUpdate(); - // Called when there's some activity on stream_reader_ that needs to be // handled. void StreamActive(); + // Send updates on our progress. + void SendUpdate(); + // The base file instance. BaseFile file_; diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc index 7bebd92..4fc4a86 100644 --- a/content/browser/download/download_file_manager.cc +++ b/content/browser/download/download_file_manager.cc @@ -195,14 +195,42 @@ void DownloadFileManager::RenameDownloadFile( 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())); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, FilePath())); return; } - download_file->Rename(full_path, overwrite_existing_file, callback); + FilePath new_path(full_path); + if (!overwrite_existing_file) { + // Make the file unique if requested. + int uniquifier = + file_util::GetUniquePathNumber(new_path, FILE_PATH_LITERAL("")); + if (uniquifier > 0) { + new_path = new_path.InsertBeforeExtensionASCII( + StringPrintf(" (%d)", uniquifier)); + } + } + + // Do the actual rename + content::DownloadInterruptReason rename_error = + download_file->Rename(new_path); + if (content::DOWNLOAD_INTERRUPT_REASON_NONE != rename_error) { + DownloadManager* download_manager = download_file->GetDownloadManager(); + DCHECK(download_manager); + + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + base::Bind(&DownloadManager::OnDownloadInterrupted, + download_manager, + global_id.local(), + download_file->BytesSoFar(), + download_file->GetHashState(), + rename_error)); + + new_path.clear(); + } + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(callback, new_path)); } int DownloadFileManager::NumberOfActiveDownloads() const { diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h index 919f1c4..bf2a53e 100644 --- a/content/browser/download/download_file_manager.h +++ b/content/browser/download/download_file_manager.h @@ -49,7 +49,6 @@ #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" @@ -62,6 +61,7 @@ class FilePath; namespace content { class ByteStreamReader; +class DownloadFile; class DownloadId; class DownloadManager; } @@ -82,8 +82,7 @@ class CONTENT_EXPORT DownloadFileManager CreateDownloadFileCallback; // Callback used with RenameDownloadFile(). - typedef content::DownloadFile::RenameCompletionCallback - RenameCompletionCallback; + typedef base::Callback<void(const FilePath&)> RenameCompletionCallback; class DownloadFileFactory { public: diff --git a/content/browser/download/download_file_manager_unittest.cc b/content/browser/download/download_file_manager_unittest.cc index 52ce433..04290d4 100644 --- a/content/browser/download/download_file_manager_unittest.cc +++ b/content/browser/download/download_file_manager_unittest.cc @@ -31,7 +31,6 @@ using ::testing::_; using ::testing::AtLeast; using ::testing::Mock; using ::testing::Return; -using ::testing::SaveArg; using ::testing::StrictMock; using ::testing::StrEq; @@ -40,10 +39,8 @@ 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)); + MOCK_METHOD2(OnDownloadRenamed, + void(int download_id, const FilePath& full_path)); private: ~TestDownloadManager() {} }; @@ -215,22 +212,48 @@ class DownloadFileManagerTest : public testing::Test { // |should_overwrite| indicates whether to replace or uniquify the file. void RenameFile(const DownloadId& id, const FilePath& new_path, - bool should_overwrite) { + const FilePath& unique_path, + content::DownloadInterruptReason rename_error, + RenameFileState state, + RenameFileOverwrite 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)); + EXPECT_CALL(*file, Rename(unique_path)) + .Times(1) + .WillOnce(Return(rename_error)); - content::DownloadFile::RenameCompletionCallback passed_callback( - base::Bind(&TestDownloadManager::OnDownloadRenamed, - download_manager_, id.local())); + if (rename_error != content::DOWNLOAD_INTERRUPT_REASON_NONE) { + EXPECT_CALL(*file, BytesSoFar()) + .Times(AtLeast(1)) + .WillRepeatedly(Return(byte_count_[id])); + EXPECT_CALL(*file, GetHashState()) + .Times(AtLeast(1)); + EXPECT_CALL(*file, GetDownloadManager()) + .Times(AtLeast(1)); + } download_file_manager_->RenameDownloadFile( - id, new_path, should_overwrite, passed_callback); + id, new_path, (should_overwrite == OVERWRITE), + base::Bind(&TestDownloadManager::OnDownloadRenamed, + download_manager_, id.local())); - EXPECT_TRUE(rename_callback.Equals(passed_callback)); + if (rename_error != content::DOWNLOAD_INTERRUPT_REASON_NONE) { + EXPECT_CALL(*download_manager_, + OnDownloadInterrupted( + id.local(), + byte_count_[id], + "", + rename_error)); + EXPECT_CALL(*download_manager_, + OnDownloadRenamed(id.local(), FilePath())); + ProcessAllPendingMessages(); + ++error_count_[id]; + } else { + EXPECT_CALL(*download_manager_, + OnDownloadRenamed(id.local(), unique_path)); + ProcessAllPendingMessages(); + } } void Complete(DownloadId id) { @@ -322,7 +345,42 @@ TEST_F(DownloadFileManagerTest, Complete) { Complete(dummy_id); } -TEST_F(DownloadFileManagerTest, Rename) { +TEST_F(DownloadFileManagerTest, RenameInProgress) { + 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, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE, + IN_PROGRESS, OVERWRITE); + + CleanUp(dummy_id); +} + +TEST_F(DownloadFileManagerTest, RenameInProgressWithUniquification) { + 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"))); + FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)"))); + ASSERT_EQ(0, file_util::WriteFile(foo, "", 0)); + RenameFile(dummy_id, foo, unique_foo, + content::DOWNLOAD_INTERRUPT_REASON_NONE, IN_PROGRESS, + DONT_OVERWRITE); + + CleanUp(dummy_id); +} + +TEST_F(DownloadFileManagerTest, RenameInProgressWithError) { scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); DownloadId dummy_id(download_manager_.get(), kDummyDownloadId); info->download_id = dummy_id; @@ -332,12 +390,14 @@ TEST_F(DownloadFileManagerTest, Rename) { CreateDownloadFile(info.Pass()); FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt"))); - RenameFile(dummy_id, foo, true); + RenameFile(dummy_id, foo, foo, + content::DOWNLOAD_INTERRUPT_REASON_FILE_NAME_TOO_LONG, + IN_PROGRESS, OVERWRITE); CleanUp(dummy_id); } -TEST_F(DownloadFileManagerTest, RenameNoOverwrite) { +TEST_F(DownloadFileManagerTest, RenameWithUniquification) { scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); DownloadId dummy_id(download_manager_.get(), kDummyDownloadId); info->download_id = dummy_id; @@ -347,7 +407,13 @@ TEST_F(DownloadFileManagerTest, RenameNoOverwrite) { CreateDownloadFile(info.Pass()); FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt"))); - RenameFile(dummy_id, foo, false); + FilePath unique_foo(foo.InsertBeforeExtension(FILE_PATH_LITERAL(" (1)"))); + // Create a file at |foo|. Since we are specifying DONT_OVERWRITE, + // RenameDownloadFile() should pick "foo (1).txt" instead of + // overwriting this file. + ASSERT_EQ(0, file_util::WriteFile(foo, "", 0)); + RenameFile(dummy_id, foo, unique_foo, + content::DOWNLOAD_INTERRUPT_REASON_NONE, COMPLETE, DONT_OVERWRITE); CleanUp(dummy_id); } @@ -363,10 +429,12 @@ TEST_F(DownloadFileManagerTest, RenameTwice) { FilePath crfoo(download_dir.path().Append( FILE_PATH_LITERAL("foo.txt.crdownload"))); - RenameFile(dummy_id, crfoo, true); + RenameFile(dummy_id, crfoo, crfoo, content::DOWNLOAD_INTERRUPT_REASON_NONE, + IN_PROGRESS, OVERWRITE); FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt"))); - RenameFile(dummy_id, foo, true); + RenameFile(dummy_id, foo, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE, + COMPLETE, OVERWRITE); CleanUp(dummy_id); } @@ -387,20 +455,24 @@ TEST_F(DownloadFileManagerTest, TwoDownloads) { FilePath crbar(download_dir.path().Append( FILE_PATH_LITERAL("bar.txt.crdownload"))); - RenameFile(dummy_id2, crbar, true); + RenameFile(dummy_id2, crbar, crbar, content::DOWNLOAD_INTERRUPT_REASON_NONE, + IN_PROGRESS, OVERWRITE); FilePath crfoo(download_dir.path().Append( FILE_PATH_LITERAL("foo.txt.crdownload"))); - RenameFile(dummy_id, crfoo, true); + RenameFile(dummy_id, crfoo, crfoo, content::DOWNLOAD_INTERRUPT_REASON_NONE, + IN_PROGRESS, OVERWRITE); FilePath bar(download_dir.path().Append(FILE_PATH_LITERAL("bar.txt"))); - RenameFile(dummy_id2, bar, true); + RenameFile(dummy_id2, bar, bar, content::DOWNLOAD_INTERRUPT_REASON_NONE, + COMPLETE, OVERWRITE); CleanUp(dummy_id2); FilePath foo(download_dir.path().Append(FILE_PATH_LITERAL("foo.txt"))); - RenameFile(dummy_id, foo, true); + RenameFile(dummy_id, foo, foo, content::DOWNLOAD_INTERRUPT_REASON_NONE, + COMPLETE, OVERWRITE); CleanUp(dummy_id); } diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc index aa07554..01009a8 100644 --- a/content/browser/download/download_file_unittest.cc +++ b/content/browser/download/download_file_unittest.cc @@ -5,7 +5,6 @@ #include "base/file_util.h" #include "base/message_loop.h" #include "base/string_number_conversions.h" -#include "base/test/test_file_util.h" #include "content/browser/browser_thread_impl.h" #include "content/browser/download/byte_stream.h" #include "content/browser/download/download_create_info.h" @@ -16,7 +15,6 @@ #include "content/public/browser/download_manager.h" #include "content/public/test/mock_download_manager.h" #include "net/base/file_stream.h" -#include "net/base/mock_file_stream.h" #include "net/base/net_errors.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -29,7 +27,6 @@ using content::DownloadManager; using ::testing::_; using ::testing::AnyNumber; using ::testing::DoAll; -using ::testing::InSequence; using ::testing::Return; using ::testing::SetArgPointee; using ::testing::StrictMock; @@ -48,14 +45,6 @@ class MockByteStreamReader : public content::ByteStreamReader { MOCK_METHOD1(RegisterCallback, void(const base::Closure&)); }; -class LocalMockDownloadManager : public content::MockDownloadManager { - public: - MOCK_METHOD4(CurrentUpdateStatus, - void(int32, int64, int64, const std::string&)); - protected: - virtual ~LocalMockDownloadManager() {} -}; - } // namespace DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain"; @@ -76,10 +65,6 @@ class DownloadFileTest : public testing::Test { // calling Release() on |download_manager_| won't ever result in its // destructor being called and we get a leak. DownloadFileTest() : - update_download_id_(-1), - bytes_(-1), - bytes_per_sec_(-1), - hash_state_("xyzzy"), ui_thread_(BrowserThread::UI, &loop_), file_thread_(BrowserThread::FILE, &loop_) { } @@ -89,19 +74,13 @@ class DownloadFileTest : public testing::Test { void SetUpdateDownloadInfo(int32 id, int64 bytes, int64 bytes_per_sec, const std::string& hash_state) { - update_download_id_ = id; bytes_ = bytes; bytes_per_sec_ = bytes_per_sec; hash_state_ = hash_state; } - void ConfirmUpdateDownloadInfo() { - download_manager_->CurrentUpdateStatus( - update_download_id_, bytes_, bytes_per_sec_, hash_state_); - } - virtual void SetUp() { - download_manager_ = new StrictMock<LocalMockDownloadManager>; + download_manager_ = new StrictMock<content::MockDownloadManager>; EXPECT_CALL(*(download_manager_.get()), UpdateDownload( DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), @@ -138,8 +117,8 @@ class DownloadFileTest : public testing::Test { .RetiresOnSaturation(); DownloadCreateInfo info; - // info.request_handle default constructed to null. info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset); + // info.request_handle default constructed to null. info.save_info.file_stream = file_stream_; download_file_.reset( new DownloadFileImpl( @@ -249,28 +228,8 @@ class DownloadFileTest : public testing::Test { } } - content::DownloadInterruptReason Rename( - const FilePath& full_path, bool overwrite_existing_file, - FilePath* result_path_p) { - base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this); - content::DownloadInterruptReason result_reason( - content::DOWNLOAD_INTERRUPT_REASON_NONE); - bool callback_was_called(false); - FilePath result_path; - - download_file_->Rename(full_path, overwrite_existing_file, - base::Bind(&DownloadFileTest::SetRenameResult, - weak_ptr_factory.GetWeakPtr(), - &callback_was_called, - &result_reason, result_path_p)); - loop_.RunAllPending(); - - EXPECT_TRUE(callback_was_called); - return result_reason; - } - protected: - scoped_refptr<StrictMock<LocalMockDownloadManager> > download_manager_; + scoped_refptr<StrictMock<content::MockDownloadManager> > download_manager_; linked_ptr<net::FileStream> file_stream_; @@ -285,7 +244,6 @@ class DownloadFileTest : public testing::Test { base::Closure sink_callback_; // Latest update sent to the download manager. - int32 update_download_id_; int64 bytes_; int64 bytes_per_sec_; std::string hash_state_; @@ -293,19 +251,6 @@ class DownloadFileTest : public testing::Test { MessageLoop loop_; private: - void SetRenameResult(bool* called_p, - content::DownloadInterruptReason* reason_p, - FilePath* result_path_p, - content::DownloadInterruptReason reason, - const FilePath& result_path) { - if (called_p) - *called_p = true; - if (reason_p) - *reason_p = reason; - if (result_path_p) - *result_path_p = result_path; - } - // UI thread. BrowserThreadImpl ui_thread_; // File thread to satisfy debug checks in DownloadFile. @@ -336,15 +281,12 @@ TEST_F(DownloadFileTest, RenameFileFinal) { FilePath path_2(initial_path.InsertBeforeExtensionASCII("_2")); FilePath path_3(initial_path.InsertBeforeExtensionASCII("_3")); FilePath path_4(initial_path.InsertBeforeExtensionASCII("_4")); - FilePath path_5(initial_path.InsertBeforeExtensionASCII("_5")); - FilePath output_path; // Rename the file before downloading any data. EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_1, false, &output_path)); + download_file_->Rename(path_1)); FilePath renamed_path = download_file_->FullPath(); EXPECT_EQ(path_1, renamed_path); - EXPECT_EQ(path_1, output_path); // Check the files. EXPECT_FALSE(file_util::PathExists(initial_path)); @@ -356,10 +298,9 @@ TEST_F(DownloadFileTest, RenameFileFinal) { // Rename the file after downloading some data. EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_2, false, &output_path)); + download_file_->Rename(path_2)); renamed_path = download_file_->FullPath(); EXPECT_EQ(path_2, renamed_path); - EXPECT_EQ(path_2, output_path); // Check the files. EXPECT_FALSE(file_util::PathExists(path_1)); @@ -370,10 +311,9 @@ TEST_F(DownloadFileTest, RenameFileFinal) { // Rename the file after downloading all the data. EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_3, false, &output_path)); + download_file_->Rename(path_3)); renamed_path = download_file_->FullPath(); EXPECT_EQ(path_3, renamed_path); - EXPECT_EQ(path_3, output_path); // Check the files. EXPECT_FALSE(file_util::PathExists(path_2)); @@ -387,10 +327,9 @@ TEST_F(DownloadFileTest, RenameFileFinal) { // Rename the file after downloading all the data and closing the file. EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_4, false, &output_path)); + download_file_->Rename(path_4)); renamed_path = download_file_->FullPath(); EXPECT_EQ(path_4, renamed_path); - EXPECT_EQ(path_4, output_path); // Check the files. EXPECT_FALSE(file_util::PathExists(path_3)); @@ -400,81 +339,6 @@ TEST_F(DownloadFileTest, RenameFileFinal) { EXPECT_TRUE(download_file_->GetHash(&hash)); EXPECT_EQ(kDataHash, base::HexEncode(hash.data(), hash.size())); - // Check that a rename with overwrite to an existing file succeeds. - std::string file_contents; - ASSERT_FALSE(file_util::PathExists(path_5)); - static const char file_data[] = "xyzzy"; - ASSERT_EQ(static_cast<int>(sizeof(file_data) - 1), - file_util::WriteFile(path_5, file_data, sizeof(file_data) - 1)); - ASSERT_TRUE(file_util::PathExists(path_5)); - EXPECT_TRUE(file_util::ReadFileToString(path_5, &file_contents)); - EXPECT_EQ(std::string(file_data), file_contents); - - EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_5, true, &output_path)); - EXPECT_EQ(path_5, output_path); - - file_contents = ""; - EXPECT_TRUE(file_util::ReadFileToString(path_5, &file_contents)); - EXPECT_NE(std::string(file_data), file_contents); - - DestroyDownloadFile(0); -} - -// Test to make sure the rename uniquifies if we aren't overwriting -// and there's a file where we're aiming. -TEST_F(DownloadFileTest, RenameUniquifies) { - ASSERT_TRUE(CreateDownloadFile(0, true)); - FilePath initial_path(download_file_->FullPath()); - EXPECT_TRUE(file_util::PathExists(initial_path)); - FilePath path_1(initial_path.InsertBeforeExtensionASCII("_1")); - FilePath path_1_suffixed(path_1.InsertBeforeExtensionASCII(" (1)")); - - ASSERT_FALSE(file_util::PathExists(path_1)); - static const char file_data[] = "xyzzy"; - ASSERT_EQ(static_cast<int>(sizeof(file_data)), - file_util::WriteFile(path_1, file_data, sizeof(file_data))); - ASSERT_TRUE(file_util::PathExists(path_1)); - - EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(path_1, false, NULL)); - EXPECT_TRUE(file_util::PathExists(path_1_suffixed)); - - FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true); - loop_.RunAllPending(); - DestroyDownloadFile(0); -} - -// Test to make sure we get the proper error on failure. -TEST_F(DownloadFileTest, RenameError) { - ASSERT_TRUE(CreateDownloadFile(0, true)); - FilePath initial_path(download_file_->FullPath()); - - // Create a subdirectory and rename into it. - FilePath tempdir(initial_path.DirName().Append(FILE_PATH_LITERAL("tempdir"))); - ASSERT_TRUE(file_util::CreateDirectory(tempdir)); - FilePath new_path(tempdir.Append(initial_path.BaseName())); - ASSERT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE, - Rename(new_path, true, NULL)); - - // Targets - FilePath path_1(new_path.InsertBeforeExtensionASCII("_1")); - FilePath path_1_suffixed(path_1.InsertBeforeExtensionASCII(" (1)")); - ASSERT_FALSE(file_util::PathExists(path_1)); - ASSERT_FALSE(file_util::PathExists(path_1_suffixed)); - - // Make the directory unwritable and try to rename within it. - { - file_util::PermissionRestorer restorer(tempdir); - ASSERT_TRUE(file_util::MakeFileUnwritable(tempdir)); - - EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_FILE_ACCESS_DENIED, - Rename(path_1, true, NULL)); - EXPECT_FALSE(file_util::PathExists(path_1_suffixed)); - } - - FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true); - loop_.RunAllPending(); DestroyDownloadFile(0); } @@ -516,22 +380,10 @@ TEST_F(DownloadFileTest, StreamEmptyError) { EXPECT_CALL(*(download_manager_.get()), OnDownloadInterrupted( DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), - content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)) - .WillOnce(InvokeWithoutArgs( - this, &DownloadFileTest::ConfirmUpdateDownloadInfo)); - - // If this next EXPECT_CALL fails flakily, it's probably a real failure. - // We'll be getting a stream of UpdateDownload calls from the timer, and - // 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, _, _)); - + 0, _, + content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)); FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false); - loop_.RunAllPending(); - DestroyDownloadFile(0); } @@ -564,26 +416,12 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) { SetupDataAppend(chunks1, 2, s1); SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, s1); - EXPECT_CALL(*(download_manager_.get()), OnDownloadInterrupted( DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(), - content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)) - .WillOnce(InvokeWithoutArgs( - this, &DownloadFileTest::ConfirmUpdateDownloadInfo)); - - // If this next EXPECT_CALL fails flakily, it's probably a real failure. - // We'll be getting a stream of UpdateDownload calls from the timer, and - // 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), - _, _)); - + strlen(kTestData1) + strlen(kTestData2), _, + content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED)); sink_callback_.Run(); - loop_.RunAllPending(); VerifyStreamAndSize(); DestroyDownloadFile(0); } diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc index 1912cce..3c9521d 100644 --- a/content/browser/download/download_item_impl.cc +++ b/content/browser/download/download_item_impl.cc @@ -489,19 +489,8 @@ void DownloadItemImpl::Cancel(bool user_cancel) { // An error occurred somewhere. void DownloadItemImpl::Interrupt(content::DownloadInterruptReason reason) { - // Somewhat counter-intuitively, it is possible for us to receive an - // interrupt after we've already been interrupted. The generation of - // interrupts from the file thread Renames and the generation of - // interrupts from disk writes go through two different mechanisms (driven - // by rename requests from UI thread and by write requests from IO thread, - // respectively), and since we choose not to keep state on the File thread, - // this is the place where the races collide. It's also possible for - // interrupts to race with cancels. - - // Whatever happens, the first one to hit the UI thread wins. - if (!IsInProgress()) - return; - + // It should not be possible both to have an error and complete. + DCHECK(IsInProgress()); last_reason_ = reason; TransitionTo(INTERRUPTED); download_stats::RecordDownloadInterrupted( @@ -750,7 +739,6 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) { void DownloadItemImpl::OnDownloadRenamedToFinalName( DownloadFileManager* file_manager, - content::DownloadInterruptReason reason, const FilePath& full_path) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); @@ -760,13 +748,12 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName( << " " << DebugString(false); DCHECK(NeedsRename()); - if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) { - Interrupt(reason); + if (full_path.empty()) + // Indicates error; also reported + // by DownloadManagerImpl::OnDownloadInterrupted. return; - } // full_path is now the current and target file path. - DCHECK(!full_path.empty()); target_path_ = full_path; SetFullPath(full_path); delegate_->DownloadRenamedToFinalName(this); @@ -788,16 +775,12 @@ void DownloadItemImpl::OnDownloadFileReleased() { } void DownloadItemImpl::OnDownloadRenamedToIntermediateName( - content::DownloadInterruptReason reason, const FilePath& full_path) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - if (content::DOWNLOAD_INTERRUPT_REASON_NONE != reason) { - Interrupt(reason); - } else { + if (!full_path.empty()) { SetFullPath(full_path); UpdateObservers(); } - delegate_->DownloadRenamedToIntermediateName(this); } diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h index a48154c..af0b505 100644 --- a/content/browser/download/download_item_impl.h +++ b/content/browser/download/download_item_impl.h @@ -253,13 +253,11 @@ class CONTENT_EXPORT DownloadItemImpl : public content::DownloadItem { // Callback invoked when the download has been renamed to its final name. void OnDownloadRenamedToFinalName(DownloadFileManager* file_manager, - content::DownloadInterruptReason reason, const FilePath& full_path); // Callback invoked when the download has been renamed to its intermediate // name. - void OnDownloadRenamedToIntermediateName( - content::DownloadInterruptReason reason, const FilePath& full_path); + void OnDownloadRenamedToIntermediateName(const FilePath& full_path); // Callback from file thread when we release the DownloadFile. void OnDownloadFileReleased(); diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc index 3224a83..c7ac84d 100644 --- a/content/browser/download/download_item_impl_unittest.cc +++ b/content/browser/download/download_item_impl_unittest.cc @@ -101,9 +101,8 @@ class MockDownloadFileManager : public DownloadFileManager { // RenameDownloadFile(_,_,_,_)) // .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)); + BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, + base::Bind(arg3, new_path)); } // Similarly for scheduling a completion callback. diff --git a/content/browser/download/download_manager_impl.cc b/content/browser/download/download_manager_impl.cc index b798899..f742c7c 100644 --- a/content/browser/download/download_manager_impl.cc +++ b/content/browser/download/download_manager_impl.cc @@ -428,7 +428,7 @@ content::DownloadId DownloadManagerImpl::StartDownload( void DownloadManagerImpl::OnDownloadFileCreated( int32 download_id, content::DownloadInterruptReason reason) { if (reason != content::DOWNLOAD_INTERRUPT_REASON_NONE) { - OnDownloadInterrupted(download_id, reason); + OnDownloadInterrupted(download_id, 0, "", 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 @@ -771,12 +771,15 @@ void DownloadManagerImpl::DownloadStopped(DownloadItem* download) { void DownloadManagerImpl::OnDownloadInterrupted( int32 download_id, + int64 size, + const std::string& hash_state, content::DownloadInterruptReason reason) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); DownloadItem* download = GetActiveDownload(download_id); if (!download) return; + download->UpdateProgress(size, 0, hash_state); download->Interrupt(reason); } diff --git a/content/browser/download/download_manager_impl.h b/content/browser/download/download_manager_impl.h index 6edebaa..ad79935 100644 --- a/content/browser/download/download_manager_impl.h +++ b/content/browser/download/download_manager_impl.h @@ -58,6 +58,8 @@ class CONTENT_EXPORT DownloadManagerImpl virtual void CancelDownload(int32 download_id) OVERRIDE; virtual void OnDownloadInterrupted( int32 download_id, + int64 size, + const std::string& hash_state, content::DownloadInterruptReason reason) OVERRIDE; virtual int RemoveDownloadsBetween(base::Time remove_begin, base::Time remove_end) OVERRIDE; diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc index da28c5e..124d4ce 100644 --- a/content/browser/download/download_manager_impl_unittest.cc +++ b/content/browser/download/download_manager_impl_unittest.cc @@ -513,11 +513,15 @@ TEST_F(DownloadManagerTest, OnDownloadInterrupted) { content::MockDownloadItem& item(AddItemToManager()); int download_id = item.GetId(); + int64 size = 0xdeadbeef; + const std::string hash_state("Undead beef"); content::DownloadInterruptReason reason( content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED); + EXPECT_CALL(item, UpdateProgress(size, 0, hash_state)); EXPECT_CALL(item, Interrupt(reason)); - download_manager_->OnDownloadInterrupted(download_id, reason); + download_manager_->OnDownloadInterrupted( + download_id, size, hash_state, reason); EXPECT_EQ(&item, GetActiveDownloadItem(download_id)); } diff --git a/content/browser/download/mock_download_file.h b/content/browser/download/mock_download_file.h index 34bcb54..8608136 100644 --- a/content/browser/download/mock_download_file.h +++ b/content/browser/download/mock_download_file.h @@ -29,9 +29,6 @@ class MockDownloadFile : virtual public content::DownloadFile { const char* data, size_t data_len)); MOCK_METHOD1(Rename, content::DownloadInterruptReason( const FilePath& full_path)); - MOCK_METHOD3(Rename, void(const FilePath& full_path, - bool overwrite_existing_file, - const RenameCompletionCallback& callback)); MOCK_METHOD0(Detach, void()); MOCK_METHOD0(Cancel, void()); MOCK_METHOD0(Finish, void()); @@ -43,7 +40,6 @@ class MockDownloadFile : virtual public content::DownloadFile { 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*()); MOCK_CONST_METHOD0(GlobalId, const content::DownloadId&()); |