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 913e5f214c8845d70cddc726190e66f35bcc52ab..8839c8dc3984accc9af8db72f54b655ab2a37afc 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -2,25 +2,33 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <stack>
+
 #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/notification_service.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 {
@@ -214,19 +222,54 @@ class DownloadsObserver : public DownloadManager::Observer,
 };
 
 class DownloadTest : public InProcessBrowserTest {
+ public:
+  enum SelectExpectation {
+    EXPECT_NO_SELECT_DIALOG = -1,
+    EXPECT_NOTHING,
+    EXPECT_SELECT_DIALOG
+  };
+
+  virtual void InitialSetup(bool prompt_for_download) {
+    // Sanity check default values for window / tab count and shelf visibility.

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Suggest moving comment down one line; it doesn't describe what's happening in
PushBrowser().

[ahendrickson, 2010/12/22 22:16:52]

Done.

+    PushBrowser(InProcessBrowserTest::browser());  // Set the initial browser.
+    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.
+    EXPECT_TRUE(CreateAndSetDownloadsDirectory());
+
+    browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                                 prompt_for_download);
+  }
+
  protected:
+
+  // We now use a browser stack.
+  void PushBrowser(Browser* browser) { browsers_.push(browser); }
+  void PopBrowser() { browsers_.pop(); }
+
+  virtual Browser* browser() const {
+    return browsers_.empty() ? NULL : browsers_.top();
+  }
+
   void SetUpInProcessBrowserTestFixture() {
-    ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
+    EXPECT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Given that there's no way to propagate the success or failure of this call up
the stack to abort the test if it fails, I'd put an ASSERT_* (or the moral
equivalent, i.e. an EXPECT_* followed by a return false, and an ASSERT_TRUE at
top test level) that test_dir_ was initialized correctly somewhere.  If this
fails, everything else is gonna barf all over the floor, which sorta defines
what needs to be ASSERT_'d to me.

[ahendrickson, 2010/12/22 22:16:52]

Unfortunately, this is an inherited function that returns void.

I moved this to |InitialSetup()|, and made that return a boolean. 
|RunSizeTest()| is also affected.

   }
 
   // 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;
+    if (browser()) {

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Nit/suggestion: if (!browser()) return false; ...
That'd allow the rest of the routine not to be indented.

[ahendrickson, 2010/12/22 22:16:52]

Done.

+      if (downloads_directory_.CreateUniqueTempDir()) {
+        browser()->profile()->GetPrefs()->SetFilePath(
+            prefs::kDownloadDefaultDirectory,
+            downloads_directory_.path());
+         return true;
+      }
     }
     return false;
   }
@@ -245,7 +288,51 @@ class DownloadTest : public InProcessBrowserTest {
     return new DownloadsObserver(
         download_manager, num_downloads,
         DownloadsObserver::FILE_RENAME,  // Really done
-        true);                          // Bail on select file
+        true);                           // Bail on select file
+  }
+
+  // Download a file, then wait for the download to finish.
+  // |disposition| indicates where the navigation occurs (current tab, new
+  // foreground tab, etc).

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Nit: You left out url.  It's pretty obvious what it does, so it's not worth many
words; maybe just change the first phrase to "Download |url|"?  (It feels weird
to list all the args except that one.)

[ahendrickson, 2010/12/22 22:16:52]

Done.

+  // |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 BrowserTestFlags enum.
+  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(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
@@ -258,10 +345,10 @@ class DownloadTest : public InProcessBrowserTest {
 
     // Find the origin path (from which the data comes).
     FilePath origin_file(test_dir_.Append(origin_filename));
-    ASSERT_TRUE(file_util::PathExists(origin_file));
+    EXPECT_TRUE(file_util::PathExists(origin_file));

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

The pattern I'd suggest for things that you'd like to be assertions in helper
functions is:
a) Make the helper function return a bool for success/failure.
b) Test right after the expect for success/failure of whatever the expect tests,
and return false if it fails.
c) Assert in the calling code that the helper function returns true.

I don't think it's really important; primarily it's about reducing noise in test
output in the case of a failure.  But I think it's a good habit to get into, in
case one of the proto-assertions not being true later leads to a crash or
timeout, which are test failure types that we'd really like to avoid.

[ahendrickson, 2010/12/22 22:16:52]

Done.

 
     // Confirm the downloaded data file exists.
-    ASSERT_TRUE(file_util::PathExists(downloaded_file));
+    EXPECT_TRUE(file_util::PathExists(downloaded_file));
     int64 origin_file_size = 0;
     int64 downloaded_file_size = 0;
     EXPECT_TRUE(file_util::GetFileSize(origin_file, &origin_file_size));
@@ -279,59 +366,574 @@ class DownloadTest : public InProcessBrowserTest {
     EXPECT_TRUE(file_util::DieFileDie(downloaded_file, false));
   }
 
+  // TODO(ahendrickson) -- |expected_title_in_progress| and
+  // |expected_title_in_finished| need to be checked.

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Thanks!

