Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(494)

Unified Diff: chrome/browser/download/download_browsertest.cc

Issue 6973052: When the download folder does not exist, change the download folder to a user's "Downloads" (Closed) Base URL: http://git.chromium.org/git/chromium.git@trunk
Patch Set: Applied Pawel's comments Created 9 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | chrome/browser/download/download_file_manager.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/download/download_browsertest.cc
diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc
index 90acfa136984f1bb11abdd07428f06cb2f86b093..8440d9370a7f1314322a4a6b1e814daf2c6b6fd1 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -8,6 +8,7 @@
#include "base/path_service.h"
#include "base/scoped_temp_dir.h"
#include "base/stl_util.h"
+#include "base/string_util.h"
#include "base/test/test_file_util.h"
#include "base/utf_string_conversions.h"
#include "chrome/browser/browser_process.h"
@@ -32,6 +33,7 @@
#include "chrome/common/chrome_notification_types.h"
#include "chrome/common/chrome_paths.h"
#include "chrome/common/pref_names.h"
+#include "chrome/common/random.h"
#include "chrome/common/url_constants.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/ui_test_utils.h"
@@ -42,6 +44,7 @@
#include "content/browser/net/url_request_slow_download_job.h"
#include "content/common/page_transition_types.h"
#include "net/base/net_util.h"
+#include "net/url_request/url_request_filter.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace {
@@ -53,6 +56,9 @@ const FilePath kGoodCrxPath(FILE_PATH_LITERAL("extensions/good.crx"));
const char kLargeThemeCrxId[] = "pjpgmfcmabopnnfonnhmdjglfpjjfkbf";
const FilePath kLargeThemePath(FILE_PATH_LITERAL("extensions/theme2.crx"));
+// The test file used in SavePageBrowserTest.SaveFolder3.
+static const FilePath kTestFile(FILE_PATH_LITERAL("download-test1.lib"));
+
// Action a test should take if a dangerous download is encountered.
enum DangerousDownloadAction {
ON_DANGEROUS_DOWNLOAD_ACCEPT, // Accept the download
@@ -128,6 +134,7 @@ class DownloadsObserver : public DownloadManager::Observer,
// State accessors.
bool select_file_dialog_seen() { return select_file_dialog_seen_; }
+ const FilePath& suggested_path() { return suggested_path_; }
// Wait for whatever state was specified in the constructor.
void WaitForFinished() {
@@ -240,8 +247,10 @@ class DownloadsObserver : public DownloadManager::Observer,
}
}
- virtual void SelectFileDialogDisplayed(int32 /* id */) {
+ virtual void SelectFileDialogDisplayed(
+ int32 /* id */, const FilePath& suggested_path) {
select_file_dialog_seen_ = true;
+ suggested_path_ = suggested_path;
SignalIfFinished();
}
@@ -317,6 +326,9 @@ class DownloadsObserver : public DownloadManager::Observer,
// Holds the download ids which were dangerous.
std::set<int32> dangerous_downloads_seen_;
+ // The suggested file path in the select file dialog.
+ FilePath suggested_path_;
+
DISALLOW_COPY_AND_ASSIGN(DownloadsObserver);
};
@@ -492,12 +504,6 @@ class CancelTestDataCollector
class DownloadTest : public InProcessBrowserTest {
public:
- enum SelectExpectation {
- EXPECT_NO_SELECT_DIALOG = -1,
- EXPECT_NOTHING,
- EXPECT_SELECT_DIALOG
- };
-
DownloadTest() {
EnableDOMAutomation();
}
@@ -545,7 +551,7 @@ class DownloadTest : public InProcessBrowserTest {
// Location of the file destination (place to which it is downloaded).
FilePath DestinationFile(Browser* browser, FilePath file) {
- return GetDownloadDirectory(browser).Append(file);
+ return GetDownloadSaveDirectory(browser).Append(file);
}
// Must be called after browser creation. Creates a temporary
@@ -566,11 +572,16 @@ class DownloadTest : public InProcessBrowserTest {
return true;
}
+ // Delete the default folder for downloaded files.
+ bool DeleteDownloadsDirectory() {
+ return downloads_directory_.Delete();
+ }
+
DownloadPrefs* GetDownloadPrefs(Browser* browser) {
return browser->profile()->GetDownloadManager()->download_prefs();
}
- FilePath GetDownloadDirectory(Browser* browser) {
+ FilePath GetDownloadSaveDirectory(Browser* browser) {
DownloadManager* download_mananger =
browser->profile()->GetDownloadManager();
return download_mananger->download_prefs()->download_path();
@@ -578,13 +589,17 @@ class DownloadTest : public InProcessBrowserTest {
// Create a DownloadsObserver that will wait for the
// specified number of downloads to finish.
- DownloadsObserver* CreateWaiter(Browser* browser, int num_downloads) {
+ // If |finish_on_select_file| is true, the object will also be
+ // considered finished when the select file dialog is displayed.
+ DownloadsObserver* CreateWaiter(Browser* browser,
+ int num_downloads,
+ bool finish_on_select_file) {
DownloadManager* download_manager =
browser->profile()->GetDownloadManager();
return new DownloadsObserver(
download_manager, num_downloads,
DownloadItem::COMPLETE, // Really done
- false, // Bail on select file
+ finish_on_select_file, // Bail on select file
ON_DANGEROUS_DOWNLOAD_FAIL);
}
@@ -621,19 +636,25 @@ class DownloadTest : public InProcessBrowserTest {
// Download |url|, then wait for the download to finish.
// |disposition| indicates where the navigation occurs (current tab, new
// foreground tab, etc).
- // |expectation| indicates whether or not a Select File dialog should be
+ // |expect_file_dialog| indicates whether a select file dialog should be
// open when the download is finished, or if we don't care.
- // If the dialog appears, the routine exits. The only effect |expectation|
- // has is whether or not the test succeeds.
+ // If the dialog appears, the routine exits. The only effect
+ // |expect_file_dialog| has is whether or not the test succeeds.
+ // |expected_suggested_path| is the path expected to be suggested in the
+ // select file dialog. This |expected_suggested_path| must be specified
+ // if |expect_file_dialog| is true. If |expect_file_dialog| is false,
+ // |expected_suggested_path| is ignored.
// |browser_test_flags| indicate what to wait for, and is an OR of 0 or more
// values in the ui_test_utils::BrowserTestWaitFlags enum.
void DownloadAndWaitWithDisposition(Browser* browser,
const GURL& url,
WindowOpenDisposition disposition,
- SelectExpectation expectation,
+ bool expect_file_dialog,
+ const FilePath& expected_suggested_path,
int browser_test_flags) {
// Setup notification, navigate, and block.
- scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1));
+ scoped_ptr<DownloadsObserver> observer(
+ CreateWaiter(browser, 1, expect_file_dialog));
// This call will block until the condition specified by
// |browser_test_flags|, but will not wait for the download to finish.
ui_test_utils::NavigateToURLWithDisposition(browser,
@@ -643,22 +664,31 @@ class DownloadTest : public InProcessBrowserTest {
// Waits for the download to complete.
observer->WaitForFinished();
- // If specified, check the state of the select file dialog.
- if (expectation != EXPECT_NOTHING) {
- EXPECT_EQ(expectation == EXPECT_SELECT_DIALOG,
- observer->select_file_dialog_seen());
+ // Checks if the select file dialog was displayed as expected.
+ // If displayed, checks the suggested path in the dialog.
+ if (expect_file_dialog) {
+ EXPECT_TRUE(observer->select_file_dialog_seen());
+ EXPECT_EQ(observer->suggested_path(), expected_suggested_path);
+ } else {
+ EXPECT_FALSE(observer->select_file_dialog_seen());
}
}
// Download a file in the current tab, then wait for the download to finish.
- void DownloadAndWait(Browser* browser,
- const GURL& url,
- SelectExpectation expectation) {
+ // Expect that no select file dialog is displayed.
+ void DownloadAndWait(Browser* browser, const GURL& url) {
DownloadAndWaitWithDisposition(
- browser,
- url,
- CURRENT_TAB,
- expectation,
+ browser, url, CURRENT_TAB, false, FilePath(),
+ ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+ }
+
+ // Download a file in the current tab, then wait for the download to finish.
+ // Expect that a select file dialog suggesting |expected_suggested_path|
+ // is displayed.
+ void DownloadAndWaitWithDialog(Browser* browser, const GURL& url,
+ const FilePath& expected_suggested_path) {
+ DownloadAndWaitWithDisposition(
+ browser, url, CURRENT_TAB, true, expected_suggested_path,
ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
}
@@ -724,7 +754,7 @@ class DownloadTest : public InProcessBrowserTest {
// Download a partial web page in a background tab and wait.
// The mock system will not complete until it gets a special URL.
- scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1));
+ scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1, false));
ui_test_utils::NavigateToURL(browser, url);
// TODO(ahendrickson): check download status text before downloading.
@@ -806,6 +836,7 @@ class DownloadTest : public InProcessBrowserTest {
// TODO: Check for filename match in download shelf.
#endif
}
+
static void ExpectWindowCountAfterDownload(size_t expected) {
#if defined(OS_CHROMEOS)
// On ChromeOS, a download panel is created to display
@@ -815,6 +846,14 @@ class DownloadTest : public InProcessBrowserTest {
EXPECT_EQ(expected, BrowserList::size());
}
+ // Returns "src/chrome/test/data/{kTestFile}", whatever URL is given.
+ static net::URLRequestJob* FactoryForTestFile(
+ net::URLRequest* request, const std::string& scheme) {
+ FilePath test_dir;
+ EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir));
+ return new URLRequestMockHTTPJob(request, test_dir.Append(kTestFile));
+ }
+
private:
// Location of the test data.
FilePath test_dir_;
@@ -949,8 +988,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeType) {
FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
- // Download the file and wait. We do not expect the Select File dialog.
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ // Download the file and wait.
+ DownloadAndWait(browser(), url);
// Check state.
EXPECT_EQ(1, browser()->tab_count());
@@ -958,6 +997,76 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeType) {
CheckDownloadUI(browser(), true, true, file);
}
+// Checks if a file is saved to the user's "Downloads" folder
+// in the following situation:
+// The default folder for downloaded files does not exist.
+// The user's "Downloads" folder exists.
+//
+// This test creates and deletes a file on the user's real "Downloads" folder,
+// which is globally shared among all tests on the testing environment.
+// Therefore, if we run browser tests in parallel, the file created by one
+// browser test may be deleted by another broswer test when the file name
+// conflicts. In order to avoid this problem, we use a special mock URL
+// "http://mock.testfile.http/<random path>" for this download test.
+// Since we redirect "http://mock.testfile.http/<random path>" to
+// "chrome/test/data/{kTestFile}" using FactoryForTestFile(),
+// "chrome/test/data/{kTestFile}" is used for the file to be downloaded.
+// Then, the downloaded file is saved as a name "Downloads/<random path>",
+// which is a unique file name in the user's real "Downloads" folder.
+//
+// Ideally, in the first place, we should not use the user's "Downloads" folder.
+// Instead, we should create a temporary "Downloads" folder for each test.
+// However, we concluded that creating temporary "Downloads" folder for each
+// test requires very invasive code changes to many places. See also here:
+// http://codereview.chromium.org/6973052/
+IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadFolder) {
+ ASSERT_TRUE(InitialSetup(false));
+ FilePath file(kTestFile);
+
+ // Redirects "http://mock.testfile.http/<random path>"
+ // to "src/chrome/test/data/{kTestFile}", whatever the <random path> is.
+ std::string kMockHostnameForTestFile = "mock.testfile.http";
+ net::URLRequestFilter* filter = net::URLRequestFilter::GetInstance();
+ filter->AddHostnameHandler(
+ "http", kMockHostnameForTestFile, FactoryForTestFile);
+ std::string random_string = Generate128BitRandomBase64String();
+ RemoveChars(random_string, "/", &random_string);
+ GURL url("http://" + kMockHostnameForTestFile + "/" + random_string);
+
+ FilePath::StringType basename;
+#if defined(OS_WIN)
+ basename = UTF8ToWide(random_string);
+#else
+ basename = random_string;
+#endif
+ FilePath default_download_dir =
+ download_util::GetDefaultDownloadDirectoryFromPathService();
+ FilePath downloaded_file = default_download_dir.Append(basename);
+ // Make sure that the target file does not exist.
+ // This fails when the file does not exist, but it is OK.
+ file_util::Delete(downloaded_file, false);
+ // Make sure that the temporary file does not exist.
+ FilePath temporary_file = default_download_dir.Append(
+ basename + FILE_PATH_LITERAL(".crdownload"));
+ // This fails when the file does not exist, but it is OK.
+ file_util::Delete(temporary_file, false);
+
+ // Delete the default folder for downloaded files.
+ ASSERT_TRUE(DeleteDownloadsDirectory());
+ ASSERT_FALSE(file_util::PathExists(GetDownloadSaveDirectory(browser())));
+
+ // Download the file and wait.
+ DownloadAndWaitWithDialog(browser(), url, downloaded_file);
+
+ EXPECT_FALSE(file_util::PathExists(downloaded_file));
+ EXPECT_FALSE(file_util::PathExists(GetDownloadSaveDirectory(browser())));
+ EXPECT_EQ(1, browser()->tab_count());
+
+ // Clean up the generated files.
+ ASSERT_TRUE(file_util::Delete(downloaded_file, false));
+ ASSERT_TRUE(file_util::Delete(temporary_file, false));
+}
+
#if defined(OS_WIN)
// Download a file and confirm that the zone identifier (on windows)
// is set to internet.
@@ -966,8 +1075,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CheckInternetZone) {
FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
- // Download the file and wait. We do not expect the Select File dialog.
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ // Download the file and wait.
+ DownloadAndWait(browser(), url);
// Check state. Special file state must be checked before CheckDownload,
// as CheckDownload will delete the output file.
@@ -980,7 +1089,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CheckInternetZone) {
}
#endif
-// Put up a Select File dialog when the file is downloaded, due to its MIME
+// Put up a select file dialog when the file is downloaded, due to its MIME
// type.
//
// This test runs correctly, but leaves behind turds in the test user's
@@ -991,14 +1100,15 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeTypeSelect) {
ASSERT_TRUE(InitialSetup(true));
FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+ FilePath file_path(DestinationFile(browser(), file));
- // Download the file and wait. We expect the Select File dialog to appear
+ // Download the file and wait. We expect the select file dialog to appear
// due to the MIME type.
- DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
+ DownloadAndWaitWithDialog(browser(), url, file_path);
// Check state.
EXPECT_EQ(1, browser()->tab_count());
- // Since we exited while the Select File dialog was visible, there should not
+ // Since we exited while the select file dialog was visible, there should not
// be anything in the download shelf and so it should not be visible.
CheckDownloadUI(browser(), false, false, FilePath());
}
@@ -1033,7 +1143,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, ContentDisposition) {
FilePath download_file(FILE_PATH_LITERAL("download-test3-attachment.gif"));
// Download a file and wait.
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(browser(), url);
CheckDownload(browser(), download_file, file);
@@ -1053,7 +1163,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, PerWindowShelf) {
FilePath download_file(FILE_PATH_LITERAL("download-test3-attachment.gif"));
// Download a file and wait.
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(browser(), url);
CheckDownload(browser(), download_file, file);
@@ -1118,7 +1228,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, IncognitoDownload) {
// Since |incognito| is a separate browser, we have to set it up explicitly.
incognito->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
false);
- DownloadAndWait(incognito, url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(incognito, url);
// We should still have 2 windows.
ExpectWindowCountAfterDownload(2);
@@ -1185,7 +1295,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CloseNewTab1) {
browser(),
url,
NEW_BACKGROUND_TAB,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
// When the download finishes, we should still have one tab.
@@ -1217,7 +1328,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DontCloseNewTab2) {
DownloadAndWaitWithDisposition(browser(),
GURL("javascript:openNew()"),
CURRENT_TAB,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
// When the download finishes, we should have two tabs.
@@ -1259,7 +1371,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DontCloseNewTab3) {
DownloadAndWaitWithDisposition(browser(),
url,
CURRENT_TAB,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_NONE);
// When the download finishes, we should have two tabs.
@@ -1292,7 +1405,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CloseNewTab2) {
DownloadAndWaitWithDisposition(browser(),
GURL("javascript:openNew()"),
CURRENT_TAB,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
// When the download finishes, we should still have one tab.
@@ -1327,7 +1441,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CloseNewTab3) {
browser(),
GURL("javascript:document.getElementById('form').submit()"),
CURRENT_TAB,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
// When the download finishes, we should still have one tab.
@@ -1357,7 +1472,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, NewWindow) {
DownloadAndWaitWithDisposition(browser(),
url,
NEW_WINDOW,
- EXPECT_NO_SELECT_DIALOG,
+ false,
+ FilePath(),
ui_test_utils::BROWSER_TEST_NONE);
// When the download finishes, the download shelf SHOULD NOT be visible in
@@ -1464,8 +1580,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadHistoryCheck) {
int64 origin_size;
file_util::GetFileSize(origin_file, &origin_size);
- // Download the file and wait. We do not expect the Select File dialog.
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ // Download the file and wait.
+ DownloadAndWait(browser(), url);
// Get details of what downloads have just happened.
std::vector<DownloadItem*> downloads;
@@ -1502,7 +1618,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, ChromeURLAfterDownload) {
GURL extensions_url(chrome::kChromeUIExtensionsURL);
ui_test_utils::NavigateToURL(browser(), flags_url);
- DownloadAndWait(browser(), download_url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(browser(), download_url);
ui_test_utils::NavigateToURL(browser(), extensions_url);
TabContents* contents = browser()->GetSelectedTabContents();
ASSERT_TRUE(contents);
@@ -1539,7 +1655,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_BrowserCloseAfterDownload) {
&result));
EXPECT_TRUE(result);
- DownloadAndWait(browser(), download_url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(browser(), download_url);
ui_test_utils::WindowedNotificationObserver signal(
chrome::NOTIFICATION_BROWSER_CLOSED,
@@ -1556,13 +1672,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AnchorDownloadTag) {
// Create a download, wait until it's complete, and confirm
// we're in the expected state.
- scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser(), 1));
+ scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser(), 1, false));
ui_test_utils::NavigateToURL(browser(), url);
observer->WaitForFinished();
// Confirm the downloaded data exists.
- FilePath downloaded_file = GetDownloadDirectory(browser());
- downloaded_file = downloaded_file.Append(FILE_PATH_LITERAL("a_red_dot.png"));
+ FilePath downloaded_file(
+ DestinationFile(browser(), FilePath(FILE_PATH_LITERAL("a_red_dot.png"))));
EXPECT_TRUE(file_util::PathExists(downloaded_file));
}
@@ -1579,7 +1695,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AutoOpen) {
MockDownloadOpeningObserver observer(
browser()->profile()->GetDownloadManager());
- DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+ DownloadAndWait(browser(), url);
// Find the download and confirm it was opened.
std::vector<DownloadItem*> downloads;
« no previous file with comments | « no previous file | chrome/browser/download/download_file_manager.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698