Fix DownloadExtensionTest_OnDeterminingFilename_InterruptedResume

chrome/browser/extensions/api/downloads/downloads_api_unittest.cc

Index: chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
index 3ec086412b6585302b21eebd3150869a20e3badf..eecbbf888fb2d4eeecf6af2f60addd0272e6c89e 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
@@ -36,9 +36,9 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/page_transition_types.h"
 #include "content/public/test/download_test_observer.h"
-#include "content/public/test/test_file_error_injector.h"
 #include "content/test/net/url_request_slow_download_job.h"
 #include "net/base/data_url.h"
 #include "net/base/net_util.h"
@@ -86,6 +86,11 @@ class DownloadsEventsListener : public content::NotificationObserver {
     STLDeleteElements(&events_);
   }
 
+  void ClearEvents() {
+    STLDeleteElements(&events_);
+    events_.clear();
+  }
+
   class Event {
    public:
     Event(Profile* profile,
@@ -335,6 +340,10 @@ class DownloadExtensionTest : public ExtensionApiTest {
                             expected_error));
   }
 
+  void ClearEvents() {
+    events_listener_->ClearEvents();
+  }
+
   std::string GetExtensionURL() {
     return extension_->url().spec();
   }
@@ -3096,108 +3105,142 @@ IN_PROC_BROWSER_TEST_F(
                          result_id)));
 }
 
-// Test download interruption while extensions determining filename, re-run
-// through fan-out and fan-in.
-// TODO(rdsmith): FILE_OPERATION_INITIALIZE is not right for this test.
+class JustInProgressDownloadObserver
+    : public content::DownloadTestObserverInProgress {

[Randy Smith (Not in Mondays), 2013/04/02 21:52:34]

It probably won't surprise you to hear that I don't like duplicate classes that
have *almost* the same functionality.  Have you checked to see what breaks if
you remove the filename check from DownloadTestObserverInProgress?  

[benjhayden, 2013/04/03 15:43:00]

Even if the tests all pass, wouldn't that cause races and flaky tests?
Howbout a TODO to make DownloadUpdatedObserver more powerful and merge this with
that?

[Randy Smith (Not in Mondays), 2013/04/03 15:56:47]

Good point; you'd have to check the tests that use it individually to see if it
was safe.  But my larger point is that that extra test may be obsolete and you
could spend a bit of time seeing if it's safe to get rid of it.  And my even
larger point is that it's worth a bit of time trying to find *some* way to avoid
adding YATO that's really similar to ones we already have.  If there's no easy
way to do that, I'll accept the new class.  But I do think it's worth some time
trying to confirm there's no easy way to do that.

+ public:
+  JustInProgressDownloadObserver(
+      DownloadManager* download_manager, size_t wait_count)
+      : content::DownloadTestObserverInProgress(download_manager, wait_count) {
+  }
+
+  virtual ~JustInProgressDownloadObserver() {}
+
+ private:
+  virtual bool IsDownloadInFinalState(DownloadItem* item) OVERRIDE {
+    return item->GetState() == DownloadItem::IN_PROGRESS;
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(JustInProgressDownloadObserver);
+};
+
+// Test download interruption while extensions determining filename. Should not
+// re-dispatch onDeterminingFilename.
 IN_PROC_BROWSER_TEST_F(
     DownloadExtensionTest,
-    DISABLED_DownloadExtensionTest_OnDeterminingFilename_InterruptedResume) {
+    DownloadExtensionTest_OnDeterminingFilename_InterruptedResume) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableDownloadResumption);
   LoadExtension("downloads_split");
   CHECK(StartTestServer());
-  std::string download_url = test_server()->GetURL("slow?0").spec();
   GoOnTheRecord();
   AddFilenameDeterminer();
 
-  // TODO Interrupt the download instead of responding to onDeterminingFilename.
-  scoped_refptr<content::TestFileErrorInjector> injector(
-      content::TestFileErrorInjector::Create(
-          GetCurrentManager()));
-  content::TestFileErrorInjector::FileErrorInfo error_info = {
-      download_url,
-      content::TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
-      0,
-      content::DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE};
-  injector->AddError(error_info);
-  injector->InjectErrors();
-
   // Start a download.
-  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
-      new DownloadsDownloadFunction(), base::StringPrintf(
-          "[{\"url\": \"%s\"}]", download_url.c_str())));
-  ASSERT_TRUE(result.get());
-  int result_id = -1;
-  ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
-  ASSERT_TRUE(item);
+  DownloadItem* item = NULL;
+  {
+    DownloadManager* manager = GetCurrentManager();
+    scoped_ptr<content::DownloadTestObserver> observer(
+        new JustInProgressDownloadObserver(manager, 1));
+    ASSERT_EQ(0, manager->InProgressCount());
+    ui_test_utils::NavigateToURLWithDisposition(
+        current_browser(),
+        GURL(URLRequestSlowDownloadJob::kUnknownSizeUrl),
+        CURRENT_TAB,

[Randy Smith (Not in Mondays), 2013/04/02 21:52:34]

Please comment this innocuous looking enum :-}.

[benjhayden, 2013/04/03 15:43:00]

Done.

+        ui_test_utils::BROWSER_TEST_NONE);
+    observer->WaitForFinished();
+    EXPECT_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::IN_PROGRESS));
+    DownloadManager::DownloadVector items;
+    manager->GetAllDownloads(&items);
+    for (DownloadManager::DownloadVector::iterator iter = items.begin();
+          iter != items.end(); ++iter) {
+      if ((*iter)->GetState() == DownloadItem::IN_PROGRESS) {
+        // There should be only one IN_PROGRESS item.
+        EXPECT_EQ(NULL, item);
+        item = *iter;
+      }
+    }
+    ASSERT_TRUE(item);
+  }
   ScopedCancellingItem canceller(item);
