content/browser/download/download_file_unittest.cc - Issue 10918024: Revert 154294 - Remove DownloadFileManager in favor of direct ownership of DownloadFiles.

Unified Diff: content/browser/download/download_file_unittest.cc

Issue 10918024: Revert 154294 - Remove DownloadFileManager in favor of direct ownership of DownloadFiles. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src/

Patch Set: Created 8 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: content/browser/download/download_file_unittest.cc

===================================================================

--- content/browser/download/download_file_unittest.cc (revision 154444)

+++ content/browser/download/download_file_unittest.cc (working copy)

@@ -12,7 +12,6 @@

#include "content/browser/download/download_file_impl.h"

#include "content/browser/download/download_request_handle.h"

#include "content/browser/power_save_blocker.h"

-#include "content/public/browser/download_destination_observer.h"

#include "content/public/browser/download_interrupt_reasons.h"

#include "content/public/browser/download_manager.h"

#include "content/public/test/mock_download_manager.h"

@@ -26,6 +25,7 @@

using content::BrowserThreadImpl;

using content::DownloadFile;

using content::DownloadId;

+using content::DownloadManager;

using ::testing::_;

using ::testing::AnyNumber;

using ::testing::DoAll;

@@ -48,21 +48,14 @@

MOCK_METHOD1(RegisterCallback, void(const base::Closure&));

};

-class MockDownloadDestinationObserver

- : public content::DownloadDestinationObserver {

+class LocalMockDownloadManager : public content::MockDownloadManager {

public:

- MOCK_METHOD3(DestinationUpdate, void(int64, int64, const std::string&));

- MOCK_METHOD1(DestinationError, void(content::DownloadInterruptReason));

- MOCK_METHOD1(DestinationCompleted, void(const std::string&));

- // Doesn't override any methods in the base class. Used to make sure

- // that the last DestinationUpdate before a Destination{Completed,Error}

- // had the right values.

- MOCK_METHOD3(CurrentUpdateStatus,

- void(int64, int64, const std::string&));

+ MOCK_METHOD4(CurrentUpdateStatus,

+ void(int32, int64, int64, const std::string&));

+ protected:

+ virtual ~LocalMockDownloadManager() {}

};

MATCHER(IsNullCallback, "") { return (arg.is_null()); }

} // namespace

@@ -80,9 +73,12 @@

static const int kDummyChildId;

static const int kDummyRequestId;

+ // We need a UI |BrowserThread| in order to destruct |download_manager_|,

+ // which has trait |BrowserThread::DeleteOnUIThread|. Without this,

+ // calling Release() on |download_manager_| won't ever result in its

+ // destructor being called and we get a leak.

DownloadFileTest() :

- observer_(new StrictMock<MockDownloadDestinationObserver>),

- observer_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(observer_.get())),

+ update_download_id_(-1),

bytes_(-1),

bytes_per_sec_(-1),

hash_state_("xyzzy"),

@@ -93,35 +89,44 @@

~DownloadFileTest() {

}

- void SetUpdateDownloadInfo(int64 bytes, int64 bytes_per_sec,

+ void SetUpdateDownloadInfo(int32 id, int64 bytes, int64 bytes_per_sec,

const std::string& hash_state) {

+ update_download_id_ = id;

bytes_ = bytes;

bytes_per_sec_ = bytes_per_sec;

hash_state_ = hash_state;

}

void ConfirmUpdateDownloadInfo() {

- observer_->CurrentUpdateStatus(bytes_, bytes_per_sec_, hash_state_);

+ download_manager_->CurrentUpdateStatus(

+ update_download_id_, bytes_, bytes_per_sec_, hash_state_);

}

virtual void SetUp() {

- EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))

+ download_manager_ = new StrictMock<LocalMockDownloadManager>;

+ EXPECT_CALL(*(download_manager_.get()),

+ UpdateDownload(

+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),

+ _, _, _))

.Times(AnyNumber())

.WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));

}

+ virtual void TearDown() {

+ // When a DownloadManager's reference count drops to 0, it is not

+ // deleted immediately. Instead, a task is posted to the UI thread's

+ // message loop to delete it.

+ // So, drop the reference count to 0 and run the message loop once

+ // to ensure that all resources are cleaned up before the test exits.

+ download_manager_ = NULL;

+ ui_thread_.message_loop()->RunAllPending();

+ }

