Move Rename functionality from DownloadFileManager to DownloadFileImple.

content/browser/download/download_file_unittest.cc

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 01009a89b9fe97d932f6dbf42b57f6cf9d1427ee..3d9fb24124b129bc6641e2887dee94e6fec540c1 100644
--- a/content/browser/download/download_file_unittest.cc
+++ b/content/browser/download/download_file_unittest.cc
@@ -15,6 +15,7 @@
 #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"
@@ -45,6 +46,19 @@ class MockByteStreamReader : public content::ByteStreamReader {
   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";
@@ -117,8 +131,8 @@ class DownloadFileTest : public testing::Test {
         .RetiresOnSaturation();
 
     DownloadCreateInfo info;
-    info.download_id = DownloadId(kValidIdDomain, kDummyDownloadId + offset);
     // 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(
@@ -228,6 +242,26 @@ class DownloadFileTest : public testing::Test {
     }
   }
 
+  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_;
 
@@ -281,12 +315,15 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   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));
@@ -298,9 +335,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // 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));
@@ -311,9 +349,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // 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));
@@ -327,9 +366,10 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
 
   // 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));
@@ -339,9 +379,54 @@ TEST_F(DownloadFileTest, RenameFileFinal) {
   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));
@@ -380,7 +465,6 @@ TEST_F(DownloadFileTest, StreamEmptyError) {
   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);
 
@@ -419,9 +503,11 @@ TEST_F(DownloadFileTest, StreamNonEmptyError) {
   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);
 }