-  ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
 
   // Wait for the onCreated and onDeterminingFilename event.
   ASSERT_TRUE(WaitFor(events::kOnDownloadCreated,
       base::StringPrintf("[{\"danger\": \"safe\","
-                          "  \"incognito\": false,"
-                          "  \"id\": %d,"
-                          "  \"mime\": \"text/plain\","
-                          "  \"paused\": false,"
-                          "  \"url\": \"%s\"}]",
-                          result_id,
-                          download_url.c_str())));
+                         "  \"incognito\": false,"
+                         "  \"id\": %d,"
+                         "  \"mime\": \"application/octet-stream\","
+                         "  \"paused\": false}]",
+                         item->GetId())));
   ASSERT_TRUE(WaitFor(
       events::kOnDownloadDeterminingFilename,
       base::StringPrintf("[{\"id\": %d,"
                          "  \"incognito\": false,"
-                         "  \"filename\":\"slow.txt\"}]",
-                         result_id)));
+                         "  \"filename\":\"download-unknown-size\"}]",
+                         item->GetId())));
   ASSERT_TRUE(item->GetTargetFilePath().empty());
   ASSERT_TRUE(item->IsInProgress());
 
-  ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
-      base::StringPrintf("[{\"id\": %d,"
-                          "  \"state\": {"
-                          "    \"previous\": \"in_progress\","
-                          "    \"current\": \"interrupted\"}}]",
-                          result_id)));
-  ASSERT_TRUE(item->IsInterrupted());
-  item->ResumeInterruptedDownload();
+  ClearEvents();
+  ui_test_utils::NavigateToURLWithDisposition(
+      current_browser(),
+      GURL(URLRequestSlowDownloadJob::kErrorDownloadUrl),
+      NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
 
-  // Wait for and respond to the onDeterminingFilename event.
-  ASSERT_TRUE(WaitFor(
-      events::kOnDownloadDeterminingFilename,
-      base::StringPrintf("[{\"id\": %d,"
-                         "  \"incognito\": false,"
-                         "  \"filename\":\"slow.txt\"}]",
-                         result_id)));
-  ASSERT_TRUE(item->GetTargetFilePath().empty());
-  ASSERT_TRUE(item->IsInProgress());
+  // Errors caught before filename determination are delayed until after
+  // filename determination.
   std::string error;
   ASSERT_TRUE(ExtensionDownloadsEventRouter::DetermineFilename(
       current_browser()->profile(),
       false,
       GetExtensionId(),
-      result_id,
+      item->GetId(),
       base::FilePath(FILE_PATH_LITERAL("42.txt")),
       false,
-      &error));
+      &error)) << error;
   EXPECT_EQ("", error);
 
-  // The download should complete successfully.
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
-                          "  \"filename\": {"
-                          "    \"previous\": \"%s\","
-                          "    \"current\": \"%s\"},"
-                          "  \"state\": {"
-                          "    \"previous\": \"in_progress\","
-                          "    \"current\": \"complete\"}}]",
-                          result_id,
-                          GetFilename("42.txt.crdownload").c_str(),
-                          GetFilename("42.txt").c_str())));
+                         "  \"error\":{\"current\":20},"
+                         "  \"state\":{"
+                         "    \"previous\":\"in_progress\","
+                         "    \"current\":\"interrupted\"}}]",
+                         item->GetId())));
+  EXPECT_EQ(item->GetState(), DownloadItem::INTERRUPTED);
+  ASSERT_TRUE(item->IsInterrupted());
+
+  ClearEvents();
+  item->ResumeInterruptedDownload();
+
+  // Errors caught before filename determination is complete are delayed until
+  // after filename determination so that, on resumption, filename determination
+  // does not need to be re-done. So, there will not be a second
+  // onDeterminingFilename event.
+
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
+                         "  \"error\":{\"previous\":20},"
+                         "  \"state\":{"
+                         "    \"previous\":\"interrupted\","
+                         "    \"current\":\"in_progress\"}}]",
+                         item->GetId())));
+
+  ClearEvents();
+  FinishPendingSlowDownloads();
+
+  // The download should complete successfully.
+  ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                         "  \"filename\": {"
+                         "    \"previous\": \"%s\","
+                         "    \"current\": \"%s\"},"
                          "  \"state\": {"
                          "    \"previous\": \"in_progress\","
                          "    \"current\": \"complete\"}}]",
-                         result_id)));
+                         item->GetId(),
+                         GetFilename("42.txt.crdownload").c_str(),
+                         GetFilename("42.txt").c_str())));
 }
 
 // TODO(benjhayden) Figure out why DisableExtension() does not fire