Rewrite DownloadsApiTest in C++.

chrome/browser/download/download_extension_test.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/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;
 using content::DownloadItem;
 using content::DownloadManager;
 using content::DownloadPersistentStoreInfo;
 
 namespace {
 
 // Comparator that orders download items by their ID. Can be used with
 // std::sort.
 struct DownloadIdComparator {
   bool operator() (DownloadItem* first, DownloadItem* second) {
     return first->GetId() < second->GetId();
   }
 };
 
-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 {
     // Filename to use. CreateHistoryDownloads will append this filename to the
     // temporary downloads directory specified by downloads_directory().
     const FilePath::CharType*   filename;
 
     // State for the download. Note that IN_PROGRESS downloads will be created
     // as CANCELLED.
     DownloadItem::DownloadState state;
 
     // Danger type for the download. Only use DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS
     // and DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT.
     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 {
     BrowserThread::PostTask(
         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());
   }
 
-  virtual DownloadManager* GetDownloadManager() {
-    return BrowserContext::GetDownloadManager(current_browser()->profile());
+  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();
+  }
+
+  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|
   // array. |count| is the number of elements in |history_info|. On success,
   // |items| will contain |count| DownloadItems in the order that they were
   // specified in |history_info|. Returns true on success and false otherwise.
   bool CreateHistoryDownloads(const HistoryDownloadInfo* history_info,
                               size_t count,
                               DownloadManager::DownloadVector* items) {
     DownloadIdComparator download_id_comparator;
     base::Time current = base::Time::Now();
     std::vector<DownloadPersistentStoreInfo> entries;
     entries.reserve(count);
     for (size_t i = 0; i < count; ++i) {
       DownloadPersistentStoreInfo entry(
           downloads_directory().Append(history_info[i].filename),
           GURL(), GURL(),    // URL, referrer
           current, current,  // start_time, end_time
           1, 1,              // received_bytes, total_bytes
           history_info[i].state,  // state
           i + 1,                  // db_handle
           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;
 
     // Order by ID so that they are in the order that we created them.
     std::sort(items->begin(), items->end(), download_id_comparator);
     // Set the danger type if necessary.
     for (size_t i = 0; i < count; ++i) {
       if (history_info[i].danger_type !=
           content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS) {
@@ skipping 13 matching lines @@
       GURL slow_download_url(URLRequestSlowDownloadJob::kUnknownSizeUrl);
       ui_test_utils::NavigateToURLWithDisposition(
           current_browser(), slow_download_url, CURRENT_TAB,
           ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
       observer->WaitForFinished();
       EXPECT_EQ(
           1u, observer->NumDownloadsSeenInState(DownloadItem::IN_PROGRESS));
       // 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());
   }
 
   DownloadItem* CreateSlowTestDownload() {
     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)
       return NULL;
 
     ui_test_utils::NavigateToURLWithDisposition(
         current_browser(), slow_download_url, CURRENT_TAB,
         ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
 
     observer->WaitForFinished();
@@ skipping 23 matching lines @@
     GURL finish_url(URLRequestSlowDownloadJob::kFinishDownloadUrl);
     ui_test_utils::NavigateToURLWithDisposition(
         current_browser(), finish_url, NEW_FOREGROUND_TAB,
         ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
     observer->WaitForFinished();
     EXPECT_EQ(1u, observer->NumDownloadsSeenInState(DownloadItem::COMPLETE));
   }
 
   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() {
     return current_browser()->profile()->IsOffTheRecord() ?
            extension_function_test_utils::INCLUDE_INCOGNITO :
            extension_function_test_utils::NONE;
   }
 
   // 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());
   }
 
   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);
   }
 
   std::string DownloadItemIdAsArgList(const DownloadItem* download_item) {
     return base::StringPrintf("[%d]", download_item->GetId());
   }
 
   // Checks if a data URL encoded image is a PNG of a given size.
@@ skipping 17 matching lines @@
         &width, &height));
     EXPECT_EQ(expected_size, width);
     EXPECT_EQ(expected_size, height);
   }
 
   const FilePath& downloads_directory() {
     return downloads_directory_.path();
   }
 
  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(
         prefs::kDownloadDefaultDirectory,
         downloads_directory_.path());
   }
 
   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 {
  public:
   MockIconExtractorImpl(const FilePath& path, IconLoader::IconSize icon_size,
                         const std::string& response)
     : expected_path_(path),
       expected_icon_size_(icon_size),
       response_(response) {
   }
@@ skipping 58 matching lines @@
       if ((*item)->IsInProgress())
         (*item)->Cancel(true);
     }
   }
 
  private:
   DownloadManager::DownloadVector* items_;
   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();
   ASSERT_TRUE(download_item);
 
   // Call pause().  It should succeed and the download should be paused on
   // return.
   EXPECT_TRUE(RunFunction(new DownloadsPauseFunction(),
                           DownloadItemIdAsArgList(download_item)));
   EXPECT_TRUE(download_item->IsPaused());
