Fixed issue with DownloadTestObserver.

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 5874cce0183e983a3661edf6a6d6a4e237c14c6d..7474bad70a553de5f7dba29426fde51606f70741 100644
--- a/chrome/browser/download/download_browsertest.cc
+++ b/chrome/browser/download/download_browsertest.cc
@@ -389,45 +389,56 @@ class DownloadTest : public InProcessBrowserTest {
     return GetDownloadPrefs(browser)->download_path();
   }
 
-  // Create a DownloadTestObserver that will wait for the
+  // Create a DownloadTestObserverTerminal that will wait for the
   // specified number of downloads to finish.
   DownloadTestObserver* CreateWaiter(Browser* browser, int num_downloads) {
     DownloadManager* download_manager = DownloadManagerForBrowser(browser);
-    return new DownloadTestObserver(
+    return new DownloadTestObserverTerminal(
         download_manager, num_downloads,
-        DownloadItem::COMPLETE,  // Really done
         true,                   // Bail on select file
         DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
   }
 
-  // Create a DownloadTestObserver that will wait for the
+  // Create a DownloadTestObserverInProgress that will wait for the
   // specified number of downloads to start.
   DownloadTestObserver* CreateInProgressWaiter(Browser* browser,
                                             int num_downloads) {

[benjhayden, 2012/03/08 20:30:02]

pre-existing nit: indentation.

[ahendrickson, 2012/03/08 21:28:55]

Done.

     DownloadManager* download_manager = DownloadManagerForBrowser(browser);
-    return new DownloadTestObserver(
-        download_manager, num_downloads,
-        DownloadItem::IN_PROGRESS,      // Has started
-        true,                           // Bail on select file
-        DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+    return new DownloadTestObserverInProgress(
+        download_manager, num_downloads);
   }
 
-  // Create a DownloadTestObserver that will wait for the
+  // Create a DownloadTestObserverTerminal that will wait for the
   // specified number of downloads to finish, or for
   // a dangerous download warning to be shown.
   DownloadTestObserver* DangerousInstallWaiter(
       Browser* browser,
       int num_downloads,
-      DownloadItem::DownloadState final_state,
       DownloadTestObserver::DangerousDownloadAction dangerous_download_action) {
     DownloadManager* download_manager = DownloadManagerForBrowser(browser);
-    return new DownloadTestObserver(
+    return new DownloadTestObserverTerminal(
         download_manager, num_downloads,
-        final_state,
         true,                         // Bail on select file
         dangerous_download_action);
   }
 
+  void CheckDownloadStatesForBrowser(Browser* browser,
+                                     size_t num,
+                                     DownloadItem::DownloadState state) {
+    std::vector<DownloadItem*> download_items;
+    GetDownloads(browser, &download_items);
+
+    EXPECT_EQ(num, download_items.size());
+
+    for (size_t i = 0; i < download_items.size(); ++i) {
+      EXPECT_EQ(state, download_items[i]->GetState()) << " Item " << i;
+    }
+  }
+
+  void CheckDownloadStates(size_t num, DownloadItem::DownloadState state) {
+    CheckDownloadStatesForBrowser(browser(), num, state);
+  }
+
   // Download |url|, then wait for the download to finish.
   // |disposition| indicates where the navigation occurs (current tab, new
   // foreground tab, etc).
@@ -452,6 +463,7 @@ class DownloadTest : public InProcessBrowserTest {
                                                 browser_test_flags);
     // Waits for the download to complete.
     observer->WaitForFinished();
+    DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));

[Randy Smith (Not in Mondays), 2012/03/08 18:25:31]

I think of DCHECKs as not being good in test code, and from my perspective a lot
of your DCHECK_EQ() really should be EXPECT_EQ or ASSERT_EQ.  Could you change
them or help me understand why they're DCHECKs?

[ahendrickson, 2012/03/08 20:56:34]

Done.

 
     // If specified, check the state of the select file dialog.
     if (expectation != EXPECT_NOTHING) {
@@ -564,6 +576,8 @@ class DownloadTest : public InProcessBrowserTest {
         NEW_FOREGROUND_TAB,
         ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
     observer->WaitForFinished();
+    DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+    CheckDownloadStatesForBrowser(browser, 1, DownloadItem::COMPLETE);

[Randy Smith (Not in Mondays), 2012/03/08 18:25:31]

These seem redundant.  You're welcome to leave it like this if you want, but I
don't think it's needed.  (Applies throughout file; as suggested elsewhere, if
you nuke one of these I'd nuke the NumDownloadsSeenInState.)

[ahendrickson, 2012/03/08 20:56:34]

For this test, they are somewhat redundant.  In others, NumDownloadsSeenInState
is more reliable.

 
     EXPECT_EQ(2, browser->tab_count());
 
@@ -694,12 +708,9 @@ class DownloadTest : public InProcessBrowserTest {
 
     DownloadManager* download_manager = DownloadManagerForBrowser(browser());
     scoped_ptr<DownloadTestObserver> observer(
-        new DownloadTestObserver(
+        new DownloadTestObserverTerminal(
             download_manager,
             1,
-            download_info.reason == DOWNLOAD_INTERRUPT_REASON_NONE ?
-                DownloadItem::COMPLETE :  // Really done
-                DownloadItem::INTERRUPTED,
             true,                   // Bail on select file
             DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
 
@@ -719,8 +730,14 @@ class DownloadTest : public InProcessBrowserTest {
                                                                 1);
     }
 
-    if (download_info.show_download_item)
+    if (download_info.show_download_item) {
       observer->WaitForFinished();
+      DownloadItem::DownloadState final_state =
+          (download_info.reason == DOWNLOAD_INTERRUPT_REASON_NONE) ?
+              DownloadItem::COMPLETE :
+              DownloadItem::INTERRUPTED;
+      DCHECK_EQ(1u, observer->NumDownloadsSeenInState(final_state));
+    }
 
     // Validate that the correct file was downloaded.
     download_items.clear();
@@ -799,16 +816,17 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadMimeTypeSelect) {
   // Download the file and wait.  We expect the Select File dialog to appear
   // due to the MIME type, but we still wait until the download completes.
   scoped_ptr<DownloadTestObserver> observer(
-      new DownloadTestObserver(
+      new DownloadTestObserverTerminal(
           DownloadManagerForBrowser(browser()),
           1,
-          DownloadItem::COMPLETE,  // Really done
           false,                   // Continue on select file.
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   ui_test_utils::NavigateToURLWithDisposition(
       browser(), url, CURRENT_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
   EXPECT_TRUE(observer->select_file_dialog_seen());
 
   // Check state.
@@ -1329,6 +1347,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, MultiDownload) {
       NEW_FOREGROUND_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
   observer2->WaitForFinished();  // Wait for the third request.
+  DCHECK_EQ(1u, observer2->NumDownloadsSeenInState(DownloadItem::COMPLETE));
 
   // Get the important info from other threads and check it.
   scoped_refptr<CancelTestDataCollector> info(new CancelTestDataCollector());
@@ -1356,12 +1375,12 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadCancelled) {
   EXPECT_EQ(1, browser()->tab_count());
 
   // TODO(rdsmith): Fragile code warning!  The code below relies on the
-  // DownloadTestObserver only finishing when the new download has reached
-  // the state of being entered into the history and being user-visible
-  // (that's what's required for the Remove to be valid and for the
-  // download shelf to be visible).  By the pure semantics of
-  // DownloadTestObserver, that's not guaranteed; DownloadItems are created
-  // in the IN_PROGRESS state and made known to the DownloadManager
+  // DownloadTestObserverInProgress only finishing when the new download
+  // has reached the state of being entered into the history and being
+  // user-visible (that's what's required for the Remove to be valid and
+  // for the download shelf to be visible).  By the pure semantics of
+  // DownloadTestObserverInProgress, that's not guaranteed; DownloadItems
+  // are created in the IN_PROGRESS state and made known to the DownloadManager
   // immediately, so any ModelChanged event on the DownloadManager after
   // navigation would allow the observer to return.  However, the only
   // ModelChanged() event the code will currently fire is in
@@ -1507,6 +1526,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, AnchorDownloadTag) {
   scoped_ptr<DownloadTestObserver> observer(CreateWaiter(browser(), 1));
   ui_test_utils::NavigateToURL(browser(), url);
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
 
   // Confirm the downloaded data exists.
   FilePath downloaded_file = GetDownloadDirectory(browser());
@@ -1552,11 +1573,12 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxDenyInstall) {
 
   scoped_ptr<DownloadTestObserver> observer(
       DangerousInstallWaiter(
-          browser(), 1, DownloadItem::CANCELLED,
+          browser(), 1,
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_DENY));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::CANCELLED));
   EXPECT_EQ(1u, observer->NumDangerousDownloadsSeen());
 
   // Download shelf should close. Download panel stays open on ChromeOS.
@@ -1581,11 +1603,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInstallDenysPermissions) {
 
   scoped_ptr<DownloadTestObserver> observer(
       DangerousInstallWaiter(
-          browser(), 1, DownloadItem::COMPLETE,
+          browser(), 1,
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
   EXPECT_EQ(1u, observer->NumDangerousDownloadsSeen());
 
   // Download shelf should close. Download panel stays open on ChromeOS.
@@ -1610,11 +1634,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInstallAcceptPermissions) {
 
   scoped_ptr<DownloadTestObserver> observer(
       DangerousInstallWaiter(
-          browser(), 1, DownloadItem::COMPLETE,
+          browser(), 1,
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
   EXPECT_EQ(1u, observer->NumDangerousDownloadsSeen());
 
   // Download shelf should close. Download panel stays open on ChromeOS.
@@ -1640,11 +1666,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxInvalid) {
 
   scoped_ptr<DownloadTestObserver> observer(
       DangerousInstallWaiter(
-          browser(), 1, DownloadItem::COMPLETE,
+          browser(), 1,
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
 
   // Check that the extension was not installed.
   ExtensionService* extension_service =
@@ -1664,11 +1692,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, CrxLargeTheme) {
 
   scoped_ptr<DownloadTestObserver> observer(
       DangerousInstallWaiter(
-          browser(), 1, DownloadItem::COMPLETE,
+          browser(), 1,
           DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_ACCEPT));
   ui_test_utils::NavigateToURL(browser(), extension_url);
 
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
   EXPECT_EQ(1u, observer->NumDangerousDownloadsSeen());
 
   // Download shelf should close. Download panel stays open on ChromeOS.
@@ -1825,16 +1855,17 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrl) {
   ASSERT_TRUE(web_contents);
 
   DownloadTestObserver* observer(
-    new DownloadTestObserver(
-        DownloadManagerForBrowser(browser()), 1,
-        DownloadItem::COMPLETE,  // Really done
-        false,                   // Ignore select file.
-        DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
+      new DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
+          false,                   // Ignore select file.
+          DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   DownloadSaveInfo save_info;
   save_info.prompt_for_save_location = true;
   DownloadManagerForBrowser(browser())->DownloadUrl(
       url, GURL(""), "", false, -1, save_info, web_contents);
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
   EXPECT_TRUE(observer->select_file_dialog_seen());
 
   // Check state.
@@ -1862,6 +1893,7 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadUrlToPath) {
   DownloadManagerForBrowser(browser())->DownloadUrl(
       url, GURL(""), "", false, -1, save_info, web_contents);
   observer->WaitForFinished();
+  DCHECK_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
 
   // Check state.
   EXPECT_EQ(1, browser()->tab_count());
@@ -1894,11 +1926,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaGet) {
   // reachable.
   ASSERT_TRUE(test_server()->Stop());
   scoped_ptr<DownloadTestObserver> waiter(
-      new DownloadTestObserver(
-          DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE,
+      new DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
           false, DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   browser()->SavePage();
   waiter->WaitForFinished();
+  DCHECK_EQ(1u, waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
 
   // Validate that the correct file was downloaded.
   GetDownloads(browser(), &download_items);
@@ -1908,8 +1942,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaGet) {
 
   // Try to download it via a context menu.
   scoped_ptr<DownloadTestObserver> waiter_context_menu(
-      new DownloadTestObserver(
-          DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE,
+      new DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
           false, DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   content::ContextMenuParams context_menu_params;
   context_menu_params.media_type = WebKit::WebContextMenuData::MediaTypeImage;
@@ -1920,6 +1954,9 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaGet) {
   menu.Init();
   menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVEIMAGEAS);
   waiter_context_menu->WaitForFinished();
+  DCHECK_EQ(
+      1u, waiter_context_menu->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(2, DownloadItem::COMPLETE);
 
   // Validate that the correct file was downloaded via the context menu.
   download_items.clear();
@@ -1969,11 +2006,13 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaPost) {
   // rather than "POST".
   ASSERT_TRUE(test_server()->Stop());
   scoped_ptr<DownloadTestObserver> waiter(
-      new DownloadTestObserver(
-          DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE,
+      new DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
           false, DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   browser()->SavePage();
   waiter->WaitForFinished();
+  DCHECK_EQ(1u, waiter->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(1, DownloadItem::COMPLETE);
 
   // Validate that the correct file was downloaded.
   GetDownloads(browser(), &download_items);
@@ -1983,8 +2022,8 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaPost) {
 
   // Try to download it via a context menu.
   scoped_ptr<DownloadTestObserver> waiter_context_menu(
-      new DownloadTestObserver(
-          DownloadManagerForBrowser(browser()), 1, DownloadItem::COMPLETE,
+      new DownloadTestObserverTerminal(
+          DownloadManagerForBrowser(browser()), 1,
           false, DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL));
   content::ContextMenuParams context_menu_params;
   context_menu_params.media_type = WebKit::WebContextMenuData::MediaTypeImage;
@@ -1994,6 +2033,9 @@ IN_PROC_BROWSER_TEST_F(DownloadTest, SavePageNonHTMLViaPost) {
   menu.Init();
   menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVEIMAGEAS);
   waiter_context_menu->WaitForFinished();
+  DCHECK_EQ(
+      1u, waiter_context_menu->NumDownloadsSeenInState(DownloadItem::COMPLETE));
+  CheckDownloadStates(2, DownloadItem::COMPLETE);
 
   // Validate that the correct file was downloaded via the context menu.
   download_items.clear();