Converted download UI tests to Browser tests.

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 6f0d99889c35b608a853f8dca61254cde649d784..e346913af9b09b4b4cac2c288f38f732000bb466 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -2,24 +2,32 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <stack>

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

Do we still need this?  

[ahendrickson, 2011/01/06 22:33:39]

No.

+
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/scoped_temp_dir.h"
 #include "base/test/test_file_util.h"
-#include "chrome/browser/ui/browser.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/download/download_item.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/net/url_request_mock_http_job.h"
+#include "chrome/browser/net/url_request_slow_download_job.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/page_transition_types.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/in_process_browser_test.h"
 #include "chrome/test/ui_test_utils.h"
+#include "net/base/net_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -213,54 +221,137 @@ class DownloadsObserver : public DownloadManager::Observer,
 };
 
 class DownloadTest : public InProcessBrowserTest {
- protected:
-  void SetUpInProcessBrowserTestFixture() {
-    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
+ public:
+  enum SelectExpectation {
+    EXPECT_NO_SELECT_DIALOG = -1,
+    EXPECT_NOTHING,
+    EXPECT_SELECT_DIALOG
+  };
+

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

I'd put a comment here that a false return implies setup failure and should be
asserted on in the caller.  That should probably be done for all the helper
routines.

[ahendrickson, 2011/01/06 22:33:39]

Done.

+  virtual bool InitialSetup(bool prompt_for_download) {
+    bool have_test_dir = PathService::Get(chrome::DIR_TEST_DATA, &test_dir_);
+    EXPECT_TRUE(have_test_dir);
+    if (!have_test_dir)
+      return false;
+
+    // Sanity check default values for window / tab count and shelf visibility.
+    int window_count = BrowserList::size();
+    EXPECT_EQ(1, window_count);
+    EXPECT_EQ(1, browser()->tab_count());
+    bool is_shelf_visible = browser()->window()->IsDownloadShelfVisible();
+    EXPECT_FALSE(is_shelf_visible);
+
+    // Set up the temporary download folder.
+    bool created_downloads_dir = CreateAndSetDownloadsDirectory(browser());
+    EXPECT_TRUE(created_downloads_dir);
+    if (!created_downloads_dir)
+      return false;
+    browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                                 prompt_for_download);
+
+    return true;
   }
 
+ protected:
+
   // Must be called after browser creation.  Creates a temporary
   // directory for downloads that is auto-deleted on destruction.
-  bool CreateAndSetDownloadsDirectory() {
-    if (downloads_directory_.CreateUniqueTempDir()) {
-      browser()->profile()->GetPrefs()->SetFilePath(
-          prefs::kDownloadDefaultDirectory,
-          downloads_directory_.path());
-      return true;
-    }
-    return false;
+  bool CreateAndSetDownloadsDirectory(Browser* browser) {
+    if (!browser)
+      return false;
+
+    if (!downloads_directory_.CreateUniqueTempDir())
+      return false;
+
+    browser->profile()->GetPrefs()->SetFilePath(
+        prefs::kDownloadDefaultDirectory,
+        downloads_directory_.path());
+
+    return true;
   }
 
-  // May only be called inside of an individual test; browser() is NULL
+  // May only be called inside of an individual test; browser is NULL
   // outside of that context.

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

This comment is left-over from when the routine called browser() to get the
browser and is a little confusing outside of that context.  It'd be more
accurate to say "browser must be specified by the caller", but even that is
pretty obvious from the calling signature and could be left out.

[ahendrickson, 2011/01/06 22:33:39]

Done.

-  FilePath GetDownloadDirectory() {
+  FilePath GetDownloadDirectory(Browser* browser) {
     DownloadManager* download_mananger =
-        browser()->profile()->GetDownloadManager();
+        browser->profile()->GetDownloadManager();
     return download_mananger->download_prefs()->download_path();
   }
 
-  DownloadsObserver* CreateWaiter(int num_downloads) {
+  DownloadsObserver* CreateWaiter(Browser* browser, int num_downloads) {
     DownloadManager* download_manager =
-        browser()->profile()->GetDownloadManager();
+        browser->profile()->GetDownloadManager();
     return new DownloadsObserver(
         download_manager, num_downloads,
         DownloadsObserver::FILE_RENAME,  // Really done
-        true);                          // Bail on select file
+        true);                           // Bail on select file
+  }
+
+  // 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
+  // open when the download is finished, or if we don't care.
+  // If the dialog appears, the test completes.  The only effect |expectation|
+  // has is whether or not the test succeeds.
+  // |browser_test_flags| indicate what to wait for, and is an OR of 0 or more
+  // values in the BrowserTestWaitFlags enum.

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

I'd refer to this as ui_test_utils::BrowserTestWaitFlags enum so folks know
where to look for it if they're curious.

[ahendrickson, 2011/01/06 22:33:39]

Done.

+  void DownloadAndWaitWithDisposition(Browser* browser,
+                                      const GURL& url,
+                                      WindowOpenDisposition disposition,
+                                      SelectExpectation expectation,
+                                      int browser_test_flags) {
+    // Setup notification, navigate, and block.
+    scoped_ptr<DownloadsObserver> observer(CreateWaiter(browser, 1));
+    // 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,
+                                                url,
+                                                disposition,
+                                                browser_test_flags);
+    // 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());
+    }
+  }
+
+  // Download a file in the current tab, then wait for the download to finish.
+  void DownloadAndWait(Browser* browser,
+                       const GURL& url,
+                       SelectExpectation expectation) {
+    DownloadAndWaitWithDisposition(
+        browser,
+        url,
+        CURRENT_TAB,
+        expectation,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   }
 
   // Should only be called when the download is known to have finished
   // (in error or not).
-  void CheckDownload(const FilePath& downloaded_filename,
+  bool CheckDownload(const FilePath& downloaded_filename,
                      const FilePath& origin_filename) {

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

For consistency of interface among the different methods of DownloadTest, I'm
inclined to think that this should take a browser argument if it needs one
rather than relying on browser().  (This is a suggestion, not a
requirement--it's test code, and I weigh programmer convenience more highly in
the convenience vs. consistency balance in test code.)

[ahendrickson, 2011/01/06 22:33:39]

Done.

     // Find the path to which the data will be downloaded.
     FilePath downloaded_file =
-        GetDownloadDirectory().Append(downloaded_filename);
+        GetDownloadDirectory(browser()).Append(downloaded_filename);
 
     // Find the origin path (from which the data comes).
     FilePath origin_file(test_dir_.Append(origin_filename));
-    ASSERT_TRUE(file_util::PathExists(origin_file));
+    bool origin_file_exists = file_util::PathExists(origin_file);
+    EXPECT_TRUE(origin_file_exists);
+    if (!origin_file_exists)
+      return false;
 
     // Confirm the downloaded data file exists.
-    ASSERT_TRUE(file_util::PathExists(downloaded_file));
+    bool downloaded_file_exists = file_util::PathExists(downloaded_file);
+    EXPECT_TRUE(downloaded_file_exists);
+    if (!downloaded_file_exists)
+      return false;
+
     int64 origin_file_size = 0;
     int64 downloaded_file_size = 0;
     EXPECT_TRUE(file_util::GetFileSize(origin_file, &origin_file_size));
@@ -275,7 +366,69 @@ class DownloadTest : public InProcessBrowserTest {
 #endif
 
     // Delete the downloaded copy of the file.
-    EXPECT_TRUE(file_util::DieFileDie(downloaded_file, false));
+    bool downloaded_file_deleted = file_util::DieFileDie(downloaded_file, false);

[Randy Smith (Not in Mondays), 2011/01/06 18:31:17]

Nit: Make < 80 characters.

[ahendrickson, 2011/01/06 22:33:39]

Done.

+    EXPECT_TRUE(downloaded_file_deleted);
+    if (!downloaded_file_deleted)
+      return false;
+
+    return true;
+  }
+
+  // TODO(ahendrickson) -- |expected_title_in_progress| and
+  // |expected_title_in_finished| need to be checked.
+  bool RunSizeTest(const GURL& url,
+                   const std::wstring& expected_title_in_progress,
+                   const std::wstring& expected_title_finished) {
+    if (!InitialSetup(false))
+      return false;
+
+    // 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));
+    ui_test_utils::NavigateToURL(browser(), url);
+
+    // TODO(ahendrickson): check download status text before downloading.
+    // Need to:
+    //  - Add a member function to the |DownloadShelf| interface class, that
+    //    indicates how many members it has.
+    //  - Add a member function to |DownloadShelf| to get the status text
+    //    of a given member (for example, via |DownloadItemView|'s
+    //    GetAccessibleName() member function), by index.
+    //  - Iterate over browser()->window()->GetDownloadShelf()'s members
+    //    to see if any match the status text we want.  Start with the last one.
+
+    // Complete sending the request.  We do this by loading a second URL in a
+    // separate tab.
+    GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl);
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(),
+        finish_url,
+        NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+    observer->WaitForFinished();
+
+    EXPECT_EQ(2, browser()->tab_count());
+
+    // TODO(ahendrickson): check download status text after downloading.
+
+    // Make sure the download shelf is showing.
+    EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+    FilePath filename;
+    net::FileURLToFilePath(url, &filename);
+    filename = filename.BaseName();
+    FilePath download_path = downloads_directory_.path().Append(filename);
+
+    bool downloaded_path_exists = file_util::PathExists(download_path);
+    EXPECT_TRUE(downloaded_path_exists);
+    if (!downloaded_path_exists)
+      return false;
+
+    // Delete the file we just downloaded.
+    EXPECT_TRUE(file_util::DieFileDie(download_path, true));
+    EXPECT_FALSE(file_util::PathExists(download_path));
+
+    return true;
   }
 
  private:
@@ -286,51 +439,504 @@ class DownloadTest : public InProcessBrowserTest {
   ScopedTempDir downloads_directory_;
 };
 
+// NOTES:
+//
+// Files for these tests are found in DIR_TEST_DATA (currently
+// "chrome\test\data\", see chrome_paths.cc).
+// Mock responses have extension .mock-http-headers appended to the file name.
+
+// Download a file due to the associated MIME type.
+//
 // Test is believed good (non-flaky) in itself, but it
 // sometimes trips over underlying flakiness in the downloads
 // subsystem in in http://crbug.com/63237.  Until that bug is
 // fixed, this test should be considered flaky.  It's entered as
 // DISABLED since if 63237 does cause a failure, it'll be a timeout.
 IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeType) {
+  ASSERT_TRUE(InitialSetup(false));
   FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
-  ASSERT_TRUE(CreateAndSetDownloadsDirectory());
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
 
-  EXPECT_EQ(1, browser()->tab_count());
+  // Download the file and wait.  We do not expect the Select File dialog.
+  DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
 
-  // Setup notification, navigate, and block.
-  scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
-  ui_test_utils::NavigateToURL(
-      browser(), URLRequestMockHTTPJob::GetMockUrl(file));
-  observer->WaitForFinished();
-
-  // Download should be finished; check state.
-  EXPECT_FALSE(observer->select_file_dialog_seen());
+  // Check state.
   EXPECT_EQ(1, browser()->tab_count());
   CheckDownload(file, file);
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 }
 
+// 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
 // download directory because of http://crbug.com/62099.  No big loss; it
 // was primarily confirming DownloadsObserver wait on select file dialog
 // functionality anyway.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
 IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DownloadMimeTypeSelect) {
+  ASSERT_TRUE(InitialSetup(true));
   FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
-  ASSERT_TRUE(CreateAndSetDownloadsDirectory());
-  browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload, true);
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // Download the file and wait.  We expect the Select File dialog to appear
+  // due to the MIME type.
+  DownloadAndWait(browser(), url, EXPECT_SELECT_DIALOG);
 
+  // Check state.
   EXPECT_EQ(1, browser()->tab_count());
+  // 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.
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+}
 