// Mock calls to this function are forwarded here.

void RegisterCallback(base::Closure sink_callback) {

sink_callback_ = sink_callback;

}

- void SetInterruptReasonCallback(bool* was_called,

- content::DownloadInterruptReason* reason_p,

- content::DownloadInterruptReason reason) {

- *was_called = true;

- *reason_p = reason;

- }

virtual bool CreateDownloadFile(int offset, bool calculate_hash) {

// There can be only one.

DCHECK(!download_file_.get());

@@ -134,36 +139,30 @@

.WillOnce(Invoke(this, &DownloadFileTest::RegisterCallback))

.RetiresOnSaturation();

+ DownloadCreateInfo info;

+ // info.request_handle default constructed to null.

+ info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);

+ info.save_info.file_stream = file_stream_;

download_file_.reset(

new DownloadFileImpl(

- content::DownloadSaveInfo(),

- GURL(), // Source

- GURL(), // Referrer

- 0, // Received bytes

- calculate_hash,

- scoped_ptr<content::ByteStreamReader>(input_stream_),

- net::BoundNetLog(),

+ &info,

+ scoped_ptr<content::ByteStreamReader>(input_stream_).Pass(),

+ new DownloadRequestHandle(),

+ download_manager_, calculate_hash,

scoped_ptr<content::PowerSaveBlocker>(NULL).Pass(),

- observer_factory_.GetWeakPtr()));

+ net::BoundNetLog()));

EXPECT_CALL(*input_stream_, Read(_, _))

.WillOnce(Return(content::ByteStreamReader::STREAM_EMPTY))

.RetiresOnSaturation();

- base::WeakPtrFactory<DownloadFileTest> weak_ptr_factory(this);

- bool called = false;

- content::DownloadInterruptReason result;

- download_file_->Initialize(base::Bind(

- &DownloadFileTest::SetInterruptReasonCallback,

- weak_ptr_factory.GetWeakPtr(), &called, &result));

- loop_.RunAllPending();

- EXPECT_TRUE(called);

+ content::DownloadInterruptReason result = download_file_->Initialize();

::testing::Mock::VerifyAndClearExpectations(input_stream_);

return result == content::DOWNLOAD_INTERRUPT_REASON_NONE;

}

virtual void DestroyDownloadFile(int offset) {

+ EXPECT_EQ(kDummyDownloadId + offset, download_file_->Id());

+ EXPECT_EQ(download_manager_, download_file_->GetDownloadManager());

EXPECT_FALSE(download_file_->InProgress());

EXPECT_EQ(static_cast<int64>(expected_data_.size()),

download_file_->BytesSoFar());

@@ -233,16 +232,19 @@

}

void FinishStream(content::DownloadInterruptReason interrupt_reason,

- bool check_observer) {

+ bool check_download_manager) {

::testing::Sequence s1;

SetupFinishStream(interrupt_reason, s1);

sink_callback_.Run();

VerifyStreamAndSize();

- if (check_observer) {

- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));

+ if (check_download_manager) {

+ EXPECT_CALL(*download_manager_, OnResponseCompleted(_, _, _));

loop_.RunAllPending();

- ::testing::Mock::VerifyAndClearExpectations(observer_.get());

- EXPECT_CALL(*(observer_.get()), DestinationUpdate(_, _, _))

+ ::testing::Mock::VerifyAndClearExpectations(download_manager_.get());

+ EXPECT_CALL(*(download_manager_.get()),

+ UpdateDownload(

+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),

+ _, _, _))

.Times(AnyNumber())

.WillRepeatedly(Invoke(this,

&DownloadFileTest::SetUpdateDownloadInfo));

@@ -270,9 +272,7 @@

}

protected:

- scoped_ptr<StrictMock<MockDownloadDestinationObserver> > observer_;

- base::WeakPtrFactory<content::DownloadDestinationObserver>

- observer_factory_;

+ scoped_refptr<StrictMock<LocalMockDownloadManager> > download_manager_;

