Re-enable DownloadsApiTest.Downloads

chrome/browser/download/download_extension_apitest.cc

Index: chrome/browser/download/download_extension_apitest.cc
diff --git a/chrome/browser/download/download_extension_apitest.cc b/chrome/browser/download/download_extension_apitest.cc
index 99f0cd03c4ae6fcbef35356f8e818cd1d73243cf..3684ad4716b02fc8bcb7ee557a50e2662c140ef7 100644
--- a/chrome/browser/download/download_extension_apitest.cc
+++ b/chrome/browser/download/download_extension_apitest.cc
@@ -2,12 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
+#include "content/public/browser/download_manager.h"
 
 class DownloadsApiTest : public ExtensionApiTest {
  public:
@@ -22,12 +25,39 @@ class DownloadsApiTest : public ExtensionApiTest {
         prefs::kDownloadDefaultDirectory, tmpdir.path());
   }
 
+  void CleanUpDownloads() {
+    // Clean up any remaining downloads so the browser can quit without
+    // prompting about in-progress downloads. Ideally this would be done in the
+    // extension itself, but currently we don't expose enough through the
+    // extension API to make this possible.
+    DownloadService* download_service = DownloadServiceFactory::GetForProfile(
+        browser()->profile());
+    ASSERT_TRUE(download_service != NULL);
+    if (!download_service->HasCreatedDownloadManager())
+      return;
+    content::DownloadManager* download_manager =
+        download_service->GetDownloadManager();
+    ASSERT_TRUE(download_manager != NULL);
+    if (download_manager->InProgressCount() == 0)
+      return;
+    typedef content::DownloadManager::DownloadVector DownloadVector;
+    DownloadVector all_downloads;
+    download_manager->GetAllDownloads(FilePath(), &all_downloads);

[asanka, 2012/01/09 17:22:17]

There's a race here where the download item might not be present in
history_downloads_ but is in_progress_.

Extension tests run into this because the downloads.download callback is issued
when we receive a response from the server for a newly initiated download, but
the download isn't added to history_downloads_ until name determination is
complete. Therefore some tests can complete before the download it creates is
visible to GetAllDownloads().

[Randy Smith (Not in Mondays), 2012/01/09 19:05:45]

Chris: Can you get ahold of the list of downloads in progress and wait until
they show up as visible before canceling them?

[cbentzel, 2012/01/09 19:16:41]

Thanks for the explanation of when this could happen.

It looks like I need to add another method to DownloadManager to correctly clear
all of these, unfortunately.

Now that "cancel" support is in, though, I may be able to do this in the
extension as Ben suggested. 

 

[cbentzel, 2012/01/09 19:16:41]

It doesn't look there is a way to get to this from the DownloadManager
interface. I could potentially create a new DownloadManagerDelegate class which
passes most calls through to the original Delegate, and keeps track of the id's
passed into ShouldStartDownload.

[Randy Smith (Not in Mondays), 2012/01/09 19:20:32]

Oh, goodness, please don't do that; an interface on DownloadManager is a much
cleaner solution.  (For context, this problem will go away when we do download
id part II, albeit with the creation of a couple other problems around making
sure that the download item presents a consistent state as it goes through the
file name determination pathway.)

What I meant was more "when the test creates a download, can you note the
download id associated with that download and then wait for all of those to
become visible and kill them?"  If the answer's no for some reason, another
interface on DownloadManager makes sense :-{.

+    for (DownloadVector::const_iterator it = all_downloads.begin();
+         it != all_downloads.end();
+         ++it) {
+      content::DownloadItem* download_item = *it;
+      download_manager->CancelDownload(download_item->GetId());
+    }
+  }
+
  private:
   ScopedTempDir tmpdir;
 };
 
-IN_PROC_BROWSER_TEST_F(DownloadsApiTest, DISABLED_Downloads) {
+IN_PROC_BROWSER_TEST_F(DownloadsApiTest, Downloads) {
   SetUpTempDownloadsDir();
   ASSERT_TRUE(StartTestServer());
   ASSERT_TRUE(RunExtensionTest("downloads")) << message_;
+  CleanUpDownloads();
 }