+  void RunSizeTest(const GURL& url,
+                  const std::wstring& expected_title_in_progress,
+                  const std::wstring& expected_title_finished) {
+    InitialSetup(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(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.

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Nit: This looks to be a duplicate with the above comment.

[ahendrickson, 2010/12/22 22:16:52]

One says "before", and the other says "after".

+
+    // 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);
+    EXPECT_TRUE(file_util::PathExists(download_path));
+
+    // Delete the file we just downloaded.
+    EXPECT_TRUE(file_util::DieFileDie(download_path, true));
+    EXPECT_FALSE(file_util::PathExists(download_path));
+  }
+
  private:
   // Location of the test data.
   FilePath test_dir_;
 
   // Location of the downloads directory for these tests
   ScopedTempDir downloads_directory_;
+
+  std::stack<Browser *> browsers_;
 };
 
+// NOTES:
+//
+// Files for these tests are found in chrome\test\data\.
+// 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) {
+  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) {
+  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) {
+  InitialSetup(false);
+  FilePath file(FILE_PATH_LITERAL("download-test2.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  FilePath file_path = GetDownloadDirectory().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) {
+  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) {
+  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.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_UnknownSize) {

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

I think we've agreed to bring references to 63237 and FLAKY_/DISABLED_ into
alignment, i.e. if it refers to 63237 in all its naked glory, it's disabled, but
and if it doesn't (or does in a "but we shouldn't trip over it" fashion) it's
enabled/flaky (depending on what other bugs are referenced).  Could you either
execute that comment/category normalization or grab me and tell me why we
shouldn't?

(Another way to put this is that the 63237 boilerplate should include words like
"will cause a timeout and thus this test must be disabled" and that tests with
that unmodified boilerplate should just be disabled.  I know of no way that
63237 could just produce a failed test without a timeout.)

[ahendrickson, 2010/12/22 22:16:52]

Hmm, we could put in code that gets around the hang for 63237 by cleaning up the
specific files we use, right?

Is this better or worse than marking the tests DISABLED?

+  GURL url(URLRequestSlowDownloadJob::kUnknownSizeUrl);
+  FilePath filename;
+  net::FileURLToFilePath(url, &filename);
+  filename = filename.BaseName();
+  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.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_KnownSize) {
+  GURL url(URLRequestSlowDownloadJob::kKnownSizeUrl);
+  FilePath filename;
+  net::FileURLToFilePath(url, &filename);
+  filename = filename.BaseName();
+  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.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_IncognitoDownload) {
+  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.
+  PushBrowser(incognito);
+  CreateAndSetDownloadsDirectory();  // Must be called after PushBrowser().
+  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.
+  browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                               false);
+  DownloadAndWait(browser(), 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 = browser()->window()->IsDownloadShelfVisible();
+  EXPECT_TRUE(is_shelf_visible);
+
+  // Close the Incognito window and don't crash.
+  browser()->CloseWindow();
+  ui_test_utils::WaitForWindowClosed(browser());
+
+  PopBrowser();  // We're done with the incognito window.
+
+  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) {
+  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 in http://crbug.com/63237.

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Nit: The word "in" is doubled in the preceding sentence.

[ahendrickson, 2010/12/22 22:16:52]

Done.

+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_CloseNewTab1) {
+  InitialSetup(false);
+
+  // Download a file in a new background tab and wait.  The tab is automatically
+  // closed when the download is done.

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Is the tab automatically closed when the download is done, or when the system
realizes that it's a download?  I'm not sure that it's the latter, but I thought
it was.

[ahendrickson, 2010/12/22 22:16:52]

Hmm, actually I'm not sure.

+  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 test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_DontCloseNewTab2) {
+  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 test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_DontCloseNewTab3) {
+  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 test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_CloseNewTab2) {
+  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 is done.
+// Verify that we have 1 tab, and the download shelf is visible.
+//
+// The test sometimes trips over underlying flakiness in the downloads
+// subsystem in in http://crbug.com/63237.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_CloseNewTab3) {
+  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 is done.
+  // 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 browser window, leaving it open.
+// Verify that we have 2 windows, and the download shelf is not visible in the
+// first window, but is visible in the second 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.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_DontCloseNewWindow) {

[Randy Smith (Not in Mondays), 2010/12/19 23:52:49]

Is there a reason why you didn't respond to my earlier comment about the
relationship between this test and NewWindow?

[ahendrickson, 2010/12/22 22:16:52]

Sorry, that was an oversight.

You are correct that one is a superset of the other.

I'll remove the "Dont" version.

+  InitialSetup(false);
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // 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.
+  Browser* download_browser = ui_test_utils::GetBrowser(1);
+  ASSERT_TRUE(download_browser != NULL);
+  EXPECT_NE(download_browser, browser());
+  EXPECT_EQ(1, download_browser->tab_count());
+  EXPECT_TRUE(download_browser->window()->IsDownloadShelfVisible());
+
+  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.
+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, DISABLED_NewWindow) {
+  InitialSetup(false);
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+
+  // 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.
+  Browser* download_browser = ui_test_utils::GetBrowser(1);
+  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::WaitForWindowClosed(download_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