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 fde56b5c19ac8ae0147f6bae8e407ba22e620e78..8c904771b6b781b7a0d9dbadf31ee47b27dcdf3a 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -2,26 +2,34 @@
 // 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/tab_contents/tab_contents.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 {
@@ -215,7 +223,40 @@ 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 force_no_prompt) {
+    // Sanity check default values for window / tab count and shelf visibility.
+    PushBrowser(InProcessBrowserTest::browser());  // Set the initial browser.
+    int window_count = BrowserList::size();
+    ASSERT_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.
+    ASSERT_TRUE(CreateAndSetDownloadsDirectory());

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

My understanding is that we can't use ASSERT_* inside of helper functions as per
our discussion.  So this may need to be rewritten--I'll wait on your talking
with Chris.

[ahendrickson, 2010/12/16 19:26:41]

I removed most of the ASSERT's, including all that were in helper functions.

+
+    if (force_no_prompt) {
+      browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                                   false);

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Is there ever a case where we *don't* want to specify whether we prompt for
download or not?  DisableMIMETypeSelect wants us always to prompt; are there any
other cases where we want to allow prompting at all?  If not, I'd make
force_on_prompt into just the argument provided to the SetBoolean; that allows
you to remove all uses of kPromptForDownload in all tests and just replace it
with the correct arg to InitialSetup().

[ahendrickson, 2010/12/16 19:26:41]

There is still one instance where it has to occur in the tests themselves -- in
the case of the Incognito download.

Done.

+    }
+  }
+
  protected:
+
+  // We now use a browser stack.
+  void PushBrowser(Browser* browser) { browsers_.push(browser); }
+  void PopBrowser() { browsers_.pop(); }

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

If you take my suggestion below about giving CreateAndSetDownloadsDirectory a
Browser* argument, I think the need for these calls goes away, which I think is
good, as people used to what they mean in other browser tests will be confused
by the difference here.

[ahendrickson, 2010/12/16 19:26:41]

It's also needed for |CreateWaiter()|, which is called via |DownloadAndWait()|. 
I expect this to be used more when we have other tests.

I think of |browser()| as returning the current browser/window that we're
working with.  When we open a new window to download into, that changes.

+
+  virtual Browser* browser() const { return browsers_.top(); }

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I'd suggest browsers_.empty() -> return NULL;

[ahendrickson, 2010/12/16 19:26:41]

Done.

+
   void SetUpInProcessBrowserTestFixture() {
     ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &test_dir_));
   }
