chrome/browser/download/download_browsertest.cc - Issue 148133007: [Downloads] Always call DM::StartDownload() for explicit downloads.

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

Issue 148133007: [Downloads] Always call DM::StartDownload() for explicit downloads. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Created 6 years, 9 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

Index: chrome/browser/download/download_browsertest.cc

diff --git a/chrome/browser/download/download_browsertest.cc b/chrome/browser/download/download_browsertest.cc

index e1ec1c9d7e0c2dc0177a6d623e98b4db26101459..9d3acecb48e89368b1a75393abfa22188be8fca5 100644

--- a/chrome/browser/download/download_browsertest.cc

+++ b/chrome/browser/download/download_browsertest.cc

@@ -426,7 +426,8 @@ class DownloadTest : public InProcessBrowserTest {

// Information passed in to |DownloadFileCheckErrors()|.

struct DownloadInfo {

- const char* url_name; // URL for the download.

+ const char* starting_url; // URL for initiating the download.

+ const char* expected_download_url; // Expected value of DI::GetURL().

DownloadMethod download_method; // Navigation or Direct.

// Download interrupt reason (NONE is OK).

content::DownloadInterruptReason reason;

@@ -847,29 +848,37 @@ class DownloadTest : public InProcessBrowserTest {

void DownloadFilesCheckErrorsLoopBody(const DownloadInfo& download_info,

size_t i) {

- std::stringstream s;

- s << " " << __FUNCTION__ << "()"

- << " index = " << i

- << " url = '" << download_info.url_name << "'"

- << " method = "

- << ((download_info.download_method == DOWNLOAD_DIRECT) ?

- "DOWNLOAD_DIRECT" : "DOWNLOAD_NAVIGATE")

- << " show_item = " << download_info.show_download_item

- << " reason = " << DownloadInterruptReasonToString(download_info.reason);

+ SCOPED_TRACE(testing::Message()

+ << " " << __FUNCTION__ << "()"

+ << " index = " << i << " starting_url = '"

+ << download_info.starting_url << "'"

+ << " download_url = '" << download_info.expected_download_url

+ << "'"

+ << " method = "

+ << ((download_info.download_method == DOWNLOAD_DIRECT)

+ ? "DOWNLOAD_DIRECT"

+ : "DOWNLOAD_NAVIGATE") << " show_item = "

+ << download_info.show_download_item << " reason = "

+ << DownloadInterruptReasonToString(download_info.reason));

std::vector<DownloadItem*> download_items;

GetDownloads(browser(), &download_items);

size_t downloads_expected = download_items.size();

- std::string server_path = "files/downloads/";

- server_path += download_info.url_name;

- GURL url = test_server()->GetURL(server_path);

- ASSERT_TRUE(url.is_valid()) << s.str();

+ // GURL("http://foo/bar").Resolve("baz") => "http://foo/bar/baz"

+ // GURL("http://foo/bar").Resolve("http://baz") => "http://baz"

+ // I.e. both starting_url and expected_download_url can either be relative

+ // to the base test server URL or be an absolute URL.

+ GURL base_url = test_server()->GetURL("files/downloads/");

+ GURL starting_url = base_url.Resolve(download_info.starting_url);

+ GURL download_url = base_url.Resolve(download_info.expected_download_url);

+ ASSERT_TRUE(starting_url.is_valid());

+ ASSERT_TRUE(download_url.is_valid());

DownloadManager* download_manager = DownloadManagerForBrowser(browser());

WebContents* web_contents =

browser()->tab_strip_model()->GetActiveWebContents();

- ASSERT_TRUE(web_contents) << s.str();

+ ASSERT_TRUE(web_contents);

scoped_ptr<content::DownloadTestObserver> observer(

new content::DownloadTestObserverTerminal(

@@ -883,7 +892,7 @@ class DownloadTest : public InProcessBrowserTest {

creation_observer(new content::DownloadTestItemCreationObserver);

scoped_ptr<DownloadUrlParameters> params(

- DownloadUrlParameters::FromWebContents(web_contents, url));

+ DownloadUrlParameters::FromWebContents(web_contents, starting_url));

params->set_callback(creation_observer->callback());

DownloadManagerForBrowser(browser())->DownloadUrl(params.Pass());

@@ -891,24 +900,12 @@ class DownloadTest : public InProcessBrowserTest {

// won't be.

creation_observer->WaitForDownloadItemCreation();

- EXPECT_EQ(download_info.show_download_item,

- creation_observer->succeeded());

- if (download_info.show_download_item) {

- EXPECT_EQ(content::DOWNLOAD_INTERRUPT_REASON_NONE,

- creation_observer->interrupt_reason());

- EXPECT_NE(content::DownloadItem::kInvalidId,

- creation_observer->download_id());

- } else {

- EXPECT_NE(content::DOWNLOAD_INTERRUPT_REASON_NONE,

- creation_observer->interrupt_reason());

- EXPECT_EQ(content::DownloadItem::kInvalidId,

- creation_observer->download_id());

- }

+ EXPECT_NE(content::DownloadItem::kInvalidId,

+ creation_observer->download_id());

} else {

// Navigate to URL normally, wait until done.

- ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(browser(),

- url,

- 1);

+ ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(

+ browser(), starting_url, 1);

}

if (download_info.show_download_item) {

@@ -928,7 +925,7 @@ class DownloadTest : public InProcessBrowserTest {

// Validate that the correct files were downloaded.

download_items.clear();

GetDownloads(browser(), &download_items);

- ASSERT_EQ(downloads_expected, download_items.size()) << s.str();

+ ASSERT_EQ(downloads_expected, download_items.size());

if (download_info.show_download_item) {

// Find the last download item.

@@ -938,9 +935,8 @@ class DownloadTest : public InProcessBrowserTest {

item = download_items[d];

}

- ASSERT_EQ(url, item->GetOriginalUrl()) << s.str();

- ASSERT_EQ(download_info.reason, item->GetLastReason()) << s.str();

+ EXPECT_EQ(download_url, item->GetURL());

+ EXPECT_EQ(download_info.reason, item->GetLastReason());

if (item->GetState() == content::DownloadItem::COMPLETE) {

// Clean up the file, in case it ended up in the My Documents folder.

@@ -1019,7 +1015,7 @@ class DownloadTest : public InProcessBrowserTest {

for (size_t i = 0; i < count; ++i) {

// Set up the full URL, for download file tracking.

std::string server_path = "files/downloads/";

- server_path += info[i].download_info.url_name;

+ server_path += info[i].download_info.starting_url;

GURL url = test_server()->GetURL(server_path);

info[i].error_info.url = url.spec();

@@ -1269,12 +1265,12 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadResourceThrottleCancels) {

// Try to start the download via Javascript and wait for the corresponding

// load stop event.

content::TestNavigationObserver observer(web_contents);

- bool download_assempted;

+ bool download_attempted;

ASSERT_TRUE(content::ExecuteScriptAndExtractBool(

browser()->tab_strip_model()->GetActiveWebContents(),

"window.domAutomationController.send(startDownload());",

- &download_assempted));

- ASSERT_TRUE(download_assempted);

+ &download_attempted));

+ ASSERT_TRUE(download_attempted);

observer.Wait();

// Check that we did not download the file.

@@ -2417,6 +2413,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

DownloadInfo download_info[] = {

{ // Normal navigated download.

"a_zip_file.zip",

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_NONE,

true,

@@ -2424,6 +2421,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

{ // Normal direct download.

"a_zip_file.zip",

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_NONE,

true,

@@ -2431,6 +2429,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

{ // Direct download with 404 error.

"there_IS_no_spoon.zip",

+ "there_IS_no_spoon.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT,

true,

@@ -2438,6 +2437,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

{ // Navigated download with 404 error.

"there_IS_no_spoon.zip",

+ "there_IS_no_spoon.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT,

false,

@@ -2445,6 +2445,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

{ // Direct download with 400 error.

"zip_file_not_found.zip",

+ "zip_file_not_found.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,

true,

@@ -2452,10 +2453,49 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

{ // Navigated download with 400 error.

"zip_file_not_found.zip",

+ "",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,

false,

false

+ },

+ { // Simulates clicking on <a href="http://..." download="">. The name does

+ // not resolve. But since this is an explicit download, the download

+ // should appear on the shelf and the error should be indicated.

+ "download-anchor-attrib-name-not-resolved.html",

+ "http://doesnotexist/shouldnotberesolved",

+ DOWNLOAD_NAVIGATE,

+ content::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED,

+ true,

+ false

+ },

+ { // Simulates clicking on <a href="http://..." download=""> where the URL

+ // leads to a 404 response. This is different from the previous test case

+ // in that the ResourceLoader issues a OnResponseStarted() callback since

+ // the headers are successfully received.

+ "download-anchor-attrib-404.html",

+ "there_IS_no_spoon.zip",

+ DOWNLOAD_NAVIGATE,

+ content::DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT,

+ true,

+ false

+ },

+ { // Similar to the above, but the resulting response contains a status

+ // code of 400.

+ "download-anchor-attrib-400.html",

+ "zip_file_not_found.zip",

+ DOWNLOAD_NAVIGATE,

+ content::DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED,

+ true,

+ false

+ },

+ { // Direct download of a URL where the hostname doesn't resolve.

+ "http://doesnotexist/shouldnotdownloadsuccessfully",

+ DOWNLOAD_DIRECT,

+ content::DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED,

+ true,

+ false

}

};

@@ -2465,10 +2505,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsServer) {

IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

FileErrorInjectInfo error_info[] = {

{ // Navigated download with injected "Disk full" error in Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,

- 1

+ true,

+ false

{

"",

@@ -2478,10 +2521,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Disk full" error in Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,

- 1

+ true,

+ false

{

"",

@@ -2491,10 +2537,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Navigated download with injected "Disk full" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,

- 1

+ true,

+ false

{

"",

@@ -2504,10 +2553,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Disk full" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,

- 1

+ true,

+ false

{

"",

@@ -2517,10 +2569,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Navigated download with injected "Failed" error in Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2530,10 +2585,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Failed" error in Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2543,10 +2601,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Navigated download with injected "Failed" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2556,10 +2617,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Failed" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2570,10 +2634,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

{ // Navigated download with injected "Name too long" error in

// Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NAME_TOO_LONG,

- 1

+ true,

+ false

{

"",

@@ -2583,10 +2650,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Name too long" error in Initialize().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NAME_TOO_LONG,

- 1

+ true,

+ false

{

"",

@@ -2596,10 +2666,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Navigated download with injected "Name too long" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2609,10 +2682,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Name too long" error in Write().

- { "a_zip_file.zip",

+ {

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_FAILED,

- 1

+ true,

+ false

{

"",

@@ -2622,10 +2698,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorsFile) {

}

{ // Direct download with injected "Disk full" error in 2nd Write().

- { "06bESSE21Evolution.ppt",

+ {

+ "06bESSE21Evolution.ppt",

DOWNLOAD_DIRECT,

content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,

- 1

+ true,

+ false

{

"",

@@ -2643,6 +2722,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolder) {

DownloadInfo download_info[] = {

{

"a_zip_file.zip",

+ "a_zip_file.zip",

DOWNLOAD_DIRECT,

// This passes because we switch to the My Documents folder.

content::DOWNLOAD_INTERRUPT_REASON_NONE,

@@ -2651,6 +2731,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorReadonlyFolder) {

{

"a_zip_file.zip",

+ "a_zip_file.zip",

DOWNLOAD_NAVIGATE,

// This passes because we switch to the My Documents folder.

content::DOWNLOAD_INTERRUPT_REASON_NONE,

« no previous file with comments | « no previous file | chrome/browser/download/download_target_determiner.cc » ('j') | content/browser/download/download_item_impl.cc » ('J')