Added read-only file error test.

chrome/browser/download/download_browsertest.cc

Index: chrome/browser/download/download_browsertest.cc
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 98ba3a57129a220da104232a1b65dc58e73d9e5f..e7f102806fc1a87419eb6b0caeacc9f0037609de 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -690,6 +690,8 @@ class DownloadTest : public InProcessBrowserTest {
     GURL url = test_server()->GetURL(server_path);
     ASSERT_TRUE(url.is_valid());
 
+    NullSelectFile(browser());  // Needed for read-only tests.

[Randy Smith (Not in Mondays), 2012/02/24 19:16:15]

This makes me uncomfortable, as it changes the behavior of a helper routine
(DownloadFileCheckErrors) in a pretty important way.  I can't actually imagine
how it could be a problem for these tests, but it's worthwhile calling out in
the description of what this routine does that it'll inhibit the select file
dialog.

[ahendrickson, 2012/02/25 01:05:17]

This makes the behavior what I wanted it to be in the first place -- if a select
file dialog appears, choose the default.  Combined with changing the "Bail on
select file" parameter below to false,  the download goes to
completion/interruption.

I'll add the comment.

[Randy Smith (Not in Mondays), 2012/02/27 21:06:03]

I don't see the comment; did you mean you'd add it later?  (Fine if so; I'm just
used to responses coming back with the code changes they refer to.)

[ahendrickson, 2012/02/28 03:59:25]

Yes, it got added after a merge with the parent.
It's in the function description.

[Randy Smith (Not in Mondays), 2012/03/01 20:37:14]

Got it--thanks.

+
     DownloadManager* download_manager = DownloadManagerForBrowser(browser());
     scoped_ptr<DownloadTestObserver> observer(
         new DownloadTestObserver(
@@ -698,7 +700,7 @@ class DownloadTest : public InProcessBrowserTest {
             download_info.reason == DOWNLOAD_INTERRUPT_REASON_NONE ?
                 DownloadItem::COMPLETE :  // Really done
                 DownloadItem::INTERRUPTED,
-            true,                   // Bail on select file
+            false,                        // Don't finish on select file
             DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
 
     if (download_info.download_method == DOWNLOAD_DIRECT) {
@@ -731,9 +733,49 @@ class DownloadTest : public InProcessBrowserTest {
       ASSERT_EQ(url, item->GetOriginalUrl());
 
       ASSERT_EQ(download_info.reason, item->GetLastReason());
+
+      if (item->GetLastReason() == DOWNLOAD_INTERRUPT_REASON_NONE) {

[Randy Smith (Not in Mondays), 2012/02/27 21:06:03]

What does this test mean?  That we weren't interrupted?  Isn't that a bit more
clear if you just check item status?

[ahendrickson, 2012/02/28 03:59:25]

It means that the download succeeded in the end.  Specifically for these tests,
they are put in the "My Documents" folder instead of the destination folder.

[Randy Smith (Not in Mondays), 2012/03/01 20:37:14]

Why not test against state COMPLETED?  It seems clearer.

[ahendrickson, 2012/03/02 17:51:57]

Done.

+        // Clean up the file, in case it ended up in the My Documents folder.
+        FilePath destination_folder = GetDownloadDirectory(browser());
+        FilePath my_downloaded_file = item->GetTargetFilePath();
+        EXPECT_TRUE(file_util::PathExists(my_downloaded_file));
+        EXPECT_TRUE(file_util::Delete(my_downloaded_file, false));
+
+        // If it's not where we asked it to be, it should be in the
+        // My Documents folder.
+        if (my_downloaded_file.value().find(destination_folder.value()) ==
+            std::string::npos) {
+          FilePath my_docs_folder;
+          EXPECT_TRUE(PathService::Get(chrome::DIR_USER_DOCUMENTS,
+                                       &my_docs_folder));
+          EXPECT_EQ(0u,
+                    my_downloaded_file.value().find(my_docs_folder.value()));
+        }
+      }
     }
   }
 
+  // Attempts to download a file to a read-only folder, based on information
+  // in |download_info|.
+  void DownloadFileToReadonlyFolder(const DownloadInfo& download_info) {
+    ASSERT_TRUE(InitialSetup(false));  // Creates temporary download folder.
+
+    // Make the test folder unwritable.
+    FilePath destination_folder = GetDownloadDirectory(browser());
+    DVLOG(1) << " " << __FUNCTION__ << "()"
+             << " folder = '" << destination_folder.value() << "'";
+    size_t perm_length = 0;
+    void* original_permissions =
+        file_util::GetPermissionInfo(destination_folder, &perm_length);
+    EXPECT_TRUE(original_permissions != NULL);
+    EXPECT_TRUE(file_util::MakeFileUnwritable(destination_folder));
+
+    DownloadFileCheckErrors(download_info);
+
+    EXPECT_TRUE(file_util::RestorePermissionInfo(destination_folder,
+                                                 original_permissions,
+                                                 perm_length));
+  }
  private:
   // Location of the test data.
   FilePath test_dir_;
@@ -2018,3 +2060,27 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadError400Navigate) {
   ASSERT_TRUE(InitialSetup(false));
   DownloadFileCheckErrors(download_info);
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderDirect) {

[Randy Smith (Not in Mondays), 2012/02/24 19:16:15]

Shouldn't both of these tests actively confirm that the file went to the My
Documents folder?  It doesn't look like they do.

[ahendrickson, 2012/02/25 01:05:17]

They do, in line 751.

[Randy Smith (Not in Mondays), 2012/02/27 21:06:03]

That test (presuming I'm reading it correctly) is "If it isn't in the Download
folder, make sure it's in the My Documents folder".  What I'm asking for is "If
we're downloading to a read only folder, make sure the download went to the My
Documents folder."  In theory, we could fallover to the MyDocuments folder for a
non-read only download, and wouldn't notice (or the reverse, but actually
writing into the read only directory seems a little extreme to have to worry
about, though it wouldn't hurt the test.).  

[ahendrickson, 2012/02/28 03:59:25]

Ah, I see.  I don't have access to the information in this function, but I added
another parameter to DownloadInfo to test in DownloadFileCheckErrors().

+  DownloadInfo download_info = {
+    "a_zip_file.zip",
+    DOWNLOAD_DIRECT,
+    // This passes because we switch to the My Documents folder.
+    DOWNLOAD_INTERRUPT_REASON_NONE,
+    true
+  };
+
+  DownloadFileToReadonlyFolder(download_info);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolderNavigate) {
+  DownloadInfo download_info = {
+    "a_zip_file.zip",
+    DOWNLOAD_NAVIGATE,
+    // This passes because we switch to the My Documents folder.
+    DOWNLOAD_INTERRUPT_REASON_NONE,
+    true
+  };
+
+  DownloadFileToReadonlyFolder(download_info);
+}