@@ -223,11 +264,17 @@ class DownloadTest : public InProcessBrowserTest {
   // Must be called after browser creation.  Creates a temporary
   // directory for downloads that is auto-deleted on destruction.
   bool CreateAndSetDownloadsDirectory() {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

The two usages of this function are in InitialSetup() and in IncognitoDownload. 
In both cases we've just before the call been playing with the browser pointer
we want for the call, and in IncognitoDownload we're doing a PushBrowser() just
before to make sure that this call has the right browser (in a way that, to me,
feels a little indirect ans susceptible to future error; you need to understand
the innards of both CreateAndSetDownloadsDirectory() and PushBrowser() at the
point of call in IncognitoDownload).  I'd recommend just making this call take a
browser argument. 

[ahendrickson, 2010/12/16 19:26:41]

I'd have to make other functions take a browser argument too, and pass it
around.

-    if (downloads_directory_.CreateUniqueTempDir()) {
-      browser()->profile()->GetPrefs()->SetFilePath(
+    if (downloads_directory_.CreateUniqueTempDir())
+      return SetDownloadsDirectoryForBrowser(browser());
+    return false;
+  }
+
+  bool SetDownloadsDirectoryForBrowser(Browser* browser) {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

The only use I can find of SetDownloadsDirectoryForBrowser is above, in
CreateAndSetDownloadsDirectory.  If that's it, I think these two routines should
be collapsed into one, at least until we need the separate functionality.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+    if (browser) {
+      browser->profile()->GetPrefs()->SetFilePath(
           prefs::kDownloadDefaultDirectory,
           downloads_directory_.path());
-      return true;
+       return true;
     }
     return false;
   }
@@ -246,7 +293,44 @@ 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 naiation occurs (current tab, new

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Nit: "navigation"

[ahendrickson, 2010/12/16 19:26:41]

Done.

+  // foreground tab, etc).
+  // |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,

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

|expectation| isn't mentioned in the comments for the routine and should be. 
The comment for that arg should mention that this routine will always unblock if
the select file dialog is brought up; the only effect of |expectation| is
determining whether the test passes/fails/etc.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+                                      int browser_test_flags) {
+    // Setup notification, navigate, and block.
+    scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
+    ui_test_utils::NavigateToURLWithDisposition(browser,
+                                                url,
+                                                disposition,
+                                                browser_test_flags);
+    observer->WaitForFinished();

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I'd make clear in comments that we're blocking for the navigation in
NavigateToURLWithDisposition and blocking for the download completion in
WaitForFinished().  (It's fine to sketch that all out in the one comment above.)

[ahendrickson, 2010/12/16 19:26:41]

Done.

+
+    // 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
@@ -280,59 +364,569 @@ class DownloadTest : public InProcessBrowserTest {
     EXPECT_TRUE(file_util::DieFileDie(downloaded_file, false));
   }
 
+  // Note: |expected_title_in_progress| and |expected_title_in_finished| need
+  // to be checked.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I think the usual format for this is "TODO(<your ldap>): ..." to indicate that
this is something that isn't being done and should be in the future.  The above
comment isn't clear as to whether it's describing a requirement on the test that
it satisfies, or a todo for the future.

An alternative would be "Note that |expected_title_in_progress| and
|expected_title_finished| are currently ignored; see TODO below."

+  void RunSizeTest(const GURL& url,
+                  const std::wstring& expected_title_in_progress,
+                  const std::wstring& expected_title_finished) {
+    InitialSetup(true);
+
+    // 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(tc): check download status text before downloading.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I think since the text is primarily yours (?), the name should be yours as well.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+    // 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(tc): 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);
+    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) {
-  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
-  ASSERT_TRUE(CreateAndSetDownloadsDirectory());
+  InitialSetup(false);
   browser()->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload, true);
+  FilePath file(FILE_PATH_LITERAL("download-test1.lib"));
+  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());
+}
+
+// Access a file with a viewable mime-type, verify that a download
+// did not initiate.
+IN_PROC_BROWSER_TEST_F(DownloadTest, NoDownload) {
+  InitialSetup(true);
+  FilePath file(FILE_PATH_LITERAL("download-test2.html"));
+  GURL url(URLRequestMockHTTPJob::GetMockUrl(file));
+  FilePath file_path = GetDownloadDirectory().Append(file);
 
-  // Setup notification, navigate, and block.
-  scoped_ptr<DownloadsObserver> observer(CreateWaiter(1));
-  ui_test_utils::NavigateToURL(
-      browser(), URLRequestMockHTTPJob::GetMockUrl(file));
-  observer->WaitForFinished();
+  // Open a web page and wait.
+  ui_test_utils::NavigateToURL(browser(), url);
 
-  // Download should not be finished; check state.
-  EXPECT_TRUE(observer->select_file_dialog_seen());
+  // Check that we did not download the web page.
+  EXPECT_FALSE(file_util::PathExists(file_path));
+  if (file_util::PathExists(file_path))
+    ASSERT_TRUE(file_util::DieFileDie(file_path, false));
+
+  // 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(true);
+  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 is considered flaky due to bug http://crbug.com/26325.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Obviously, remove references to 26325.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+// 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(true);
+  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.
+  ASSERT_NE(static_cast<TabContentsWrapper *>(NULL),
+            browser()->AddSelectedTabWithURL(GURL(), PageTransition::TYPED));
+  EXPECT_EQ(2, browser()->tab_count());

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

It would seem natural to this test to check that the shelf is visible here.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+
+  // Hide the download shelf.
+  browser()->window()->GetDownloadShelf()->Close();
   EXPECT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  // Go to the first tab.
+  browser()->SelectTabContentsAt(0, true);
+  ASSERT_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.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I'd add "These tests don't currently test much due to holes in RunSizeTest; see
comments in that routine for details."

[ahendrickson, 2010/12/16 19:26:41]

Done.

+
+// 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, FLAKY_UnknownSize) {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

For what it's worth, I believe these tests won't normally  trip over 63237 (the
slow download will generally allow the history system to catch up), which may be
why they're marked flaky rather than disabled.  I think it's still possible that
sometimes the history system will be wicked slow, and we'll still time out.  But
these tests should usually work properly.  I.e. I don't think FLAKY_ is right;
I'd still vote for DISABLED_ (since I think a timeout *could* happen) but we
could also enable these.

Obviously, we want to watch the flakiness dashboard after committing unless we
check them in DISABLED_.

[ahendrickson, 2010/12/16 19:26:41]

I'm going to enable them, then.

+  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
+// 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, 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();
+  ASSERT_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));
+  incognito->profile()->GetPrefs()->SetBoolean(prefs::kPromptForDownload,
+                                               false);
+  DownloadAndWait(incognito, url, EXPECT_NO_SELECT_DIALOG);
+  PopBrowser();

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I'd call this below, near CloseWindow(), just to keep reality and the browser
list in alignment.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+
+  // We should still have 2 windows.
+  window_count = BrowserList::size();
+  ASSERT_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::WaitForWindowClosed(incognito);
+  window_count = BrowserList::size();
+  ASSERT_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);
+}
+

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Not a requirement for this CL, but I invite you to think about slightly more
descriptive suffixes than the [123] we've got below and to change the names of
the tests if you do.  The suffixes are confusing, especially because we're often
dealing with some number of tabs, which sometimes match the suffix and sometimes
don't :-J.

