Rewrite DownloadsApiTest in C++.

chrome/browser/download/download_extension_test.cc

Index: chrome/browser/download/download_extension_test.cc
diff --git a/chrome/browser/download/download_extension_test.cc b/chrome/browser/download/download_extension_test.cc
index fb6ca46b8999e666e353538e30400c8ebeb6eec3..1de53aa2c8fcda0d48a0bdf2ac01084ea2765183 100644
--- a/chrome/browser/download/download_extension_test.cc
+++ b/chrome/browser/download/download_extension_test.cc
@@ -5,28 +5,43 @@
 #include <algorithm>
 
 #include "base/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
+#include "base/message_loop.h"
 #include "base/scoped_temp_dir.h"
+#include "base/stl_util.h"
 #include "base/stringprintf.h"
 #include "chrome/browser/download/download_extension_api.h"
 #include "chrome/browser/download/download_file_icon_extractor.h"
 #include "chrome/browser/download/download_service.h"
 #include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_test_observer.h"
+#include "chrome/browser/extensions/extension_apitest.h"
+#include "chrome/browser/extensions/extension_event_names.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/net/url_request_mock_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tab_contents/tab_contents.h"
+#include "chrome/common/chrome_notification_types.h"
 #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/download_persistent_store_info.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/common/page_transition_types.h"
 #include "content/test/net/url_request_slow_download_job.h"
 #include "net/base/data_url.h"
 #include "net/base/net_util.h"
 #include "ui/gfx/codec/png_codec.h"
+#include "webkit/fileapi/file_system_context.h"
+#include "webkit/fileapi/file_system_operation_interface.h"
 
 using content::BrowserContext;
 using content::BrowserThread;
@@ -44,7 +59,201 @@ struct DownloadIdComparator {
   }
 };
 
-class DownloadExtensionTest : public InProcessBrowserTest {
+class DownloadsEventsListener : public content::NotificationObserver {
+ public:
+  // WaitFor() gives up after this many milliseconds, figuring that the event it
+  // wants will never happen so that a failing test doesn't take too long to
+  // fail. This may be too short on very busy machines, or too long on very fast
+  // ones.
+  static const int kTimeOutMs = -1;

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

The comment and the value (which I take to mean "infinite") don't seem like
they're in alignment?

[benjhayden, 2012/06/19 15:01:59]

Done.

+
+  DownloadsEventsListener()
+    : waiting_(false) {
+    registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT,
+                   content::NotificationService::AllSources());
+  }
+  virtual ~DownloadsEventsListener() {
+    STLDeleteElements(&events_);

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Shouldn't this have a registrar_.Remove()?

[benjhayden, 2012/06/19 15:01:59]

Done.

+  }
+
+  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)),

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Is there some location you can point to for the format of these args?  It's hard
to verify the below code without knowing what the args are supposed to look
like.

[benjhayden, 2012/06/19 15:01:59]

The structure of args_ depends on event_name. See the DispatchEvent() calls in
ExtensionDownloadsEventRouter, or {experimental_,}downloads.json.

+        caught_(caught) {
+    }
+
+    const base::Time& caught() { return caught_; }
+
+    bool Equals(const Event& other) {
+      if ((profile_ != other.profile_) ||
+          (event_name_ != other.event_name_))
+        return false;
+      if ((event_name_ == extension_event_names::kOnDownloadCreated ||
+           event_name_ == extension_event_names::kOnDownloadChanged) &&
+          args_.get() &&
+          other.args_.get()) {
+        base::ListValue* left_list = NULL;
+        base::DictionaryValue* left_dict = NULL;
+        base::ListValue* right_list = NULL;
+        base::DictionaryValue* right_dict = NULL;
+        if (!args_->GetAsList(&left_list) ||
+            !other.args_->GetAsList(&right_list) ||
+            !left_list->GetDictionary(0, &left_dict) ||
+            !right_list->GetDictionary(0, &right_dict))

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Does this mean the Equals function returns false if either of the lists are
empty?  (This may be just my confusion as to args format; see above.)

[benjhayden, 2012/06/19 15:01:59]

For onCreated and onChanged events, yes.

+          return false;
+        for (base::DictionaryValue::Iterator iter(*left_dict);
+            iter.HasNext(); iter.Advance()) {

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

nit: indent?  (May be just formatting in Rietveld.)

[benjhayden, 2012/06/19 15:01:59]

Done.

+          base::Value* right_value = NULL;
+          if (right_dict->HasKey(iter.key()) &&
+              right_dict->Get(iter.key(), &right_value) &&
+              !iter.value().Equals(right_value)) {
+            return false;
+          }
+        }
+        return true;
+      } else if ((event_name_ == extension_event_names::kOnDownloadErased) &&
+                 args_.get() &&
+                 other.args_.get()) {
+        int my_id = -1, other_id = -1;
+        return (args_->GetAsInteger(&my_id) &&
+                other.args_->GetAsInteger(&other_id) &&
+                my_id == other_id);
+      }
+      return json_args_ == other.json_args_;
+    }
+
+    std::string Debug() {
+      return base::StringPrintf("Event(%p, %s, %s, %f)",
+                                profile_,
+                                event_name_.c_str(),
+                                json_args_.c_str(),
+                                caught_.ToJsTime());
+    }
+
+   private:
+    Profile* profile_;
+    std::string event_name_;
+    std::string json_args_;
+    scoped_ptr<base::Value> args_;
+    base::Time caught_;
+
+    DISALLOW_COPY_AND_ASSIGN(Event);
+  };
+
+  typedef ExtensionDownloadsEventRouter::DownloadsNotificationSource
+    DownloadsNotificationSource;
+
+  void Observe(int type, const content::NotificationSource& source,
+               const content::NotificationDetails& details) {
+    switch (type) {
+      case chrome::NOTIFICATION_EXTENSION_DOWNLOADS_EVENT:
+        {
+          DownloadsNotificationSource* dns =
+              content::Source<DownloadsNotificationSource>(source).ptr();
+          ObserveInternal(dns->profile, dns->event_name,
+                          *content::Details<std::string>(details).ptr());
+          break;
+        }
+      default:
+        NOTREACHED();
+    }
+  }
+
+  // This is public to accommodate the HTML5 FileSystem Files event.

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

