Created an interface for DownloadFile, for use in unit tests.

chrome/browser/download/download_manager_unittest.cc

Index: chrome/browser/download/download_manager_unittest.cc
diff --git a/chrome/browser/download/download_manager_unittest.cc b/chrome/browser/download/download_manager_unittest.cc
index a85fb3efebcc5a9b6295b7ca1953985fb39b3d10..e0199e5aee7a1b36403247b1e62ea066d6ab7ffc 100644
--- a/chrome/browser/download/download_manager_unittest.cc
+++ b/chrome/browser/download/download_manager_unittest.cc
@@ -24,7 +24,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/browser/download/download_buffer.h"
 #include "content/browser/download/download_create_info.h"
-#include "content/browser/download/download_file.h"
+#include "content/browser/download/download_file_impl.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_id_factory.h"
 #include "content/browser/download/download_item.h"
@@ -36,7 +36,6 @@
 #include "content/test/test_browser_thread.h"
 #include "grit/generated_resources.h"
 #include "net/base/io_buffer.h"
-#include "net/base/mock_file_stream.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -57,7 +56,7 @@ class DownloadManagerTest : public testing::Test {
         download_manager_delegate_(new ChromeDownloadManagerDelegate(
             profile_.get())),
         id_factory_(new DownloadIdFactory(kValidIdDomain)),
