Added read-only file error test.

chrome/browser/download/download_browsertest.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/bind.h"
 #include "base/bind_helpers.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
@@ skipping 672 matching lines @@
     ASSERT_TRUE(test_server()->Start());
     std::vector<DownloadItem*> download_items;
     GetDownloads(browser(), &download_items);
     ASSERT_TRUE(download_items.empty());
 
     std::string server_path = "files/downloads/";
     server_path += download_info.url_name;
     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(
             download_manager,
             1,
             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) {
       // Go directly to download.
       WebContents* web_contents = browser()->GetSelectedWebContents();
       ASSERT_TRUE(web_contents);
       DownloadSaveInfo save_info;
       save_info.prompt_for_save_location = false;
 
       DownloadManagerForBrowser(browser())->DownloadUrl(
           url, GURL(""), "", false, -1, save_info, web_contents);
     } else {
       // Navigate to URL normally, wait until done.
       ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(browser(),
                                                                 url,
                                                                 1);
     }
 
     if (download_info.show_download_item)
       observer->WaitForFinished();

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

If show_download_item isn't set, doesn't this mean we don't wait?  And won't
that produce races/flakiness if we get to the tests below before the download
has really started?

Unless I'm missing something, good race hygiene requires you find something to
wait on so that whatever has happened is going to happen (ideally not something
that presupposes that the result you're testing for actually has happened.)

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

Any suggestions on what to wait for in this case?

[ahendrickson, 2012/03/01 15:18:18]

I've addressed this in CL 9426029.

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

And just confirming: That CL is going in first?  I'll want to re-review this CL
after that one goes in, to see how the fix relates in place.

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

You are correct.  That CL has changed to 9570005 however.

 
     // Validate that the correct file was downloaded.
     download_items.clear();
     GetDownloads(browser(), &download_items);

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

This won't see downloads before they're put into the history; will that be a
problem for any of the !show_download_item cases?

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

The !show_download_item won't have anything inserted into the history, and won't
have anything displayed on the shelf.

     size_t item_count = download_info.show_download_item ? 1 : 0;
     ASSERT_EQ(item_count, download_items.size());
 
     if (download_info.show_download_item) {
       DownloadItem* item = download_items[0];
       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_;
 
   // Location of the downloads directory for these tests
   ScopedTempDir downloads_directory_;
 };
 
 // NOTES:
 //
@@ skipping 1264 matching lines @@
     "zip_file_not_found.zip",
     DOWNLOAD_NAVIGATE,
     DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,
     false
   };
 
   // Do initial setup.
   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);
+}