I found this comment confusing when I first read it.  Maybe "This is public to
allow creating an indirect callback to this function from the HTML5 FileSystem
test below."?

I'll also note that you could get around the entire problem by consing up a
NotificationSource and NotificationDetails by hand and using Observer directly. 
Admittedly hacky, and I don't have a preference one way or the other, but
mentioning it anyway.

[benjhayden, 2012/06/19 15:01:59]

Done.

+  void ObserveInternal(Profile* profile, const std::string& event_name,
+                       const std::string& json_args) {
+    Event* new_event = new Event(
+        profile, event_name, json_args, base::Time::Now());
+    events_.push_back(new_event);
+    if (waiting_ &&
+        waiting_for_.get() &&
+        waiting_for_->Equals(*new_event)) {
+      waiting_ = false;
+      MessageLoopForUI::current()->Quit();
+    }
+  }
+
+  bool WaitFor(int timeout_ms,
+               Profile* profile,
+               const std::string& event_name,
+               const std::string& json_args) {
+    waiting_for_.reset(new Event(profile, event_name, json_args, base::Time()));
+    for (std::deque<Event*>::const_iterator iter = events_.begin();
+         iter != events_.end(); ++iter) {
+      if ((*iter)->Equals(*waiting_for_.get()))
+        return true;
+    }
+    waiting_ = true;
+    if (timeout_ms > 0) {
+      MessageLoop::current()->PostDelayedTask(
+          FROM_HERE, base::Bind(
+              &DownloadsEventsListener::TimedOut, base::Unretained(this)),
+          timeout_ms);
+    }
+    ui_test_utils::RunMessageLoop();
+    bool success = !waiting_;
+    if (waiting_) {
+      // Print the events that were caught since the last WaitFor() call to help
+      // find the erroneous event.
+      // TODO(benjhayden) Fuzzy-match and highlight the erroneous event.
+      for (std::deque<Event*>::const_iterator iter = events_.begin();
+          iter != events_.end(); ++iter) {
+        if ((*iter)->caught() > last_wait_) {
+          LOG(INFO) << "Caught " << (*iter)->Debug();
+        }
+      }
+      if (waiting_for_.get()) {
+        LOG(INFO) << "Timed out waiting for " << waiting_for_->Debug();
+      }
+      waiting_ = false;
+    }
+    waiting_for_.reset();
+    last_wait_ = base::Time::Now();
+    return success;
+  }
+
+  bool WaitFor(Profile* profile,
+               const std::string& event_name,
+               const std::string& json_args) {
+    return WaitFor(kTimeOutMs, profile, event_name, json_args);
+  }
+
+ private:
+  void TimedOut() {
+    if (!waiting_for_.get())
+      return;
+    MessageLoopForUI::current()->Quit();
+  }
+
+  bool waiting_;
+  base::Time last_wait_;
+  scoped_ptr<Event> waiting_for_;
+  content::NotificationRegistrar registrar_;
+  std::deque<Event*> events_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadsEventsListener);
+};
+
+class DownloadExtensionTest : public ExtensionApiTest {
+ public:
+  DownloadExtensionTest()
+    : extension_(NULL),
+      incognito_browser_(NULL),
+      current_browser_(NULL) {
+  }
+
+  void ObserveEvent(Profile* profile, const std::string& event_name,
+                    const std::string& json_args) {
+    events_listener_->ObserveInternal(profile, event_name, json_args);
+  }
+
  protected:
   // Used with CreateHistoryDownloads
   struct HistoryDownloadInfo {
@@ -61,7 +270,12 @@ class DownloadExtensionTest : public InProcessBrowserTest {
     content::DownloadDangerType danger_type;
   };
 
-  virtual Browser* current_browser() { return browser(); }
+  void LoadExtension(const char* name) {

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

I find this confusing; reasonable to put a comment in explaining what this does?
 I'm specifically confused as to why we're redirecting from a function name that
implies (to me) non-incognito to an incognito function.

[benjhayden, 2012/06/19 15:01:59]

Done.

+    extension_ = LoadExtensionIncognito(test_data_dir_.AppendASCII(name));
+    CHECK(extension_);
+  }
+
+  Browser* current_browser() { return current_browser_; }
 
   // InProcessBrowserTest
   virtual void SetUpOnMainThread() OVERRIDE {
@@ -69,14 +283,59 @@ class DownloadExtensionTest : public InProcessBrowserTest {
         BrowserThread::IO, FROM_HERE,
         base::Bind(&chrome_browser_net::SetUrlRequestMocksEnabled, true));
     InProcessBrowserTest::SetUpOnMainThread();
+    GoOnTheRecord();
     CreateAndSetDownloadsDirectory();
     current_browser()->profile()->GetPrefs()->SetBoolean(
         prefs::kPromptForDownload, false);
-    GetDownloadManager()->RemoveAllDownloads();
+    GetOnRecordManager()->RemoveAllDownloads();
+    events_listener_.reset(new DownloadsEventsListener());
+  }
+
+  void GoOnTheRecord() { current_browser_ = browser(); }
+
+  void GoOffTheRecord() {
+    if (!incognito_browser_) {
+      incognito_browser_ = CreateIncognitoBrowser();
+      GetOffRecordManager()->RemoveAllDownloads();
+    }
+    current_browser_ = incognito_browser_;
+  }
+
+  bool WaitFor(int timeout_ms, const std::string& event_name,
+               const std::string& json_args) {
+    return events_listener_->WaitFor(
+        timeout_ms, current_browser()->profile(), event_name, json_args);
+  }
+
+  bool WaitFor(const std::string& event_name, const std::string& json_args) {
+    return events_listener_->WaitFor(
+        current_browser()->profile(), event_name, json_args);
+  }
+
+  std::string GetURL(const char* path) {

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Why not test_server()->GetURL?

[benjhayden, 2012/06/19 15:01:59]

Done.

+    return base::StringPrintf("http://localhost:%d/%s",
+        test_server()->host_port_pair().port(),
+        path);
+  }
+
+  std::string GetExtensionURL() {
+    return extension_->url().spec();
   }
 
-  virtual DownloadManager* GetDownloadManager() {
-    return BrowserContext::GetDownloadManager(current_browser()->profile());
+  std::string GetFilename(const char* path) {
+    return downloads_directory_.path().AppendASCII(path).AsUTF8Unsafe();
+  }
+
+  DownloadManager* GetOnRecordManager() {
+    return BrowserContext::GetDownloadManager(browser()->profile());
+  }
+  DownloadManager* GetOffRecordManager() {
+    return BrowserContext::GetDownloadManager(
+        browser()->profile()->GetOffTheRecordProfile());
+  }
+  DownloadManager* GetCurrentManager() {
+    return (current_browser_ == incognito_browser_) ?
+      GetOffRecordManager() : GetOnRecordManager();
   }

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

I have mixed feelings about On/Off record state being stateful in the contexts
of the tests; my leaning would have been to plumb a flag/pass in the correct
profile to all functions whose behavior would have been different in the two
cases so that you don't have to keep state in your mind as you scan the test
code.  OTOH, I could imagine that could end up being a lot of confusing
plumbing.  So I'll raise the issue, but leave it up to you which way you go.

[benjhayden, 2012/06/19 15:01:59]

I originally started with passing flags everywhere. Besides the mysterious
nameless boolean problem (which could be solved with an enum), there was the
problem of copy-pasting bits of test code around and forgetting to update each
and every little boolean. I could store the boolean in each TEST_F, but that's
unnecessary code duplication.
Reading through the code, I like how not every line of test code needs to think
about incognito. When you tell a story, you don't set the scene over and over in
every sentence.

 
   // Creates a set of history downloads based on the provided |history_info|
@@ -101,8 +360,8 @@ class DownloadExtensionTest : public InProcessBrowserTest {
           false);                 // opened
       entries.push_back(entry);
     }
-    GetDownloadManager()->OnPersistentStoreQueryComplete(&entries);
-    GetDownloadManager()->GetAllDownloads(FilePath(), items);
+    GetOnRecordManager()->OnPersistentStoreQueryComplete(&entries);
+    GetOnRecordManager()->GetAllDownloads(FilePath(), items);
     EXPECT_EQ(count, items->size());
     if (count != items->size())
       return false;
@@ -136,7 +395,7 @@ class DownloadExtensionTest : public InProcessBrowserTest {
       // We don't expect a select file dialog.
       ASSERT_FALSE(observer->select_file_dialog_seen());
     }
-    GetDownloadManager()->GetAllDownloads(FilePath(), items);
+    GetCurrentManager()->GetAllDownloads(FilePath(), items);
     ASSERT_EQ(count, items->size());
   }
 
@@ -144,7 +403,7 @@ class DownloadExtensionTest : public InProcessBrowserTest {
     scoped_ptr<DownloadTestObserver> observer(
         CreateInProgressDownloadObserver(1));
     GURL slow_download_url(URLRequestSlowDownloadJob::kUnknownSizeUrl);
-    DownloadManager* manager = GetDownloadManager();
+    DownloadManager* manager = GetCurrentManager();
 
     EXPECT_EQ(0, manager->InProgressCount());
     if (manager->InProgressCount() != 0)
@@ -188,15 +447,22 @@ class DownloadExtensionTest : public InProcessBrowserTest {
 
   DownloadTestObserver* CreateDownloadObserver(size_t download_count) {
     return new DownloadTestObserverTerminal(
-        GetDownloadManager(), download_count, true,
+        GetCurrentManager(), download_count, true,
         DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
   }
 
   DownloadTestObserver* CreateInProgressDownloadObserver(
       size_t download_count) {
-    return new DownloadTestObserverInProgress(GetDownloadManager(),
-                                              download_count,
-                                              true);
+    return new DownloadTestObserverInProgress(
+        GetCurrentManager(), download_count, true);
+  }
+
+  bool RunFunction(UIThreadExtensionFunction* function,
+                   const std::string& args) {
+    scoped_refptr<UIThreadExtensionFunction> delete_function(function);
+    SetUpExtensionFunction(function);
+    return extension_function_test_utils::RunFunction(
+        function, args, browser(), GetFlags());
   }
 
   extension_function_test_utils::RunFunctionFlags GetFlags() {
@@ -207,25 +473,18 @@ class DownloadExtensionTest : public InProcessBrowserTest {
 
   // extension_function_test_utils::RunFunction*() only uses browser for its
   // profile(), so pass it the on-record browser so that it always uses the
-  // on-record profile.
-
-  bool RunFunction(UIThreadExtensionFunction* function,
-                   const std::string& args) {
-    // extension_function_test_utils::RunFunction() does not take
-    // ownership of |function|.
-    scoped_refptr<ExtensionFunction> function_owner(function);
-    return extension_function_test_utils::RunFunction(
-        function, args, browser(), GetFlags());
-  }
+  // on-record profile to match real-life behavior.

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Why does it match real-life behavior?  Does this same thing happen if an
extension runs in incognito mode?  If not, how are we testing incognito mode
running of functions?

[benjhayden, 2012/06/19 15:01:59]

Yes, ExtensionFunctions get the on-record profile even when in incognito
contexts.
ExtensionFunctions use include_incognito() to determine whether to act like
they're in incognito contexts. You'd have to ask Aaron why.
I tell the functions that they're in incognito by passing
extension_function_test_utils::INCLUDE_INCOGNITO (from GetFlags()) through
ExtensionFunction::include_incognito().

 
   base::Value* RunFunctionAndReturnResult(UIThreadExtensionFunction* function,
                                           const std::string& args) {
+    SetUpExtensionFunction(function);
     return extension_function_test_utils::RunFunctionAndReturnResult(
         function, args, browser(), GetFlags());
   }
 
   std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
                                         const std::string& args) {
+    SetUpExtensionFunction(function);
     return extension_function_test_utils::RunFunctionAndReturnError(
         function, args, browser(), GetFlags());
   }
@@ -233,6 +492,7 @@ class DownloadExtensionTest : public InProcessBrowserTest {
   bool RunFunctionAndReturnString(UIThreadExtensionFunction* function,
                                   const std::string& args,
                                   std::string* result_string) {
+    SetUpExtensionFunction(function);
     scoped_ptr<base::Value> result(RunFunctionAndReturnResult(function, args));
     EXPECT_TRUE(result.get());
     return result.get() && result->GetAsString(result_string);
@@ -270,6 +530,17 @@ class DownloadExtensionTest : public InProcessBrowserTest {
   }
 
  private:
+  void SetUpExtensionFunction(UIThreadExtensionFunction* function) {
+    if (extension_) {
+      // Recreate the tab each time for insulation.
+      TabContents* tab = current_browser()->AddSelectedTabWithURL(
+          extension_->GetResourceURL("empty.html"),
+          content::PAGE_TRANSITION_LINK);
+      function->set_extension(extension_);
+      function->SetRenderViewHost(tab->web_contents()->GetRenderViewHost());
+    }
+  }
+
   void CreateAndSetDownloadsDirectory() {
     ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir());
     current_browser()->profile()->GetPrefs()->SetFilePath(
@@ -278,26 +549,12 @@ class DownloadExtensionTest : public InProcessBrowserTest {
   }
 
   ScopedTempDir downloads_directory_;
-};
-
-class DownloadExtensionTestIncognito : public DownloadExtensionTest {
- public:
-  virtual Browser* current_browser() OVERRIDE { return current_browser_; }
-
-  virtual void SetUpOnMainThread() OVERRIDE {
-    GoOnTheRecord();
-    DownloadExtensionTest::SetUpOnMainThread();
-    incognito_browser_ = CreateIncognitoBrowser();
-    GoOffTheRecord();
-    GetDownloadManager()->RemoveAllDownloads();
-  }
-
-  void GoOnTheRecord() { current_browser_ = browser(); }
-  void GoOffTheRecord() { current_browser_ = incognito_browser_; }
-
- private:
+  const extensions::Extension* extension_;
   Browser* incognito_browser_;
   Browser* current_browser_;
+  scoped_ptr<DownloadsEventsListener> events_listener_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadExtensionTest);
 };
 
 class MockIconExtractorImpl : public DownloadFileIconExtractor {
@@ -376,7 +633,32 @@ class ScopedItemVectorCanceller {
   DISALLOW_COPY_AND_ASSIGN(ScopedItemVectorCanceller);
 };
 
-} // namespace
+void FileCreated(const std::string& url, const base::Closure& done,

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Could you put two comments here, one that these functions are listed in reverse
order of execution for reference reasons (side note: I debated asking for
prototypes + ordering by execution order, and decided that was overkill, but I
wouldn't object if you decided you wanted to do it), and another before each
function just saying what it does?  Alternatively, a longer comment just going
over the thread hopping and operations done on each thread at the top.  I think
it'll make it easier to read for people who don't know the code. 

[benjhayden, 2012/06/19 15:01:59]

Done.

+                 base::PlatformFileError result) {
+  CHECK(BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, done));
+}
+
+void CreateFileOnIOThread(fileapi::FileSystemOperationInterface* operation,
+                          const base::Closure& done,
+                          const std::string& url) {
+  operation->CreateFile(GURL(url),
+                        true/*exclusive*/,
+                        base::Bind(&FileCreated, url, done));
+}
+
+void OpenFileSystemCallback(fileapi::FileSystemContext* fs,
+                            const std::string& filename,
+                            const base::Closure& done,
+                            base::PlatformFileError result,
+                            const std::string& fs_name,
+                            const GURL& root) {
+  fileapi::FileSystemOperationInterface* operation =
+    fs->CreateFileSystemOperation(root);
+  CHECK(BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
+      &CreateFileOnIOThread, operation, done, root.spec() + filename)));

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

nit: indent?  (Again not sure about Rietveld formatting.)

[benjhayden, 2012/06/19 15:01:59]

Looks right to me. The previous line ends with a paren, so this line is indented
+4.

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

Right--did the alignment wrong in my head.  Ooops.

+}
+
+}  // namespace
 
 IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_PauseResumeCancel) {
   DownloadItem* download_item = CreateSlowTestDownload();
@@ -430,8 +712,17 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_PauseResumeCancel) {
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
 
-  // Calling pause()/resume()/cancel() with invalid download Ids is
-  // tested in the API test (DownloadsApiTest).
+  // Calling paused on a non-existent download yields kInvalidOperationError.
+  error = RunFunctionAndReturnError(
+      new DownloadsPauseFunction(), "[-42]");
+  EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+               error.c_str());
+
+  // Calling resume on a non-existent download yields kInvalidOperationError
+  error = RunFunctionAndReturnError(
+      new DownloadsResumeFunction(), "[-42]");
+  EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
+               error.c_str());
 }
 
 // Test downloads.getFileIcon() on in-progress, finished, cancelled and deleted