linked_ptr<net::FileStream> file_stream_;

@@ -286,7 +286,8 @@

// Sink callback data for stream.

base::Closure sink_callback_;

- // Latest update sent to the observer.

+ // Latest update sent to the download manager.

+ int32 update_download_id_;

int64 bytes_;

int64 bytes_per_sec_;

std::string hash_state_;

@@ -487,12 +488,21 @@

// Test that calling the sink_callback_ on an empty stream shouldn't

// do anything.

AppendDataToFile(NULL, 0);

+ ::testing::Mock::VerifyAndClearExpectations(download_manager_.get());

+ EXPECT_CALL(*(download_manager_.get()),

+ UpdateDownload(

+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),

+ _, _, _))

+ .Times(AnyNumber())

+ .WillRepeatedly(Invoke(this, &DownloadFileTest::SetUpdateDownloadInfo));

// Finish the download this way and make sure we see it on the

- // observer.

- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));

+ // DownloadManager.

+ EXPECT_CALL(*(download_manager_.get()),

+ OnResponseCompleted(DownloadId(kValidIdDomain,

+ kDummyDownloadId + 0).local(),

+ 0, _));

FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, false);

- loop_.RunAllPending();

DestroyDownloadFile(0);

}

@@ -503,9 +513,10 @@

EXPECT_TRUE(file_util::PathExists(initial_path));

// Finish the download in error and make sure we see it on the

- // observer.

- EXPECT_CALL(*(observer_.get()),

- DestinationError(

+ // DownloadManager.

+ EXPECT_CALL(*(download_manager_.get()),

+ OnDownloadInterrupted(

+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),

content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))

.WillOnce(InvokeWithoutArgs(

this, &DownloadFileTest::ConfirmUpdateDownloadInfo));

@@ -515,7 +526,8 @@

// the last one may have the correct information even if the failure

// doesn't produce an update, as the timer update may have triggered at the

// same time.

- EXPECT_CALL(*(observer_.get()), CurrentUpdateStatus(0, _, _));

+ EXPECT_CALL(*(download_manager_.get()),

+ CurrentUpdateStatus(kDummyDownloadId + 0, 0, _, _));

FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false);

@@ -533,10 +545,13 @@

::testing::Sequence s1;

SetupDataAppend(chunks1, 2, s1);

SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, s1);

- EXPECT_CALL(*(observer_.get()), DestinationCompleted(_));

+ EXPECT_CALL(*(download_manager_.get()),

+ OnResponseCompleted(DownloadId(kValidIdDomain,

+ kDummyDownloadId + 0).local(),

+ strlen(kTestData1) + strlen(kTestData2),

+ _));

sink_callback_.Run();

VerifyStreamAndSize();

- loop_.RunAllPending();

DestroyDownloadFile(0);

}

@@ -551,8 +566,9 @@

SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED,

s1);

- EXPECT_CALL(*(observer_.get()),

- DestinationError(

+ EXPECT_CALL(*(download_manager_.get()),

+ OnDownloadInterrupted(

+ DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),

content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED))

.WillOnce(InvokeWithoutArgs(

this, &DownloadFileTest::ConfirmUpdateDownloadInfo));

@@ -562,8 +578,9 @@

// the last one may have the correct information even if the failure

// doesn't produce an update, as the timer update may have triggered at the

// same time.

- EXPECT_CALL(*(observer_.get()),

- CurrentUpdateStatus(strlen(kTestData1) + strlen(kTestData2),

+ EXPECT_CALL(*(download_manager_.get()),

+ CurrentUpdateStatus(kDummyDownloadId + 0,

+ strlen(kTestData1) + strlen(kTestData2),

_, _));

sink_callback_.Run();

@@ -573,7 +590,7 @@

}

// Send some data, wait 3/4s of a second, run the message loop, and

-// confirm the values the observer received are correct.

+// confirm the values the DownloadManager received are correct.

TEST_F(DownloadFileTest, ConfirmUpdate) {

CreateDownloadFile(0, true);

« no previous file with comments | « content/browser/download/download_file_manager_unittest.cc ('k') | content/browser/download/download_item_impl.h » ('j') | no next file with comments »