+// Navigate to a new background page, but don't download.  Confirm that the
+// download shelf is not visible and that we have two tabs.
+//
+// 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, FLAKY_DontCloseNewTab1) {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I can't seen any reason why this test should be flaky--it doesn't download.  If
it is flaky, that's worth knowing.  I'd check it in enabled and watch the
flakiness dashboard.

[ahendrickson, 2010/12/16 19:26:41]

Done.

+  InitialSetup(false);
+  // Because it's an HTML link, it should open a tab rather than downloading.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

This comment reads weirdly in combination with the NEW_BACKGROUND_TAB below. 
The comment implies that there's code deep in the guts of chromium that is being
tested that checks the nature of the URL, and opens a new tab because it's an
HTML link.  But the NEW_BACKGROUND_TAB below implies to me "Open a new tab
unilaterally".  If I look at the original UI test, it didn't have this comment;
I'd remove it.

[ahendrickson, 2010/12/16 19:26:41]

Changed the comments, here and elsewhere.

+  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.
+  ASSERT_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/13 21:44:58]

nit: "in in"

[ahendrickson, 2010/12/16 19:26:41]

I don't understand.

+// Additionally, there is Windows-specific flake, http://crbug.com/20809.
+IN_PROC_BROWSER_TEST_F(DownloadTest, FLAKY_CloseNewTab1) {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Can't imagine why this wouldn't timeout via 62637.  Remember that the
FLAKY/DISABLED settings were colliding with the transition Pawel made to turn
the bots red on timeouts for flaky tests, so tests that were only occasionally
failing might have been marked FLAKY.  

But this all pretty much comes down to "we're going to need to be watching the
flakiness dashboard for these after commit" :-J :-J.

+  InitialSetup(true);
+
+  // Download a file in a new background tab and wait.  The tab is automatically
+  // closed when the download is done.
+  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.
+  ASSERT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  ASSERT_EQ(1, browser()->tab_count());
+
+  CheckDownload(file, file);
+}
+
+// Open a web page, then download a file in another tab via a Javascript call.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Elsewhere current tab, foreground tab, background tab is specified along with
"open a web page"; I'd suggest doing that here as well (possibly specifying that
that decision is made automatically by the system here).

