Fix DownloadExtensionTest_OnDeterminingFilename_InterruptedResume

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/json/json_reader.h"
 #include "base/message_loop.h"
@@ skipping 18 matching lines @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager.h"
 #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"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_job_factory.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "webkit/blob/blob_data.h"
 #include "webkit/blob/blob_storage_controller.h"
@@ skipping 27 matching lines @@
     registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT,
                    content::NotificationService::AllSources());
   }
 
   virtual ~DownloadsEventsListener() {
     registrar_.Remove(this, chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT,
                       content::NotificationService::AllSources());
     STLDeleteElements(&events_);
   }
 
+  void ClearEvents() {
+    STLDeleteElements(&events_);
+    events_.clear();
+  }
+
   class Event {
    public:
     Event(Profile* profile,
           const std::string& event_name,
           const std::string& json_args,
           base::Time caught)
       : profile_(profile),
         event_name_(event_name),
         json_args_(json_args),
         args_(base::JSONReader::Read(json_args)),
@@ skipping 229 matching lines @@
     return WaitFor(events::kOnDownloadChanged,
         base::StringPrintf("[{\"id\": %d,"
                             "  \"error\": {\"current\": %d},"
                             "  \"state\": {"
                             "    \"previous\": \"in_progress\","
                             "    \"current\": \"interrupted\"}}]",
                             item->GetId(),
                             expected_error));
   }
 
+  void ClearEvents() {
+    events_listener_->ClearEvents();
+  }
+
   std::string GetExtensionURL() {
     return extension_->url().spec();
   }
   std::string GetExtensionId() {
     return extension_->id();
   }
 
   std::string GetFilename(const char* path) {
     std::string result =
       downloads_directory_.path().AppendASCII(path).AsUTF8Unsafe();
@@ skipping 2741 matching lines @@
                           GetFilename("42 (1).txt.crdownload").c_str(),
                           GetFilename("42 (1).txt").c_str())));
   ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
       base::StringPrintf("[{\"id\": %d,"
                          "  \"state\": {"
                          "    \"previous\": \"in_progress\","
                          "    \"current\": \"complete\"}}]",
                          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);
 
+  ASSERT_TRUE(WaitFor(events::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                         "  \"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,
-                          GetFilename("42.txt.crdownload").c_str(),
-                          GetFilename("42.txt").c_str())));
-  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
 // OnListenerRemoved.