@@ skipping 33 matching lines @@
       new DownloadsPauseFunction(), DownloadItemIdAsArgList(download_item));
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
 
   // Calling resume on a non-active download yields kInvalidOperationError
   error = RunFunctionAndReturnError(
       new DownloadsResumeFunction(), DownloadItemIdAsArgList(download_item));
   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
 // download items.
 IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_FileIcon_Active) {
   DownloadItem* download_item = CreateSlowTestDownload();
   ASSERT_TRUE(download_item);
 
   // Get the icon for the in-progress download.  This call should succeed even
   // if the file type isn't registered.
@@ skipping 74 matching lines @@
       expected_path, IconLoader::NORMAL, ""));
   std::string error = RunFunctionAndReturnError(function.release(), args);
   EXPECT_STREQ(download_extension_errors::kIconNotFoundError,
                error.c_str());
 
   // Once the download item is deleted, we should return kInvalidOperationError.
   download_item->Delete(DownloadItem::DELETE_DUE_TO_USER_DISCARD);
   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
 // whether they exist or not.  If the file doesn't exist we should receive a
 // generic icon from the OS/toolkit that may or may not be specific to the file
 // type.
 IN_PROC_BROWSER_TEST_F(DownloadExtensionTest, DownloadsApi_FileIcon_History) {
   const HistoryDownloadInfo kHistoryInfo[] = {
     { FILE_PATH_LITERAL("real.txt"),
       DownloadItem::COMPLETE,
@@ skipping 270 matching lines @@
   base::ListValue* result_list = NULL;
   ASSERT_TRUE(result->GetAsList(&result_list));
   ASSERT_EQ(1UL, result_list->GetSize());
   base::DictionaryValue* item_value = NULL;
   ASSERT_TRUE(result_list->GetDictionary(0, &item_value));
   FilePath::StringType item_name;
   ASSERT_TRUE(item_value->GetString("filename", &item_name));
   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;
   FilePath::StringType filename;
   bool is_incognito = false;
   std::string error;
   std::string on_item_arg;
   std::string off_item_arg;
   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());
   ASSERT_TRUE(result_value->GetAsList(&result_list));
   ASSERT_EQ(2UL, result_list->GetSize());
   ASSERT_TRUE(result_list->GetDictionary(0, &result_dict));
   ASSERT_TRUE(result_dict->GetString("filename", &filename));
   ASSERT_TRUE(result_dict->GetBoolean("incognito", &is_incognito));
   EXPECT_TRUE(on_item->GetFullPath() == FilePath(filename));
   EXPECT_FALSE(is_incognito);
@@ skipping 25 matching lines @@
   error = RunFunctionAndReturnError(new DownloadsResumeFunction(),
                                     off_item_arg);
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
   error = RunFunctionAndReturnError(
       new DownloadsGetFileIconFunction(),
       base::StringPrintf("[%d, {}]", off_item->GetId()));
   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.
   // NOTE(benjhayden): This does not include the FileIcon test from history,
   // just active downloads. This shouldn't be a problem.
   EXPECT_TRUE(RunFunctionAndReturnString(
       new DownloadsGetFileIconFunction(),
       base::StringPrintf("[%d, {}]", on_item->GetId()), &result_string));
   EXPECT_TRUE(RunFunctionAndReturnString(
       new DownloadsGetFileIconFunction(),
@@ skipping 37 matching lines @@
   EXPECT_TRUE(off_item->IsCancelled());
   error = RunFunctionAndReturnError(new DownloadsPauseFunction(),
                                     off_item_arg);
   EXPECT_STREQ(download_extension_errors::kInvalidOperationError,
                error.c_str());
   error = RunFunctionAndReturnError(new DownloadsResumeFunction(),
                                     off_item_arg);
   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())));
+}