Move Rename functionality from DownloadFileManager to DownloadFileImple.

content/browser/download/download_file_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/file_util.h"
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/download/byte_stream.h"
 #include "content/browser/download/download_create_info.h"
 #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_interrupt_reasons.h"
 #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"
 
 using content::BrowserThread;
 using content::BrowserThreadImpl;
 using content::DownloadFile;
 using content::DownloadId;
 using content::DownloadManager;
 using ::testing::_;
@@ skipping 10 matching lines @@
   MockByteStreamReader() {}
   ~MockByteStreamReader() {}
 
   // ByteStream functions
   MOCK_METHOD2(Read, content::ByteStreamReader::StreamState(
       scoped_refptr<net::IOBuffer>*, size_t*));
   MOCK_CONST_METHOD0(GetStatus, content::DownloadInterruptReason());
   MOCK_METHOD1(RegisterCallback, void(const base::Closure&));
 };
 
+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;
+}
+
 }  // namespace
 
 DownloadId::Domain kValidIdDomain = "valid DownloadId::Domain";
 
 class DownloadFileTest : public testing::Test {
  public:
 
   static const char* kTestData1;
   static const char* kTestData2;
   static const char* kTestData3;
@@ skipping 52 matching lines @@
 
     input_stream_ = new StrictMock<MockByteStreamReader>();
 
     // TODO: Need to actually create a function that'll set the variables
     // based on the inputs from the callback.
     EXPECT_CALL(*input_stream_, RegisterCallback(_))
         .WillOnce(Invoke(this, &DownloadFileTest::RegisterCallback))
         .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(
             &info,
             scoped_ptr<content::ByteStreamReader>(input_stream_).Pass(),
             new DownloadRequestHandle(),
             download_manager_, calculate_hash,
             scoped_ptr<content::PowerSaveBlocker>(NULL).Pass(),
             net::BoundNetLog()));
 
@@ skipping 89 matching lines @@
       EXPECT_CALL(*(download_manager_.get()),
                   UpdateDownload(
                       DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
                       _, _, _))
           .Times(AnyNumber())
           .WillRepeatedly(Invoke(this,
                                  &DownloadFileTest::SetUpdateDownloadInfo));
     }
   }
 
+  content::DownloadInterruptReason Rename(
+      const FilePath& full_path, bool overwrite_existing_file,
+      FilePath* result_path_p) {
+    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(&SetRenameResult,
+                                      &callback_was_called,
+                                      &result_reason, result_path_p));
+    loop_.RunAllPending();
+
+    // If the callback wasn't called we've got a memory stomper waiting
+    // to happen; best to crash.
+    CHECK(callback_was_called);

[asanka, 2012/07/05 18:47:21]

The last time came up, I ended up creating a local WeakPtrFactory<TestClassName>
which vended a weak reference to |this|. The weak reference was used to bind to
TestClassName::HandleCallback() that handled the callback. This way when the
WeakPtrFactory<> went out of scope, it would invalidate the callback and prevent
it from stomping memory.

[Randy Smith (Not in Mondays), 2012/07/09 20:35:51]

Nice.  Done.

+    return result_reason;
+  }
+
  protected:
   scoped_refptr<StrictMock<content::MockDownloadManager> > download_manager_;
 
   linked_ptr<net::FileStream> file_stream_;
 
   // DownloadFile instance we are testing.
   scoped_ptr<DownloadFile> download_file_;
 
   // Stream for sending data into the download file.
   // Owned by download_file_; will be alive for lifetime of download_file_.
@@ skipping 33 matching lines @@
 // Rename the file before any data is downloaded, after some has, after it all
 // has, and after it's closed.
 TEST_F(DownloadFileTest, RenameFileFinal) {
   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_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,
-            download_file_->Rename(path_1));
+            Rename(path_1, false, &output_path));
   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));
   EXPECT_TRUE(file_util::PathExists(path_1));
 
   // Download the data.
   const char* chunks1[] = { kTestData1, kTestData2 };
   AppendDataToFile(chunks1, 2);
 
   // Rename the file after downloading some data.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_2));
+            Rename(path_2, false, &output_path));
   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));
   EXPECT_TRUE(file_util::PathExists(path_2));
 
   const char* chunks2[] = { kTestData3 };
   AppendDataToFile(chunks2, 1);
 
   // Rename the file after downloading all the data.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_3));
+            Rename(path_3, false, &output_path));
   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));
   EXPECT_TRUE(file_util::PathExists(path_3));
 
   // Should not be able to get the hash until the file is closed.
   std::string hash;
   EXPECT_FALSE(download_file_->GetHash(&hash));
   FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true);
   loop_.RunAllPending();
 
   // Rename the file after downloading all the data and closing the file.
   EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,