@@ -526,9 +817,6 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_FileIcon_Active) {
   error = RunFunctionAndReturnError(new DownloadsGetFileIconFunction(), args);
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
-
-  // Asking for icons of other (invalid) sizes is tested in the API test
-  // (DownloadsApiTest).
 }
 
 // Test that we can acquire file icons for history downloads regardless of
@@ -819,8 +1107,8 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_SearchPlural) {
   ASSERT_EQ(items[2]->GetFullPath().value(), item_name);
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadExtensionTestIncognito,
-                       DownloadsApi_SearchIncognito) {
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_SearchIncognito) {
   scoped_ptr<base::Value> result_value;
   base::ListValue* result_list = NULL;
   base::DictionaryValue* result_dict = NULL;
@@ -832,22 +1120,22 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTestIncognito,
   std::string result_string;
 
   // Set up one on-record item and one off-record item.
+  GoOffTheRecord();
+  DownloadItem* off_item = CreateSlowTestDownload();
+  ASSERT_TRUE(off_item);
+  ASSERT_TRUE(off_item->IsOtr());
+  off_item_arg = DownloadItemIdAsArgList(off_item);

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Why reverse the order?  Any particular reason?  If it's subtle, it probably
deserves a comment.

[benjhayden, 2012/06/19 15:01:59]

Done.

 
   GoOnTheRecord();
   DownloadItem* on_item = CreateSlowTestDownload();
   ASSERT_TRUE(on_item);
   ASSERT_FALSE(on_item->IsOtr());
   on_item_arg = DownloadItemIdAsArgList(on_item);
-
-  GoOffTheRecord();
-  DownloadItem* off_item = CreateSlowTestDownload();
-  ASSERT_TRUE(off_item);
-  ASSERT_TRUE(off_item->IsOtr());
   ASSERT_TRUE(on_item->GetFullPath() != off_item->GetFullPath());
-  off_item_arg = DownloadItemIdAsArgList(off_item);
 
   // Extensions running in the incognito window should have access to both
   // items because the Test extension is in spanning mode.
+  GoOffTheRecord();
   result_value.reset(RunFunctionAndReturnResult(
       new DownloadsSearchFunction(), "[{}]"));
   ASSERT_TRUE(result_value.get());
@@ -893,9 +1181,6 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTestIncognito,
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
 
-  // TODO(benjhayden): Test incognito_split_mode() extension.
-  // TODO(benjhayden): Test download(), onCreated, onChanged, onErased.
-
   GoOffTheRecord();
 
   // Do the FileIcon test for both the on- and off-items while off the record.
@@ -953,3 +1238,767 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTestIncognito,
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
 }
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_DownloadsEventsListener) {
+  // Test that DownloadsEventsListener times out quickly enough for an event
+  // that isn't going to happen.
+  ASSERT_FALSE(WaitFor(100, extension_event_names::kOnDownloadErased, "-1"));

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

What is this actually testing?  Just that 100 != infinity?

[benjhayden, 2012/06/19 15:01:59]

This is actually testing that DownloadsEventsListener times out quickly enough
for an event that isn't going to happen.
Do you have a more specific question?

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

What you look to be testing is that WaitFor(100, ...) actually times out. 
There's no definition of "quickly enough"; it's just a binary thing.  Thus my
question as to whether it's just that 100 != infinity (to WaitFor(); it clearly
doesn't to other entities :-}).  Is that right?  

If so, it's primarily the words "quickly enough" that I dislike; you're just
testing that the timeout functionality works.  (I wince a bit at testing it and
then not using it, especially in a browser_test, as that just takes time.  But
it's the, to me, misleading comment that provoked my question.)

[benjhayden, 2012/06/21 17:50:48]

The test doesn't use the timeout, and I can always add it back if I need it, so
I took it out.

+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Basic) {
+  LoadExtension("downloads_split");

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

What does this do/what is it for?  Some documentation wouldn't be amiss. 
Possibly it's worth abstracting this behind a base function that's named
something more explanatory, and having a longer comment at that base function?

[benjhayden, 2012/06/19 15:01:59]

Done.

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

As discussed, maybe a comment in the downloads_split extension explaining its
purpose in life?

[benjhayden, 2012/06/21 17:50:48]

Done.

+  CHECK(StartTestServer());
+  std::string download_url = GetURL("slow?0");
+  GoOnTheRecord();
+  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()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Doesn't this result in a hung test if the event never happens?  (Possibly same
comment as question about -1 timeout above.)

[benjhayden, 2012/06/19 15:01:59]

Yep. See my new comments on kTimeOutMs.

+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/plain\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          GetFilename("slow.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Incognito) {

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

If you're testing incognito, shouldn't part of that test be that it's not
visible from non-incognito contexts?

[benjhayden, 2012/06/19 15:01:59]

That's tested in SearchIncognito.

+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  GoOffTheRecord();
+  std::string download_url = GetURL("slow?0");
+  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()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": true,"
+                          "  \"mime\": \"text/plain\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\":%d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"new\": \"complete\","
+                          "    \"old\": \"in_progress\"}}]",
+                          result_id,
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          GetFilename("slow.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_UnsafeHeaders) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  GoOnTheRecord();
+  // Test that we disallow certain headers case-insensitive.
+  static const char* kUnsafeHeaders[] = {
+    "Accept-chArsEt",
+    "accept-eNcoding",
+    "coNNection",
+    "coNteNt-leNgth",
+    "cooKIE",
+    "cOOkie2",
+    "coNteNt-traNsfer-eNcodiNg",
+    "dAtE",
+    "ExpEcT",
+    "hOsT",
+    "kEEp-aLivE",
+    "rEfErEr",
+    "tE",
+    "trAilER",
+    "trANsfer-eNcodiNg",
+    "upGRAde",
+    "usER-agENt",
+    "viA",
+    "pRoxY-",
+    "sEc-",
+    "pRoxY-probably-not-evil",
+    "sEc-probably-not-evil",
+    "oRiGiN",
+    "Access-Control-Request-Headers",
+    "Access-Control-Request-Method",
+  };
+  for (size_t index = 0; index < arraysize(kUnsafeHeaders); ++index) {
+    std::string download_url = GetURL("slow?0");
+    EXPECT_STREQ(download_extension_errors::kGenericError,
+                  RunFunctionAndReturnError(new DownloadsDownloadFunction(),
+                                            base::StringPrintf(
+        "[{\"url\": \"%s\","
+        "  \"filename\": \"unsafe-header-%lu.txt\","
+        "  \"headers\": [{"
+        "    \"name\": \"%s\","
+        "    \"value\": \"unsafe\"}]}]",
+        download_url.c_str(), index, kUnsafeHeaders[index])).c_str());
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Subdirectory) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that subdirectories (slashes) are disallowed in filenames.
+  // TODO(benjhayden) Update this when subdirectories are supported.
+  std::string download_url = GetURL("slow?0");
+  GoOnTheRecord();
+  EXPECT_STREQ(download_extension_errors::kGenericError,
+                RunFunctionAndReturnError(new DownloadsDownloadFunction(),
+                                          base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"sub/dir/ect/ory.txt\"}]",
+      download_url.c_str())).c_str());
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_InvalidFilename) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that invalid filenames are disallowed.
+  std::string download_url = GetURL("slow?0");
+  GoOnTheRecord();
+  EXPECT_STREQ(download_extension_errors::kGenericError,
+                RunFunctionAndReturnError(new DownloadsDownloadFunction(),
+                                          base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"../../../../../etc/passwd\"}]",
+      download_url.c_str())).c_str());
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_InvalidURLs) {
+  LoadExtension("downloads_split");
+  GoOnTheRecord();
+  static const char* kInvalidURLs[] = {
+    "foo bar",
+    "../hello",
+    "/hello",
+    "google.com/",
+    "http://",
+    "#frag",
+    "foo/bar.html#frag",
+    "javascript:document.write(\\\"hello\\\");",
+    "javascript:return false;",
+    "ftp://example.com/example.txt",
+  };
+  for (size_t index = 0; index < arraysize(kInvalidURLs); ++index) {
+    EXPECT_STREQ(download_extension_errors::kInvalidURLError,
+                  RunFunctionAndReturnError(new DownloadsDownloadFunction(),
+                                            base::StringPrintf(
+        "[{\"url\": \"%s\"}]", kInvalidURLs[index])).c_str());
+  }
+}
+
+// TODO(benjhayden): Set up a test ftp server, add ftp://localhost* to
+// permissions, test downloading from ftp.
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_URLFragment) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Valid URLs plus fragments are still valid URLs.
+  std::string download_url = GetURL("slow?0#fragment");
+  GoOnTheRecord();
+  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()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/plain\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          GetFilename("slow.txt.crdownload").c_str(),
+                          GetFilename("slow.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_DataURL) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  std::string download_url = "data:text/plain,hello";
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"data.txt\"}]", download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/plain\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("data.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          GetFilename("data.txt.crdownload").c_str(),
+                          GetFilename("data.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_File) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Valid file URLs are valid URLs.
+  std::string download_url = "file:///";
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"file.txt\"}]", download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+#if defined(OS_WIN)
+  download_url += "/";
+#endif
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/html\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("file.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          GetFilename("file.txt.crdownload").c_str(),
+                          GetFilename("file.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_AuthBasic_Fail) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  std::string download_url = GetURL("auth-basic");
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"auth-basic-fail.txt\"}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/html\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          download_url.c_str())));
+  // The item may or may not be interrupted before the onCreated event fires.

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

I'd add an explanatory line here or below (that in the "before" case the WaitFor
below is just checking that the event that already happened is more specific
than you've already tested for.)

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Also, this pattern seems to occur repeatedly in the file; any way to abstract
it?

[benjhayden, 2012/06/19 15:01:59]

Done.

[benjhayden, 2012/06/19 15:01:59]

Done.

+  static const int kExpectedError = 30;
+  if (item->IsInterrupted()) {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+        base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/html\","
+                          "  \"paused\": false,"
+                          "  \"state\": \"interrupted\","
+                          "  \"error\": %d,"
+                          "  \"url\": \"%s\"}]",
+                          kExpectedError,
+                          download_url.c_str())));
+  } else {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+        base::StringPrintf("[{\"id\": %d,"
+                            "  \"error\": {\"new\": 30},"
+                            "  \"state\":{"
+                            "    \"old\": \"in_progress\","
+                            "    \"new\":\"interrupted\"}}]",
+                            result_id)));
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Headers) {

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

It looks like this test is only testing that the system doesn't break when you
put in the "headers" arg (i.e. it doesn't look like we verify, in this test,
that the headers arg made it through to the server.)  That right?

[benjhayden, 2012/06/19 15:01:59]

The test server verifies the expected_headers, and only completes the download
if the headers are there.

+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test the |header| download option.
+  std::string download_url = GetURL("files/downloads/a_zip_file.zip?"
+      "expected_headers=Foo:bar&expected_headers=Qx:yo");
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"headers-succeed.txt\","
+      "  \"headers\": ["
+      "    {\"name\": \"Foo\", \"value\": \"bar\"},"
+      "    {\"name\": \"Qx\", \"value\":\"yo\"}]}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"application/octet-stream\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          download_url.c_str())));
+  std::string incomplete_filename = GetFilename(
+      "headers-succeed.txt.crdownload");
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          incomplete_filename.c_str(),
+                          GetFilename("headers-succeed.txt").c_str())));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

