Index: content/browser/download/download_manager_impl_unittest.cc |
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc |
index f59f2920a3967ea306cc75655302aa7ae626c3ab..4d2d5b12e771437ee675285514aa47c057e25d29 100644 |
--- a/content/browser/download/download_manager_impl_unittest.cc |
+++ b/content/browser/download/download_manager_impl_unittest.cc |
@@ -18,7 +18,6 @@ |
#include "build/build_config.h" |
#include "content/browser/download/byte_stream.h" |
#include "content/browser/download/download_create_info.h" |
-#include "content/browser/download/download_file_manager.h" |
#include "content/browser/download/download_item_factory.h" |
#include "content/browser/download/download_item_impl.h" |
#include "content/browser/download/download_item_impl_delegate.h" |
@@ -32,6 +31,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" |
@@ -77,9 +77,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) { |
+ MockStart(download_file.get()); |
+ } |
+ |
+ MOCK_METHOD1(MockStart, void(content::DownloadFile*)); |
+ |
MOCK_METHOD1(Interrupt, void(DownloadInterruptReason)); |
MOCK_METHOD1(Delete, void(DeleteReason)); |
MOCK_METHOD0(Remove, void()); |
@@ -88,7 +95,7 @@ class MockDownloadItemImpl : public DownloadItemImpl { |
MOCK_CONST_METHOD0(PercentComplete, int()); |
MOCK_CONST_METHOD0(AllDataSaved, bool()); |
MOCK_METHOD0(TogglePause, void()); |
- MOCK_METHOD1(OnDownloadCompleting, void(DownloadFileManager*)); |
+ MOCK_METHOD0(OnDownloadCompleting, void()); |
MOCK_CONST_METHOD1(MatchesQuery, bool(const string16& query)); |
MOCK_CONST_METHOD0(IsPartialDownload, bool()); |
MOCK_CONST_METHOD0(IsInProgress, bool()); |
@@ -103,8 +110,7 @@ class MockDownloadItemImpl : public DownloadItemImpl { |
content::DownloadDangerType)); |
MOCK_METHOD1(OnTargetPathSelected, void(const FilePath&)); |
MOCK_METHOD1(OnContentCheckCompleted, void(content::DownloadDangerType)); |
- MOCK_METHOD2(OnIntermediatePathDetermined, void(DownloadFileManager*, |
- const FilePath&)); |
+ MOCK_METHOD1(OnIntermediatePathDetermined, void(const FilePath&)); |
MOCK_CONST_METHOD0(GetState, DownloadState()); |
MOCK_CONST_METHOD0(GetUrlChain, const std::vector<GURL>&()); |
MOCK_METHOD1(SetTotalBytes, void(int64)); |
@@ -157,7 +163,7 @@ class MockDownloadItemImpl : public DownloadItemImpl { |
MOCK_CONST_METHOD0(GetFileNameToReportUser, FilePath()); |
MOCK_METHOD1(SetDisplayName, void(const FilePath&)); |
MOCK_CONST_METHOD0(GetUserVerifiedFilePath, FilePath()); |
- MOCK_METHOD1(OffThreadCancel, void(DownloadFileManager* file_manager)); |
+ MOCK_METHOD0(OffThreadCancel, void()); |
MOCK_CONST_METHOD1(DebugString, std::string(bool)); |
MOCK_METHOD0(MockDownloadOpenForTesting, void()); |
MOCK_METHOD1(GetExternalData, ExternalData*(const void*)); |
@@ -198,52 +204,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); |
+ // Save 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) { |
+ 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> { |
@@ -289,7 +260,7 @@ class MockDownloadItemFactory |
private: |
std::map<int32, MockDownloadItemImpl*> items_; |
- DownloadItemImplDelegate item_delegate_; |
+ NullDownloadItemImplDelegate item_delegate_; |
DISALLOW_COPY_AND_ASSIGN(MockDownloadItemFactory); |
}; |
@@ -350,8 +321,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; |
} |
@@ -412,8 +389,8 @@ 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()); |
@@ -423,15 +400,11 @@ class DownloadManagerTest : public testing::Test { |
new StrictMock<MockDownloadManagerDelegate>); |
EXPECT_CALL(*mock_download_manager_delegate_.get(), Shutdown()) |
.WillOnce(Return()); |
- mock_download_file_manager_ = new StrictMock<MockDownloadFileManager>; |
- EXPECT_CALL(*mock_download_file_manager_.get(), |
- OnDownloadManagerShutdown(_)); |
mock_browser_context_.reset(new StrictMock<MockBrowserContext>); |
EXPECT_CALL(*mock_browser_context_.get(), IsOffTheRecord()) |
.WillRepeatedly(Return(false)); |
download_manager_ = new DownloadManagerImpl( |
- mock_download_file_manager_.get(), |
scoped_ptr<content::DownloadItemFactory>( |
mock_download_item_factory_.get()).Pass(), NULL); |
download_manager_->SetDelegate(mock_download_manager_delegate_.get()); |
@@ -453,7 +426,6 @@ class DownloadManagerTest : public testing::Test { |
ASSERT_EQ(NULL, mock_download_item_factory_.get()); |
message_loop_.RunAllPending(); |
mock_download_manager_delegate_.reset(); |
- mock_download_file_manager_ = NULL; |
mock_browser_context_.reset(); |
} |
@@ -469,12 +441,14 @@ class DownloadManagerTest : public testing::Test { |
++next_download_id_; |
info.download_id = content::DownloadId(kDownloadIdDomain, id); |
info.request_handle = DownloadRequestHandle(); |
- download_manager_->CreateDownloadItem(&info); |
+ 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; |
} |
@@ -495,15 +469,15 @@ class DownloadManagerTest : public testing::Test { |
return *mock_download_manager_delegate_; |
} |
- MockDownloadFileManager& GetMockDownloadFileManager() { |
- return *mock_download_file_manager_; |
- } |
- |
// Probe at private internals. |
void DownloadStopped(DownloadItemImpl* item) { |
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. |
@@ -543,7 +517,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_; |
int next_download_id_; |
@@ -562,15 +535,26 @@ TEST_F(DownloadManagerTest, StartDownload) { |
.WillOnce(Return(content::DownloadId(this, local_id))); |
EXPECT_CALL(GetMockDownloadManagerDelegate(), GenerateFileHash()) |
.WillOnce(Return(true)); |
- EXPECT_CALL(GetMockDownloadFileManager(), MockCreateDownloadFile( |
- info.get(), static_cast<content::ByteStreamReader*>(NULL), |
- download_manager_.get(), true, _, _)); |
download_manager_->StartDownload(info.Pass(), stream.Pass()); |
EXPECT_TRUE(download_manager_->GetActiveDownloadItem(local_id)); |
} |
-// Does the DownloadManager prompt when requested? |
+// Confirm that calling DelegateStart behaves properly if the delegate |
+// blocks starting. |
+TEST_F(DownloadManagerTest, DelegateStart_False) { |
+ // Put a mock we have a handle to on the download manager. |
+ MockDownloadItemImpl& item(AddItemToManager()); |
+ int download_id = item.GetId(); |
+ |
+ EXPECT_CALL(GetMockDownloadManagerDelegate(), |
+ ShouldStartDownload(download_id)) |
+ .WillOnce(Return(false)); |
+ DelegateStart(&item); |
+} |
+ |
+// Does the DownloadManager prompt when requested? Lumping in |
+// test for DelegateStart when the delegate doesn't block the download in here. |
TEST_F(DownloadManagerTest, RestartDownload) { |
// Put a mock we have a handle to on the download manager. |
MockDownloadItemImpl& item(AddItemToManager()); |
@@ -584,10 +568,13 @@ TEST_F(DownloadManagerTest, RestartDownload) { |
FilePath expected_path(download_dir.path().Append( |
FILE_PATH_LITERAL("location"))); |
+ EXPECT_CALL(GetMockDownloadManagerDelegate(), |
+ ShouldStartDownload(download_id)) |
+ .WillOnce(Return(true)); |
EXPECT_CALL(item, GetTargetDisposition()) |
.WillOnce(Return(DownloadItem::TARGET_DISPOSITION_PROMPT)); |
EXPECT_CALL(GetMockDownloadManagerDelegate(), ChooseDownloadPath(&item)); |
- download_manager_->RestartDownload(download_id); |
+ DelegateStart(&item); |
// The alternative pathway goes straight to OnTargetPathAvailable, |
// so it more naturally belongs below. |
@@ -616,25 +603,10 @@ TEST_F(DownloadManagerTest, OnTargetPathAvailable) { |
EXPECT_CALL(item, GetTargetFilePath()) |
.WillRepeatedly(ReturnRef(target_path)); |
EXPECT_CALL(item, OnIntermediatePathDetermined( |
- &GetMockDownloadFileManager(), intermediate_path)); |
+ intermediate_path)); |
download_manager_->RestartDownload(item.GetId()); |
} |
-// Do the results of an OnDownloadInterrupted get passed through properly |
-// to the DownloadItem? |
-TEST_F(DownloadManagerTest, OnDownloadInterrupted) { |
- // 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(item, Interrupt(reason)); |
- download_manager_->OnDownloadInterrupted(download_id, reason); |
- EXPECT_EQ(&item, download_manager_->GetActiveDownloadItem(download_id)); |
-} |
- |
// Does DownloadStopped remove Download from appropriate queues? |
// This test tests non-persisted downloads. |
TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) { |
@@ -648,7 +620,7 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_NonPersisted) { |
EXPECT_CALL(item, GetDbHandle()) |
.WillRepeatedly(Return(DownloadItem::kUninitializedHandle)); |
- EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager())); |
+ 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). |
@@ -676,7 +648,7 @@ TEST_F(DownloadManagerTest, OnDownloadStopped_Persisted) { |
EXPECT_CALL(item, GetDbHandle()) |
.WillRepeatedly(Return(db_handle)); |
- EXPECT_CALL(item, OffThreadCancel(&GetMockDownloadFileManager())); |
+ EXPECT_CALL(item, OffThreadCancel()); |
DownloadStopped(&item); |
EXPECT_EQ(NULL, download_manager_->GetActiveDownloadItem(download_id)); |
} |