-  // Setup notification, navigate, and block.
-  scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
-  ui_test_utils::NavigateToURL(
-      browser(), URLRequestMockHTTPJob::GetMockUrl(file));
-  observer->WaitForFinished();
+// Access a file with a viewable mime-type, verify that a download
+// did not initiate.
+IN_PROC_BROWSER_TEST_F(DownloadTest, NoDownload) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-test2.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  FilePath file_path = GetDownloadDirectory(browser()).Append(file);
 
-  // Download should not be finished; check state.
-  EXPECT_TRUE(observer->select_file_dialog_seen());
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Check that we did not download the web page.
+  EXPECT_FALSE(file_util::PathExists(file_path));
+
+  // Check state.
   EXPECT_EQ(1, browser()->tab_count());
   EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
 }
 
+// Download a 0-size file with a content-disposition header, verify that the
+// download tab opened and the file exists as the filename specified in the
+// header.  This also ensures we properly handle empty file downloads.
+// The download shelf should be visible in the current tab.
+//
+// Test is believed mostly good (non-flaky) in itself, but it
+// sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_ContentDisposition) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-test3.gif"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  FilePath download_file(FILE_PATH_LITERAL("download-test3-attachment.gif"));
+
+  // Download a file and wait.
+  DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+
+  CheckDownload(download_file, file);
+
+  // Check state.
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+}
+
+// Test that the download shelf is per-window by starting a download in one
+// tab, opening a second tab, closing the shelf, going back to the first tab,
+// and checking that the shelf is closed.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_PerWindowShelf) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-test3.gif"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  FilePath download_file(FILE_PATH_LITERAL("download-test3-attachment.gif"));
+
+  // Download a file and wait.
+  DownloadAndWait(browser(), url, EXPECT_NO_SELECT_DIALOG);
+
+  CheckDownload(download_file, file);
+
+  // Check state.
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Open a second tab and wait.
+  EXPECT_NE(static_cast<TabContentsWrapper*>(NULL),
+            browser()->AddSelectedTabWithURL(GURL(), PageTransition::TYPED));
+  EXPECT_EQ(2, browser()->tab_count());
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+
+  // Hide the download shelf.
+  browser()->window()->GetDownloadShelf()->Close();
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  // Go to the first tab.
+  browser()->SelectTabContentsAt(0, true);
+  EXPECT_EQ(2, browser()->tab_count());
+
+  // The download shelf should not be visible.
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+}
+
+// UnknownSize and KnownSize are tests which depend on
+// URLRequestSlowDownloadJob to serve content in a certain way. Data will be
+// sent in two chunks where the first chunk is 35K and the second chunk is 10K.
+// The test will first attempt to download a file; but the server will "pause"
+// in the middle until the server receives a second request for
+// "download-finish".  At that time, the download will finish.
+// These tests don't currently test much due to holes in |RunSizeTest()|.  See
+// comments in that routine for details.
+
+// Test is believed mostly good (non-flaky) in itself, but it
+// very occasionally trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_UnknownSize) {
+  GURL url(URLRequestSlowDownloadJob::kUnknownSizeUrl);
+  FilePath filename;
+  net::FileURLToFilePath(url, &filename);
+  filename = filename.BaseName();
+  ASSERT_TRUE(RunSizeTest(url,
+                          L"32.0 KB - " + filename.ToWStringHack(),
+                          L"100% - " + filename.ToWStringHack()));
+}
+
+// Test is believed mostly good (non-flaky) in itself, but it
+// very occasionally trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_KnownSize) {
+  GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl);
+  FilePath filename;
+  net::FileURLToFilePath(url, &filename);
+  filename = filename.BaseName();
+  ASSERT_TRUE(RunSizeTest(url,
+                          L"71% - " + filename.ToWStringHack(),
+                          L"100% - " + filename.ToWStringHack()));
+}
+
+// Test that when downloading an item in Incognito mode, we don't crash when
+// closing the last Incognito window (http://crbug.com/13983).
+// Also check that the download shelf is not visible after closing the
+// Incognito window.
+//
+// Test is believed mostly good (non-flaky) in itself, but it
+// sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_IncognitoDownload) {
+  ASSERT_TRUE(InitialSetup(false));
+
+  // Open an Incognito window.
+  Browser* incognito = CreateIncognitoBrowser();  // Waits.
+  ASSERT_TRUE(incognito);
+  int window_count = BrowserList::size();
+  EXPECT_EQ(2, window_count);
+
+  // Download a file in the Incognito window and wait.
+  CreateAndSetDownloadsDirectory(incognito);
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  // 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);
+
+  // We should still have 2 windows.
+  window_count = BrowserList::size();
+  EXPECT_EQ(2, window_count);
+
+  // Verify that the download shelf is showing for the Incognito window.
+  bool is_shelf_visible = incognito->window()->IsDownloadShelfVisible();
+  EXPECT_TRUE(is_shelf_visible);
+
+  // Close the Incognito window and don't crash.
+  incognito->CloseWindow();
+  ui_test_utils::WaitForNotificationFrom(NotificationType::BROWSER_CLOSED,
+                                         Source<Browser>(incognito));
+
+  window_count = BrowserList::size();
+  EXPECT_EQ(1, window_count);
+
+  // Verify that the regular window does not have a download shelf.
+  is_shelf_visible = browser()->window()->IsDownloadShelfVisible();
+  EXPECT_FALSE(is_shelf_visible);
+
+  CheckDownload(file, file);
+}
+
+// Navigate to a new background page, but don't download.  Confirm that the
+// download shelf is not visible and that we have two tabs.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DontCloseNewTab1) {
+  ASSERT_TRUE(InitialSetup(false));
+  // Because it's an HTML link, it should open a web page rather than
+  // downloading.
+  FilePath file1(FILE_PATH_LITERAL("download-test2.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file1));
+
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      url,
+      NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  // We should have two tabs now.
+  EXPECT_EQ(2, browser()->tab_count());
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+}
+
+// Download a file in a background tab. Verify that the tab is closed
+// automatically, and that the download shelf is visible in the current tab.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_CloseNewTab1) {
+  ASSERT_TRUE(InitialSetup(false));
+
+  // Download a file in a new background tab and wait.  The tab is automatically
+  // closed when the download begins.
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  DownloadAndWaitWithDisposition(
+      browser(),
+      url,
+      NEW_BACKGROUND_TAB,
+      EXPECT_NO_SELECT_DIALOG,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+  // When the download finishes, we should still have one tab.
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  EXPECT_EQ(1, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Open a web page in the current tab, then download a file in another tab via
+// a Javascript call.
+// Verify that we have 2 tabs, and the download shelf is visible in the current
+// tab.
+//
+// The download_page1.html page contains an openNew() function that opens a
+// tab and then downloads download-test1.lib.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DontCloseNewTab2) {
+  ASSERT_TRUE(InitialSetup(false));
+  // Because it's an HTML link, it should open a web page rather than
+  // downloading.
+  FilePath file1(FILE_PATH_LITERAL("download_page1.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file1));
+
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Download a file in a new tab and wait (via Javascript).
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  DownloadAndWaitWithDisposition(browser(),
+                                 GURL("javascript:openNew()"),
+                                 CURRENT_TAB,
+                                 EXPECT_NO_SELECT_DIALOG,
+                                 ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
+
+  // When the download finishes, we should have two tabs.
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  EXPECT_EQ(2, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Open a web page in the current tab, open another tab via a Javascript call,
+// then download a file in the new tab.
+// Verify that we have 2 tabs, and the download shelf is visible in the current
+// tab.
+//
+// The download_page2.html page contains an openNew() function that opens a
+// tab.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DontCloseNewTab3) {
+  ASSERT_TRUE(InitialSetup(false));
+  // Because it's an HTML link, it should open a web page rather than
+  // downloading.
+  FilePath file1(FILE_PATH_LITERAL("download_page2.html"));
+  GURL url1(URLRequestMockHTTPJob::GetMockUrl(file1));
+
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url1);
+
+  // Open a new tab and wait.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(),
+      GURL("javascript:openNew()"),
+      CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
+
+  EXPECT_EQ(2, browser()->tab_count());
+
+  // Download a file and wait.
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  DownloadAndWaitWithDisposition(browser(),
+                                 url,
+                                 CURRENT_TAB,
+                                 EXPECT_NO_SELECT_DIALOG,
+                                 ui_test_utils::BROWSER_TEST_NONE);
+
+  // When the download finishes, we should have two tabs.
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  EXPECT_EQ(2, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Open a web page in the current tab, then download a file via Javascript,
+// which will do so in a temporary tab.
+// Verify that we have 1 tab, and the download shelf is visible.
+//
+// The download_page3.html page contains an openNew() function that opens a
+// tab with download-test1.lib in the URL.  When the URL is determined to be
+// a download, the tab is closed automatically.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_CloseNewTab2) {
+  ASSERT_TRUE(InitialSetup(false));
+  // Because it's an HTML link, it should open a web page rather than
+  // downloading.
+  FilePath file1(FILE_PATH_LITERAL("download_page3.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file1));
+
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Download a file and wait.
+  // The file to download is "download-test1.lib".
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  DownloadAndWaitWithDisposition(browser(),
+                                 GURL("javascript:openNew()"),
+                                 CURRENT_TAB,
+                                 EXPECT_NO_SELECT_DIALOG,
+                                 ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
+
+  // When the download finishes, we should still have one tab.
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  EXPECT_EQ(1, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Open a web page in the current tab, then call Javascript via a button to
+// download a file in a new tab, which is closed automatically when the
+// download begins.
+// Verify that we have 1 tab, and the download shelf is visible.
+//
+// The download_page4.html page contains a form with download-test1.lib as the
+// action.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_CloseNewTab3) {
+  ASSERT_TRUE(InitialSetup(false));
+  // Because it's an HTML link, it should open a web page rather than
+  // downloading.
+  FilePath file1(FILE_PATH_LITERAL("download_page4.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file1));
+
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url);
+
+  // Download a file in a new tab and wait.  The tab will automatically close
+  // when the download begins.
+  // The file to download is "download-test1.lib".
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  DownloadAndWaitWithDisposition(
+      browser(),
+      GURL("javascript:document.getElementById('form').submit()"),
+      CURRENT_TAB,
+      EXPECT_NO_SELECT_DIALOG,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_TAB);
+
+  // When the download finishes, we should still have one tab.
+  EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  EXPECT_EQ(1, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Download a file in a new window.
+// Verify that we have 2 windows, and the download shelf is not visible in the
+// first window, but is visible in the second window.
+// Close the new window.
+// Verify that we have 1 window, and the download shelf is not visible.
+//
+// Regression test for http://crbug.com/44454
+//
+// Test is believed mostly good (non-flaky) in itself, but it
+// sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.  Until that bug is
+// fixed, this test should be considered flaky.  It's entered as
+// DISABLED since if 63237 does cause a failure, it'll be a timeout.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_NewWindow) {
+  ASSERT_TRUE(InitialSetup(false));
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  Browser* first_browser = browser();
+
+  // Download a file in a new window and wait.
+  DownloadAndWaitWithDisposition(browser(),
+                                 url,
+                                 NEW_WINDOW,
+                                 EXPECT_NO_SELECT_DIALOG,
+                                 ui_test_utils::BROWSER_TEST_NONE);
+
+  // When the download finishes, the download shelf SHOULD NOT be visible in
+  // the first window.
+  int window_count = BrowserList::size();
+  EXPECT_EQ(2, window_count);
+  EXPECT_EQ(1, browser()->tab_count());
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  // The download shelf SHOULD be visible in the second window.
+  std::set<Browser*> original_browsers;
+  original_browsers.insert(browser());
+  Browser* download_browser =
+      ui_test_utils::GetBrowserNotInSet(original_browsers);
+  ASSERT_TRUE(download_browser != NULL);
+  EXPECT_NE(download_browser, browser());
+  EXPECT_EQ(1, download_browser->tab_count());
+  EXPECT_TRUE(download_browser->window()->IsDownloadShelfVisible());
+
+  // Close the new window.
+  download_browser->CloseWindow();
+  ui_test_utils::WaitForNotificationFrom(NotificationType::BROWSER_CLOSED,
+                                         Source<Browser>(download_browser));
+  EXPECT_EQ(first_browser, browser());
+  window_count = BrowserList::size();
+  EXPECT_EQ(1, window_count);
+  EXPECT_EQ(1, browser()->tab_count());
+  // The download shelf should not be visible in the remaining window.
+  EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  CheckDownload(file, file);
+}
+
 }  // namespace