Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(395)

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

Issue 10799005: Replace the DownloadFileManager with direct ownership (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Incorporated comments. Created 8 years, 5 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
diff --git a/content/browser/download/download_file_unittest.cc b/content/browser/download/download_file_unittest.cc
index 24eaade547ec246906dac97f05ae49c3152d346a..78da4871a193f3683bdb024547651937afde382f 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
benjhayden 2012/08/01 18:00:18 Much better! Could you expose bytes_ from the fixt
Randy Smith (Not in Mondays) 2012/08/03 17:32:44 I want to make sure that the last progress update
+ // 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);

Powered by Google App Engine
This is Rietveld 408576698