Also, a later comments specifically calls out that the javascript opens a
temporary tab, so that should probably be done here for consistency (and because
it's relevant to the main point of the test, which is how many tabs are left).

[ahendrickson, 2010/12/16 19:26:41]

Added "current tab", but no temporary tab is created.

+// 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(true);
+  // Because it's an HTML link, it should open a tab 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.
+  ASSERT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  ASSERT_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.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

I'd suggest that in general, when the test relies on the "temporary tab" nature
of javascript:openNew() that that be called out.

[ahendrickson, 2010/12/16 19:26:41]

The |openNew()| function is embedded in the web page, and varies.  It doesn't
always open a temporary 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(true);
+  // Because it's an HTML link, it should open a tab 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);
+
+  ASSERT_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.
+  ASSERT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  ASSERT_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(true);
+  // Because it's an HTML link, it should open a tab 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.
+  ASSERT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  ASSERT_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. Close the new tab when the download is done.

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Is the new tab closed by the test?  I don't see the code to do so below--is it
in the javascript associated with the form on the download page?

(Mostly me trying to understand the comment, but it wouldn't hurt to answer the
question in the comment as well, for the next person trying to understand.)

[ahendrickson, 2010/12/16 19:26:41]

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(true);
+  // Because it's an HTML link, it should open a tab 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.
+  ASSERT_TRUE(browser()->window()->IsDownloadShelfVisible());
+  ASSERT_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) {
+  InitialSetup(true);
+  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();
+  ASSERT_EQ(2, window_count);
+  ASSERT_EQ(1, browser()->tab_count());
+  ASSERT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  // The download shelf SHOULD be visible in the second window.
+  Browser* download_browser = ui_test_utils::GetBrowser(1);

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Does GetBrowser() give a guarantee that we get the windows in order of creation?
 If not, I think getting the list and removing the known browser from it is the
right thing.

[ahendrickson, 2010/12/16 19:26:41]

In this particular case, it's guaranteed.  If you open and close browsers
randomly, it won't be.

+  ASSERT_NE(download_browser, browser());
+  ASSERT_EQ(1, download_browser->tab_count());
+  ASSERT_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) {

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

This test looks like a strict superset of DontCloseNewWindow.  If you agree with
that analysis, I'd nuke DontCloseNewWindow.

+  InitialSetup(true);
+  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);

[Randy Smith (Not in Mondays), 2010/12/13 21:44:58]

Micro-nit: Consistency is a win, so in the absence of hitting the 80 column
limit I'd format these lines the same (either starting on the same line as the
call or starting on the next line) throughout this file.  But saying I don't
care a lot is a bit of an overstatement; this is more making you aware of the
possibility of changing it than an actual request :-}.

[ahendrickson, 2010/12/16 19:26:41]

I personally prefer this method of lining up variables; I find it easier to
read.

I only go to the 'indent 4 spaces' method when i run into the 80-character
limit.

+
+  // When the download finishes, the download shelf SHOULD NOT be visible in
+  // the first window.
+  int window_count = BrowserList::size();
+  ASSERT_EQ(2, window_count);
+  ASSERT_EQ(1, browser()->tab_count());
+  ASSERT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  // The download shelf SHOULD be visible in the second window.
+  Browser* download_browser = ui_test_utils::GetBrowser(1);
+  ASSERT_NE(download_browser, browser());
+  ASSERT_EQ(1, download_browser->tab_count());
+  ASSERT_TRUE(download_browser->window()->IsDownloadShelfVisible());
+
+  // Close the new window.
+  download_browser->CloseWindow();
+  ui_test_utils::WaitForWindowClosed(download_browser);
+  window_count = BrowserList::size();
+  ASSERT_EQ(1, window_count);
+  ASSERT_EQ(1, browser()->tab_count());
+  // The download shelf should not be visible in the remaining window.
+  ASSERT_FALSE(browser()->window()->IsDownloadShelfVisible());
+
+  CheckDownload(file, file);
 }
 
 }  // namespace