I think it's generally a good idea to have a line or two of comment before each
test describing what the test is actually testing.  When the test is really
simple and the name is clear, that's not needed, but otherwise I think it's a
useful pattern.

[benjhayden, 2012/06/19 15:01:59]

Done.

+                       DownloadExtensionTest_Download_Headers_Fail) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that headers-succeed would fail if the resource requires the headers
+  // and chrome fails to propagate them back to the server.  This tests both
+  // that testserver.py does not succeed when it should fail as well as how
+  // the downloads extension api exposes the failure to extensions.

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

As well as that the api really does propagate the headers all the way through.

[benjhayden, 2012/06/19 15:01:59]

No, that's tested by DownloadExtensionTest_Download_Headers

+  std::string download_url = GetURL("files/downloads/a_zip_file.zip?"
+      "expected_headers=Foo:bar&expected_headers=Qx:yo");
+  static const int kExpectedError = 33;
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"headers-fail.txt\"}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"bytesReceived\": 0,"
+                          "  \"mime\": \"\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]", download_url.c_str())));
+  if (item->IsInterrupted()) {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+        base::StringPrintf("[{\"danger\": \"safe\","
+                            "  \"incognito\": false,"
+                            "  \"bytesReceived\": 0,"
+                            "  \"mime\": \"\","
+                            "  \"paused\": false,"
+                            "  \"id\": %d,"
+                            "  \"state\": \"interrupted\","
+                            "  \"error\": %d,"
+                            "  \"url\": \"%s\"}]",
+                            result_id,
+                            kExpectedError,
+                            download_url.c_str())));
+  } else {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+        base::StringPrintf("[{\"id\": %d,"
+                            "  \"error\": {\"new\": %d},"
+                            "  \"state\": {"
+                            "    \"old\": \"in_progress\","
+                            "    \"new\": \"interrupted\"}}]",
+                            result_id,
+                            kExpectedError)));
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_AuthBasic) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  std::string download_url = GetURL("auth-basic");
+  // This is just base64 of 'username:secret'.
+  static const char* kAuthorization = "dXNlcm5hbWU6c2VjcmV0";
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"auth-basic-succeed.txt\","
+      "  \"headers\": [{"
+      "    \"name\": \"Authorization\","
+      "    \"value\": \"Basic %s\"}]}]",
+      download_url.c_str(), kAuthorization)));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/html\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]", download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]", result_id)));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Post) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  std::string download_url = GetURL("files/post/downloads/a_zip_file.zip?"
+      "expected_body=BODY");
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"filename\": \"post-succeed.txt\","
+      "  \"method\": \"POST\","
+      "  \"body\": \"BODY\"}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"application/octet-stream\","
+                          "  \"paused\": false,"
+                          "  \"bytesReceived\": 164,"
+                          "  \"url\": \"%s\"}]", download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]", result_id)));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Post_Get) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that downloadPostSuccess would fail if the resource requires the
+  // POST method, and chrome fails to propagate the |method| parameter back to
+  // the server. This tests both that testserver.py does not succeed when it
+  // should fail, and this tests how the downloads extension api exposes the
+  // failure to extensions.
+  std::string download_url = GetURL("files/post/downloads/a_zip_file.zip?"
+      "expected_body=BODY");
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+        new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"body\": \"BODY\","
+      "  \"filename\": \"post-get.txt\"}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  static const int kExpectedError = 33;
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"\","
+                          "  \"paused\": false,"
+                          "  \"id\": %d,"
+                          "  \"url\": \"%s\"}]",
+                          result_id,
+                          download_url.c_str())));
+  if (item->IsInterrupted()) {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+        base::StringPrintf("[{\"danger\": \"safe\","
+                            "  \"incognito\": false,"
+                            "  \"mime\": \"\","
+                            "  \"paused\": false,"
+                            "  \"id\": %d,"
+                            "  \"state\": \"interrupted\","
+                            "  \"error\": %d,"
+                            "  \"url\": \"%s\"}]",
+                            result_id,
+                            kExpectedError,
+                            download_url.c_str())));
+  } else {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+        base::StringPrintf("[{\"id\": %d,"
+                            "  \"error\": {\"new\": %d},"
+                            "  \"state\": {"
+                            "    \"old\": \"in_progress\","
+                            "    \"new\": \"interrupted\"}}]",
+                            result_id,
+                            kExpectedError)));
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Post_NoBody) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that downloadPostSuccess would fail if the resource requires the
+  // POST method, and chrome fails to propagate the |body| parameter back to
+  // the server. This tests both that testserver.py does not succeed when it
+  // should fail, and this tests how the downloads extension api exposes the
+  // failure to extensions.
+  std::string download_url = GetURL("files/post/downloads/a_zip_file.zip?"
+      "expected_body=BODY");
+  GoOnTheRecord();
+  scoped_ptr<base::Value> result(RunFunctionAndReturnResult(
+      new DownloadsDownloadFunction(), base::StringPrintf(
+      "[{\"url\": \"%s\","
+      "  \"method\": \"POST\","
+      "  \"filename\": \"post-nobody.txt\"}]",
+      download_url.c_str())));
+  ASSERT_TRUE(result.get());
+  int result_id = -1;
+  ASSERT_TRUE(result->GetAsInteger(&result_id));
+  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  static const int kExpectedError = 33;
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"\","
+                          "  \"paused\": false,"
+                          "  \"id\": %d,"
+                          "  \"url\": \"%s\"}]",
+                          result_id,
+                          download_url.c_str())));
+  if (item->IsInterrupted()) {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+        base::StringPrintf("[{\"danger\": \"safe\","
+                            "  \"incognito\": false,"
+                            "  \"mime\": \"\","
+                            "  \"paused\": false,"
+                            "  \"id\": %d,"
+                            "  \"state\": \"interrupted\","
+                            "  \"error\": %d,"
+                            "  \"url\": \"%s\"}]",
+                            result_id,
+                            kExpectedError,
+                            download_url.c_str())));
+  } else {
+    ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+        base::StringPrintf("[{\"id\": %d,"
+                            "  \"error\": {\"new\": %d},"
+                            "  \"state\": {"
+                            "    \"old\": \"in_progress\","
+                            "    \"new\": \"interrupted\"}}]",
+                            result_id,
+                            kExpectedError)));
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_Cancel) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test that cancel()ing an in-progress download causes its state to
+  // transition to interrupted, and test that that state transition is
+  // detectable by an onChanged event listener.  TODO(benjhayden): Test other
+  // sources of interruptions such as server death.
+  std::string download_url = GetURL("download-known-size");
+  GoOnTheRecord();
+  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()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"application/octet-stream\","
+                          "  \"paused\": false,"
+                          "  \"id\": %d,"
+                          "  \"url\": \"%s\"}]",
+                          result_id,
+                          download_url.c_str())));
+  item->Cancel(true);
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"error\": {\"new\": 40},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"interrupted\"}}]",
+                          result_id)));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
+                       DownloadExtensionTest_Download_FileSystemURL) {
+  LoadExtension("downloads_split");
+  CHECK(StartTestServer());
+  // Test downloading filesystem: URLs.
+  // NOTE: chrome disallows creating HTML5 FileSystem Files in incognito.

[Randy Smith (Not in Mondays), 2012/06/18 18:42:58]

Should we test that?

[benjhayden, 2012/06/19 15:01:59]

What would it buy to test that HTML5 Files are disallowed in incognito?
I suppose it would let us know if/when we need to start testing downloading
html5 files in incognito.
Is that what you were thinking?

[Randy Smith (Not in Mondays), 2012/06/19 19:23:23]

In a sorta vague fuzzy background way, yeah.  I don't consider it high priority.

+  std::string download_url = "filesystem:" + GetExtensionURL() +
+    "temporary/on_record.txt";
+  static const char* kHTML5FileCreated = "html5_file_created";
+  GoOnTheRecord();
+  fileapi::FileSystemContext* fs = BrowserContext::GetFileSystemContext(
+      browser()->profile());
+  base::Closure file_created = base::Bind(
+      &DownloadExtensionTest::ObserveEvent,
+      base::Unretained(this),
+      browser()->profile(),
+      kHTML5FileCreated,
+      "on_record");
+  fileapi::FileSystemContext::OpenFileSystemCallback fs_opened = base::Bind(
+      &OpenFileSystemCallback,
+      base::Unretained(fs),
+      "on_record.txt",
+      file_created);
+  fs->OpenFileSystem(GURL(GetExtensionURL()),
+                      fileapi::kFileSystemTypeTemporary,
+                      true/*create*/,
+                      fs_opened);
+  ASSERT_TRUE(WaitFor(kHTML5FileCreated, "on_record"));
+  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()->GetActiveDownloadItem(result_id);
+  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  ASSERT_TRUE(item);
+  ScopedCancellingItem canceller(item);
+  ASSERT_EQ(download_url, item->GetURL().spec());
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadCreated,
+      base::StringPrintf("[{\"danger\": \"safe\","
+                          "  \"filename\": \"%s\","
+                          "  \"incognito\": false,"
+                          "  \"mime\": \"text/plain\","
+                          "  \"paused\": false,"
+                          "  \"url\": \"%s\"}]",
+                          GetFilename("on_record.txt.crdownload").c_str(),
+                          download_url.c_str())));
+  ASSERT_TRUE(WaitFor(extension_event_names::kOnDownloadChanged,
+      base::StringPrintf("[{\"id\": %d,"
+                          "  \"filename\": {"
+                          "    \"old\": \"%s\","
+                          "    \"new\": \"%s\"},"
+                          "  \"state\": {"
+                          "    \"old\": \"in_progress\","
+                          "    \"new\": \"complete\"}}]",
+                          result_id,
+                          GetFilename("on_record.txt.crdownload").c_str(),
+                          GetFilename("on_record.txt").c_str())));
+}