-            download_file_->Rename(path_4));
+            Rename(path_4, false, &output_path));
   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));
   EXPECT_TRUE(file_util::PathExists(path_4));
 
   // Check the hash.
   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);
 }
 

[asanka, 2012/07/05 18:47:21]

Also throw in a RenameError test now that error reporting on renames can't be
tested in DownloadFileManagerTest?

A simple one where should_overwrite = true and a conflicting file exists and is
unwriteable would be sufficient. base/test/test_file_util.h has
MakeFileUnwriteable() and PermissionRestorer which will help create a temporary
unwriteable file.

[Randy Smith (Not in Mondays), 2012/07/09 20:35:51]

Good thought!  I didn't know about MakeFileUnwritable (or had forgotten).  Done.

+// Test to make sure the rename uniquifies if we aren't overwriting
+// and there's a file where we're aiming.
+TEST_F(DownloadFileTest, RenameOverwrite) {
+  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);
+}
+
+// TODO(rdsmith): Test proper behavior of DownloadFile in the case of
+// errors on the rename (including writing to the file after rename error).
+
 // Various tests of the StreamActive method.
 TEST_F(DownloadFileTest, StreamEmptySuccess) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   FilePath initial_path(download_file_->FullPath());
   EXPECT_TRUE(file_util::PathExists(initial_path));
 
   // Test that calling the sink_callback_ on an empty stream shouldn't
   // do anything.
   AppendDataToFile(NULL, 0);
   ::testing::Mock::VerifyAndClearExpectations(download_manager_.get());
@@ skipping 15 matching lines @@
   DestroyDownloadFile(0);
 }
 
 TEST_F(DownloadFileTest, StreamEmptyError) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   FilePath initial_path(download_file_->FullPath());
   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()),

[asanka, 2012/07/05 18:47:21]

Expect a sequence restricted UpdateDownload() call before the
OnDownloadInterrupted() call. I think we are now sensitive to getting
UpdateDownload() before OnDownloadInterrupted().

Also applies to other tests below. 

[Randy Smith (Not in Mondays), 2012/07/09 20:35:51]

So the tricky thing here is that we're going to have an arbitrary number of
UpdateDownload() calls coming in generated by the timer.  So I think the
invariant that we want is that the last UpdateDownload call to come in before
the OnDownloadInterrupted had the information we wanted.  This isn't idea, as if
the UpdateDownload call wasn't sparked by the interrupt, the test may still
sometimes pass.  But I don't think that's likely, and I'll put a comment next to
the flaky failure location indicating what it means.  Let me know what you
think.

               OnDownloadInterrupted(
                   DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
-                  0, _,
                   content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED));
   FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED, false);
 
   DestroyDownloadFile(0);
 }
 
 TEST_F(DownloadFileTest, StreamNonEmptySuccess) {
   ASSERT_TRUE(CreateDownloadFile(0, true));
   FilePath initial_path(download_file_->FullPath());
   EXPECT_TRUE(file_util::PathExists(initial_path));
@@ skipping 18 matching lines @@
   EXPECT_TRUE(file_util::PathExists(initial_path));
 
   const char* chunks1[] = { kTestData1, kTestData2 };
   ::testing::Sequence s1;
   SetupDataAppend(chunks1, 2, s1);
   SetupFinishStream(content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED,
                     s1);
   EXPECT_CALL(*(download_manager_.get()),
               OnDownloadInterrupted(
                   DownloadId(kValidIdDomain, kDummyDownloadId + 0).local(),
-                  strlen(kTestData1) + strlen(kTestData2), _,
                   content::DOWNLOAD_INTERRUPT_REASON_NETWORK_DISCONNECTED));
   sink_callback_.Run();
+  loop_.RunAllPending();
+  EXPECT_EQ(static_cast<int64>(strlen(kTestData1) + strlen(kTestData2)),
+            bytes_);
   VerifyStreamAndSize();
   DestroyDownloadFile(0);
 }
 
 // Send some data, wait 3/4s of a second, run the message loop, and
 // confirm the values the DownloadManager received are correct.
 TEST_F(DownloadFileTest, ConfirmUpdate) {
   CreateDownloadFile(0, true);
 
   const char* chunks1[] = { kTestData1, kTestData2 };
   AppendDataToFile(chunks1, 2);
 
   // Run the message loops for 750ms and check for results.
   loop_.PostDelayedTask(FROM_HERE, MessageLoop::QuitClosure(),
                          base::TimeDelta::FromMilliseconds(750));
   loop_.Run();
 
   EXPECT_EQ(static_cast<int64>(strlen(kTestData1) + strlen(kTestData2)),
             bytes_);
   EXPECT_EQ(download_file_->GetHashState(), hash_state_);
 
   FinishStream(content::DOWNLOAD_INTERRUPT_REASON_NONE, true);
   DestroyDownloadFile(0);
 }