-        download_manager_(new MockDownloadManager(

[Randy Smith (Not in Mondays), 2011/11/10 21:58:43]

Why this change here?  I thought that was the other CL?

[ahendrickson, 2011/11/13 00:50:21]

Ah, yes, this is a result of splitting a CL in two, not that it changes
functionality in any way.

Reverted.

+        download_manager_(new DownloadManager(
             download_manager_delegate_,
             id_factory_,
             &download_status_updater_)),
@@ -163,43 +162,52 @@ const char* DownloadManagerTest::kTestData = "a;sdlfalsdfjalsdkfjad";
 const size_t DownloadManagerTest::kTestDataLen =
     strlen(DownloadManagerTest::kTestData);
 
-// A DownloadFile that we can inject errors into.  Uses MockFileStream.
-// Note:  This can't be in an anonymous namespace because it must be declared
-// as a friend of |DownloadFile| in order to access its private members.
-class DownloadFileWithMockStream : public DownloadFile {
+// A DownloadFile that we can inject errors into.
+class DownloadFileWithErrors : public DownloadFileImpl {

[Randy Smith (Not in Mondays), 2011/11/10 21:58:43]

Just noting that in the context of non-test code I'd want to bias away from
implementation inheritance, but in test code I'm cool with it.  (In the abstract
it's a short-cut that can come back to bite you because it splits code that
works together into separate locations, but I think it's a fine shortcut for
testing.)

  public:
-  DownloadFileWithMockStream(DownloadCreateInfo* info,
-                             DownloadManager* manager,
-                             net::testing::MockFileStream* stream);
+  DownloadFileWithErrors(DownloadCreateInfo* info, DownloadManager* manager);
+  virtual ~DownloadFileWithErrors() {}
 
-  virtual ~DownloadFileWithMockStream() {}
+  // BaseFile delegated functions.
+  virtual net::Error Initialize(bool calculate_hash);
+  virtual net::Error AppendDataToFile(const char* data, size_t data_len);
+  virtual net::Error Rename(const FilePath& full_path);
 
-  void SetForcedError(int error);
+  void set_forced_error(net::Error error) { forced_error_ = error; }
+  void clear_forced_error() { forced_error_ = net::OK; }
+  net::Error forced_error() const { return forced_error_; }
 
- protected:
-  // This version creates a |MockFileStream| instead of a |FileStream|.
-  virtual void CreateFileStream() OVERRIDE;
+ private:
+  net::Error ReturnError(net::Error function_error) {
+    if (forced_error_ != net::OK) {
+      net::Error ret = forced_error_;
+      clear_forced_error();
+      return ret;
+    }
+
+    return function_error;
+  }
+
+  net::Error forced_error_;
 };
 
-DownloadFileWithMockStream::DownloadFileWithMockStream(
-    DownloadCreateInfo* info,
-    DownloadManager* manager,
-    net::testing::MockFileStream* stream)
-    : DownloadFile(info, new DownloadRequestHandle(), manager) {
-  DCHECK(file_stream_ == NULL);
-  file_stream_.reset(stream);
+DownloadFileWithErrors::DownloadFileWithErrors(DownloadCreateInfo* info,
+                                               DownloadManager* manager)
+    : DownloadFileImpl(info, new DownloadRequestHandle(), manager),
+      forced_error_(net::OK) {
+}
+
+net::Error DownloadFileWithErrors::Initialize(bool calculate_hash) {
+  return ReturnError(DownloadFileImpl::Initialize(calculate_hash));
 }
 
-void DownloadFileWithMockStream::SetForcedError(int error)
-{
-  // |file_stream_| can only be set in the constructor and in
-  // CreateFileStream(), both of which insure that it is a |MockFileStream|.
-  net::testing::MockFileStream* mock_stream =
-    static_cast<net::testing::MockFileStream *>(file_stream_.get());
-  mock_stream->set_forced_error(error);
+net::Error DownloadFileWithErrors::AppendDataToFile(const char* data,
+                                                    size_t data_len) {
+  return ReturnError(DownloadFileImpl::AppendDataToFile(data, data_len));
 }
-void DownloadFileWithMockStream::CreateFileStream() {
-  file_stream_.reset(new net::testing::MockFileStream);
+
+net::Error DownloadFileWithErrors::Rename(const FilePath& full_path) {
+  return ReturnError(DownloadFileImpl::Rename(full_path));
 }
 
 namespace {
@@ -280,12 +288,12 @@ const struct {
   { FILE_PATH_LITERAL("Unconfirmed xxx.crdownload"), true, true, false, 1, },
 };
 
-class MockDownloadFile : public DownloadFile {
+class GMockDownloadFile : public DownloadFileImpl {
  public:
-  MockDownloadFile(DownloadCreateInfo* info, DownloadManager* manager)
-      : DownloadFile(info, new DownloadRequestHandle(), manager),
+  GMockDownloadFile(DownloadCreateInfo* info, DownloadManager* manager)
+      : DownloadFileImpl(info, new DownloadRequestHandle(), manager),
         renamed_count_(0) { }
-  virtual ~MockDownloadFile() { Destructed(); }
+  virtual ~GMockDownloadFile() { Destructed(); }
   MOCK_METHOD1(Rename, net::Error(const FilePath&));
   MOCK_METHOD0(Destructed, void());
 
@@ -399,8 +407,8 @@ TEST_F(DownloadManagerTest, StartDownload) {
     download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
 
     DownloadFile* download_file(
-        new DownloadFile(info.get(), new DownloadRequestHandle(),
-                         download_manager_));
+        new DownloadFileImpl(info.get(), new DownloadRequestHandle(),
+                             download_manager_));
     AddDownloadToFileManager(info->download_id.local(), download_file);
     download_file->Initialize(false);
     download_manager_->StartDownload(info->download_id.local());
@@ -430,8 +438,8 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
     info->url_chain.push_back(GURL());
     const FilePath new_path(kDownloadRenameCases[i].suggested_path);
 
-    MockDownloadFile* download_file(
-        new MockDownloadFile(info.get(), download_manager_));
+    GMockDownloadFile* download_file(
+        new GMockDownloadFile(info.get(), download_manager_));
     AddDownloadToFileManager(info->download_id.local(), download_file);
 
     // |download_file| is owned by DownloadFileManager.
@@ -445,10 +453,10 @@ TEST_F(DownloadManagerTest, DownloadRenameTest) {
       FilePath crdownload(download_util::GetCrDownloadPath(new_path));
       EXPECT_CALL(*download_file, Rename(_))
           .WillOnce(testing::WithArgs<0>(Invoke(CreateFunctor(
-              download_file, &MockDownloadFile::TestMultipleRename,
+              download_file, &GMockDownloadFile::TestMultipleRename,
               1, crdownload))))
           .WillOnce(testing::WithArgs<0>(Invoke(CreateFunctor(
-              download_file, &MockDownloadFile::TestMultipleRename,
+              download_file, &GMockDownloadFile::TestMultipleRename,
               2, new_path))));
     }
     download_manager_->CreateDownloadItem(info.get(), DownloadRequestHandle());
@@ -495,8 +503,8 @@ TEST_F(DownloadManagerTest, DownloadInterruptTest) {
   const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
   const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
 
-  MockDownloadFile* download_file(
-      new MockDownloadFile(info.get(), download_manager_));
+  GMockDownloadFile* download_file(
+      new GMockDownloadFile(info.get(), download_manager_));
   AddDownloadToFileManager(info->download_id.local(), download_file);
 
   // |download_file| is owned by DownloadFileManager.
@@ -569,8 +577,8 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
   ASSERT_TRUE(file_util::CreateTemporaryFile(&path));
 
   // This file stream will be used, until the first rename occurs.
-  net::testing::MockFileStream* mock_stream = new net::testing::MockFileStream;
-  ASSERT_EQ(0, mock_stream->Open(
+  net::FileStream* stream = new net::FileStream;
+  ASSERT_EQ(0, stream->Open(
       path,
       base::PLATFORM_FILE_OPEN_ALWAYS | base::PLATFORM_FILE_WRITE));
 
@@ -584,10 +592,11 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
   info->url_chain.push_back(GURL());
   info->total_bytes = static_cast<int64>(kTestDataLen * 3);
   info->save_info.file_path = path;
+  info->save_info.file_stream.reset(stream);
 
   // Create a download file that we can insert errors into.
-  DownloadFileWithMockStream* download_file(new DownloadFileWithMockStream(
-      info.get(), download_manager_, mock_stream));
+  DownloadFileWithErrors* download_file(new DownloadFileWithErrors(
+      info.get(), download_manager_));
   AddDownloadToFileManager(local_id, download_file);
 
   // |download_file| is owned by DownloadFileManager.
@@ -614,7 +623,7 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
   UpdateData(local_id, kTestData, kTestDataLen);
 
   // Add more data, but an error occurs.
-  download_file->SetForcedError(net::ERR_FAILED);
+  download_file->set_forced_error(net::ERR_FAILED);
   UpdateData(local_id, kTestData, kTestDataLen);
 
   // Check the state.  The download should have been interrupted.
@@ -630,7 +639,7 @@ TEST_F(DownloadManagerTest, DownloadFileErrorTest) {
   EXPECT_EQ(DownloadItem::INTERRUPTED, download->state());
 
   // Check the download shelf's information.
-  size_t error_size = kTestDataLen * 2;
+  size_t error_size = kTestDataLen * 3;
   size_t total_size = kTestDataLen * 3;
   ui::DataUnits amount_units = ui::GetByteDisplayUnits(kTestDataLen);
   string16 simple_size =
@@ -663,8 +672,8 @@ TEST_F(DownloadManagerTest, DownloadCancelTest) {
   const FilePath new_path(FILE_PATH_LITERAL("foo.zip"));
   const FilePath cr_path(download_util::GetCrDownloadPath(new_path));
 
-  MockDownloadFile* download_file(
-      new MockDownloadFile(info.get(), download_manager_));
+  GMockDownloadFile* download_file(
+      new GMockDownloadFile(info.get(), download_manager_));
   AddDownloadToFileManager(info->download_id.local(), download_file);
 
   // |download_file| is owned by DownloadFileManager.
@@ -760,8 +769,8 @@ TEST_F(DownloadManagerTest, DownloadOverwriteTest) {
   // name has been chosen, so we need to initialize the download file
   // properly.
   DownloadFile* download_file(
-      new DownloadFile(info.get(), new DownloadRequestHandle(),
-                       download_manager_));
+      new DownloadFileImpl(info.get(), new DownloadRequestHandle(),
+                           download_manager_));
   download_file->Rename(cr_path);
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);
@@ -837,8 +846,8 @@ TEST_F(DownloadManagerTest, DownloadRemoveTest) {
   // name has been chosen, so we need to initialize the download file
   // properly.
   DownloadFile* download_file(
-      new DownloadFile(info.get(), new DownloadRequestHandle(),
-                       download_manager_));
+      new DownloadFileImpl(info.get(), new DownloadRequestHandle(),
+                           download_manager_));
   download_file->Rename(cr_path);
   // This creates the .crdownload version of the file.
   download_file->Initialize(false);