Revert 121912 - Implement chrome.experimental.downloads.onChanged

Attempting to fix leak in DownloadProtectionServiceTest.CheckClientDownloadValidateRequest

ExtensionDownloadsEventRouter now also observes all DownloadItems and dispatches onChanged events.

Download.OnChanged records the percentage of OnDownloadUpdated() calls that are propagated as onChanged events instead of being suppressed.

BUG=12133

Review URL: http://codereview.chromium.org/8203005

TBR=benjhayden@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9360051

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@121915 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 103 insertions, 525 deletions

diff --git a/chrome/browser/download/download_extension_api.cc b/chrome/browser/download/download_extension_api.cc
index 0f92f2b..3ad3a46 100644
--- a/chrome/browser/download/download_extension_api.cc
+++ b/chrome/browser/download/download_extension_api.cc
@@ -860,9 +860,7 @@ void DownloadsGetFileIconFunction::OnIconURLExtracted(const std::string& url) {
 
 ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(Profile* profile)
   : profile_(profile),
-    manager_(NULL),
-    delete_item_jsons_(&item_jsons_),
-    delete_on_changed_stats_(&on_changed_stats_) {
+    manager_(NULL) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(profile_);
   // Register a callback with the DownloadService for this profile to be called
@@ -878,149 +876,56 @@ ExtensionDownloadsEventRouter::ExtensionDownloadsEventRouter(Profile* profile)
 void ExtensionDownloadsEventRouter::Init(DownloadManager* manager) {
   DCHECK(manager_ == NULL);
   manager_ = manager;
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   manager_->AddObserver(this);
 }
 
 ExtensionDownloadsEventRouter::~ExtensionDownloadsEventRouter() {
   if (manager_ != NULL)
     manager_->RemoveObserver(this);
-  for (ItemMap::const_iterator iter = downloads_.begin();
-       iter != downloads_.end(); ++iter) {
-    if (iter->second != NULL)
-      iter->second->RemoveObserver(this);
-  }
-}
-
-ExtensionDownloadsEventRouter::OnChangedStat::OnChangedStat()
-  : fires(0),
-    total(0) {
-}
-
-ExtensionDownloadsEventRouter::OnChangedStat::~OnChangedStat() {
-  if (total > 0)
-    UMA_HISTOGRAM_PERCENTAGE("Download.OnChanged", (fires * 100 / total));
-}
-
-void ExtensionDownloadsEventRouter::OnDownloadUpdated(DownloadItem* item) {
-  int download_id = item->GetId();
-  if (item->GetState() == DownloadItem::REMOVING) {
-    // The REMOVING state indicates that this item is being erased.
-    // Let's unregister as an observer so that we don't see any more updates
-    // from it, dispatch the onErased event, and remove its json and is
-    // OnChangedStat from our maps.
-    downloads_.erase(download_id);
-    item->RemoveObserver(this);
-    DispatchEvent(extension_event_names::kOnDownloadErased,
-                  base::Value::CreateIntegerValue(download_id));
-    delete item_jsons_[download_id];
-    item_jsons_.erase(download_id);
-    delete on_changed_stats_[download_id];
-    on_changed_stats_.erase(download_id);
-    return;
-  }
-
-  base::DictionaryValue* old_json = item_jsons_[download_id];
-  scoped_ptr<base::DictionaryValue> new_json(DownloadItemToJSON(item));
-  scoped_ptr<base::DictionaryValue> delta(new base::DictionaryValue());
-  delta->SetInteger(kIdKey, download_id);
-  std::set<std::string> new_fields;
-  bool changed = false;
-
-  // For each field in the new json representation of the item except the
-  // bytesReceived field, if the field has changed from the previous old json,
-  // set the differences in the |delta| object and remember that something
-  // significant changed.
-  for (base::DictionaryValue::Iterator iter(*new_json.get());
-       iter.HasNext(); iter.Advance()) {
-    new_fields.insert(iter.key());
-    if (iter.key() != kBytesReceivedKey) {
-      base::Value* old_value = NULL;
-      if (!old_json->HasKey(iter.key()) ||
-          (old_json->Get(iter.key(), &old_value) &&
-           !iter.value().Equals(old_value))) {
-        delta->Set(iter.key() + ".new", iter.value().DeepCopy());
-        if (old_value)
-          delta->Set(iter.key() + ".old", old_value->DeepCopy());
-        changed = true;
-      }
-    }
-  }
-
-  // If a field was in the previous json but is not in the new json, set the
-  // difference in |delta|.
-  for (base::DictionaryValue::Iterator iter(*old_json);
-       iter.HasNext(); iter.Advance()) {
-    if (new_fields.find(iter.key()) == new_fields.end()) {
-      delta->Set(iter.key() + ".old", iter.value().DeepCopy());
-      changed = true;
-    }
-  }
-
-  // Update the OnChangedStat and dispatch the event if something significant
-  // changed. Replace the stored json with the new json.
-  ++(on_changed_stats_[download_id]->total);
-  if (changed) {
-    DispatchEvent(extension_event_names::kOnDownloadChanged, delta.release());
-    ++(on_changed_stats_[download_id]->fires);
-  }
-  item_jsons_[download_id]->Swap(new_json.get());
-}
-
-void ExtensionDownloadsEventRouter::OnDownloadOpened(DownloadItem* item) {
 }
 
 void ExtensionDownloadsEventRouter::ModelChanged(DownloadManager* manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(manager_ == manager);
-  typedef std::set<int> DownloadIdSet;
-
-  // Get all the download items.
   DownloadManager::DownloadVector current_vec;
   manager_->SearchDownloads(string16(), &current_vec);
-
-  // Populate set<>s of download item identifiers so that we can find
-  // differences between the old and the new set of download items.
-  DownloadIdSet current_set, prev_set;
-  for (ItemMap::const_iterator iter = downloads_.begin();
-       iter != downloads_.end(); ++iter) {
-    prev_set.insert(iter->first);
-  }
+  DownloadIdSet current_set;
   ItemMap current_map;
   for (DownloadManager::DownloadVector::const_iterator iter =
        current_vec.begin();
        iter != current_vec.end(); ++iter) {
     DownloadItem* item = *iter;
     int item_id = item->GetId();
-    CHECK(item_id >= 0);
+    // TODO(benjhayden): Remove the following line when every item's id >= 0,
+    // which will allow firing onErased events for items from the history.
+    if (item_id < 0) continue;
     DCHECK(current_map.find(item_id) == current_map.end());
     current_set.insert(item_id);
     current_map[item_id] = item;
   }
-  DownloadIdSet new_set;  // current_set - prev_set;
+  DownloadIdSet new_set;  // current_set - downloads_;
+  DownloadIdSet erased_set;  // downloads_ - current_set;
+  std::insert_iterator<DownloadIdSet> new_insertor(new_set, new_set.begin());
+  std::insert_iterator<DownloadIdSet> erased_insertor(
+      erased_set, erased_set.begin());
   std::set_difference(current_set.begin(), current_set.end(),
-                      prev_set.begin(), prev_set.end(),
-                      std::insert_iterator<DownloadIdSet>(
-                        new_set, new_set.begin()));
-
-  // For each download that was not in the old set of downloads but is in the
-  // new set of downloads, fire an onCreated event, register as an Observer of
-  // the item, store a json representation of the item so that we can easily
-  // find changes in that json representation, and make an OnChangedStat.
+                      downloads_.begin(), downloads_.end(),
+                      new_insertor);
+  std::set_difference(downloads_.begin(), downloads_.end(),
+                      current_set.begin(), current_set.end(),
+                      erased_insertor);
   for (DownloadIdSet::const_iterator iter = new_set.begin();
        iter != new_set.end(); ++iter) {
     scoped_ptr<base::DictionaryValue> item(
         DownloadItemToJSON(current_map[*iter]));
-    DispatchEvent(extension_event_names::kOnDownloadCreated, item->DeepCopy());
-    DCHECK(item_jsons_.find(*iter) == item_jsons_.end());
-    on_changed_stats_[*iter] = new OnChangedStat();
-    current_map[*iter]->AddObserver(this);
-    item_jsons_[*iter] = item.release();
+    DispatchEvent(extension_event_names::kOnDownloadCreated, item.release());
   }
-  downloads_.swap(current_map);
-
-  // Dispatching onErased is handled in OnDownloadUpdated when an item
-  // transitions to the REMOVING state.
+  for (DownloadIdSet::const_iterator iter = erased_set.begin();
+       iter != erased_set.end(); ++iter) {
+    DispatchEvent(extension_event_names::kOnDownloadErased,
+                  base::Value::CreateIntegerValue(*iter));
+  }
+  downloads_.swap(current_set);
 }
 
 void ExtensionDownloadsEventRouter::ManagerGoingDown(
diff --git a/chrome/browser/download/download_extension_api.h b/chrome/browser/download/download_extension_api.h
index 3da28e7..92e4aa9 100644
--- a/chrome/browser/download/download_extension_api.h
+++ b/chrome/browser/download/download_extension_api.h
@@ -12,7 +12,6 @@
 
 #include "base/file_path.h"
 #include "base/memory/singleton.h"
-#include "base/stl_util.h"
 #include "base/string16.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/extension_function.h"
@@ -302,41 +301,24 @@ class DownloadsGetFileIconFunction : public AsyncDownloadsFunction {
   DISALLOW_COPY_AND_ASSIGN(DownloadsGetFileIconFunction);
 };
 
-// Observes a single DownloadManager and many DownloadItems and dispatches
-// onCreated and onErased events.
-class ExtensionDownloadsEventRouter : public content::DownloadManager::Observer,
-                                      public content::DownloadItem::Observer {
+class ExtensionDownloadsEventRouter
+    : public content::DownloadManager::Observer {
  public:
   explicit ExtensionDownloadsEventRouter(Profile* profile);
   virtual ~ExtensionDownloadsEventRouter();
 
   virtual void ModelChanged(content::DownloadManager* manager) OVERRIDE;
   virtual void ManagerGoingDown(content::DownloadManager* manager) OVERRIDE;
-  virtual void OnDownloadUpdated(content::DownloadItem* download) OVERRIDE;
-  virtual void OnDownloadOpened(content::DownloadItem* download) OVERRIDE;
 
  private:
-  struct OnChangedStat {
-    OnChangedStat();
-    ~OnChangedStat();
-    int fires;
-    int total;
-  };
-
-  typedef std::map<int, content::DownloadItem*> ItemMap;
-  typedef std::map<int, base::DictionaryValue*> ItemJsonMap;
-  typedef std::map<int, OnChangedStat*> OnChangedStatMap;
-
   void Init(content::DownloadManager* manager);
   void DispatchEvent(const char* event_name, base::Value* json_arg);
+  typedef base::hash_map<int, content::DownloadItem*> ItemMap;
+  typedef std::set<int> DownloadIdSet;
 
   Profile* profile_;
   content::DownloadManager* manager_;
-  ItemMap downloads_;
-  ItemJsonMap item_jsons_;
-  STLValueDeleter<ItemJsonMap> delete_item_jsons_;
-  OnChangedStatMap on_changed_stats_;
-  STLValueDeleter<OnChangedStatMap> delete_on_changed_stats_;
+  DownloadIdSet downloads_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionDownloadsEventRouter);
 };
diff --git a/chrome/test/data/extensions/api_test/downloads/test.js b/chrome/test/data/extensions/api_test/downloads/test.js
index 392fd0b..0a1368c 100644
--- a/chrome/test/data/extensions/api_test/downloads/test.js
+++ b/chrome/test/data/extensions/api_test/downloads/test.js
@@ -43,458 +43,169 @@ chrome.test.getConfig(function(testConfig) {
   // The "/slow" handler waits a specified amount of time before returning a
   // safe file. Specify zero seconds to return quickly.
   var SAFE_FAST_URL = getURL('slow?0');
-
   var NEVER_FINISH_URL = getURL('download-known-size');
-
-  // This URL should only work with the POST method and a request body
-  // containing 'BODY'.
-  var POST_URL = getURL('files/post/downloads/a_zip_file.zip?' +
-                        'expected_body=BODY');
-
-  // This URL should only work with headers 'Foo: bar' and 'Qx: yo'.
-  var HEADERS_URL = getURL('files/downloads/a_zip_file.zip?' +
-                           'expected_headers=Foo:bar&expected_headers=Qx:yo');
+  var ERROR_GENERIC = downloads.ERROR_GENERIC;
+  var ERROR_INVALID_URL = downloads.ERROR_INVALID_URL;
+  var ERROR_INVALID_OPERATION = downloads.ERROR_INVALID_OPERATION;
 
   chrome.test.runTests([
     // TODO(benjhayden): Test onErased using remove().
-
-    // TODO(benjhayden): Sub-directories depend on http://crbug.com/109443
-    // TODO(benjhayden): Windows slashes.
-    // function downloadSubDirectoryFilename() {
-    //   var downloadId = getNextId();
-    //   var callbackCompleted = chrome.test.callbackAdded();
-    //   function myListener(delta) {
-    //     if ((delta.id != downloadId) ||
-    //         !delta.filename ||
-    //         (delta.filename.new.indexOf('/foo/slow') == -1))
-    //       return;
-    //     downloads.onChanged.removeListener(myListener);
-    //     callbackCompleted();
-    //   }
-    //   downloads.onChanged.addListener(myListener);
-    //   downloads.download(
-    //       {'url': SAFE_FAST_URL, 'filename': 'foo/slow'},
-    //       chrome.test.callback(function(id) {
-    //         chrome.test.assertEq(downloadId, id);
-    //       }));
-    // },
-
-    function downloadSimple() {
-      // Test that we can begin a download.
-      var downloadId = getNextId();
-      downloads.download(
-          {'url': SAFE_FAST_URL},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadPostSuccess() {
-      // Test the |method| download option.
-      var downloadId = getNextId();
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides completion.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        downloads.search({id: downloadId},
-                          chrome.test.callback(function(items) {
-          chrome.test.assertEq(1, items.length);
-          chrome.test.assertEq(downloadId, items[0].id);
-          var EXPECTED_SIZE = 164;
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].totalBytes);
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].fileSize);
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].bytesReceived);
-        }));
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': POST_URL,
-           'method': 'POST',
-           'filename': downloadId + '.txt',
-           'body': 'BODY'},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadPostWouldFailWithoutMethod() {
-      // 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.
-      var downloadId = getNextId();
-
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides interruption.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        // TODO(benjhayden): Change COMPLETE to INTERRUPTED after
-        // http://crbug.com/112342
-        downloads.search({id: downloadId},
-                          chrome.test.callback(function(items) {
-          chrome.test.assertEq(1, items.length);
-          chrome.test.assertEq(downloadId, items[0].id);
-          chrome.test.assertEq(0, items[0].totalBytes);
-        }));
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': POST_URL,
-           'filename': downloadId + '.txt',  // Prevent 'file' danger.
-           'body': 'BODY'},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadPostWouldFailWithoutBody() {
-      // Test that downloadPostSuccess would fail if the resource requires the
-      // POST method and a request body, 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.
-      var downloadId = getNextId();
-
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides interruption.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        // TODO(benjhayden): Change COMPLETE to INTERRUPTED after
-        // http://crbug.com/112342
-        downloads.search({id: downloadId},
-                          chrome.test.callback(function(items) {
-          chrome.test.assertEq(1, items.length);
-          chrome.test.assertEq(downloadId, items[0].id);
-          chrome.test.assertEq(0, items[0].totalBytes);
-        }));
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': POST_URL,
-           'filename': downloadId + '.txt',  // Prevent 'file' danger.
-           'method': 'POST'},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadHeadersSuccess() {
-      // Test the |header| download option.
-      var downloadId = getNextId();
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides completion.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        downloads.search({id: downloadId},
-                          chrome.test.callback(function(items) {
-          chrome.test.assertEq(1, items.length);
-          chrome.test.assertEq(downloadId, items[0].id);
-          var EXPECTED_SIZE = 164;
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].totalBytes);
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].fileSize);
-          chrome.test.assertEq(EXPECTED_SIZE, items[0].bytesReceived);
-        }));
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': HEADERS_URL,
-           'filename': downloadId + '.txt',  // Prevent 'file' danger.
-           'headers': [{'name': 'Foo', 'value': 'bar'},
-                       {'name': 'Qx', 'value': 'yo'}]},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadHeadersWouldFail() {
-      // Test that downloadHeadersSuccess() 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.
-      var downloadId = getNextId();
-
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides interruption.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        // TODO(benjhayden): Change COMPLETE to INTERRUPTED after
-        // http://crbug.com/112342
-        downloads.search({id: downloadId},
-                          chrome.test.callback(function(items) {
-          chrome.test.assertEq(1, items.length);
-          chrome.test.assertEq(downloadId, items[0].id);
-          chrome.test.assertEq(0, items[0].totalBytes);
-        }));
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': HEADERS_URL},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadInterrupted() {
-      // 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.
-      var downloadId = getNextId();
-
-      var createdCompleted = chrome.test.callbackAdded();
-      function createdListener(createdItem) {
-        // Ignore onCreated events for any download besides our own.
-        if (createdItem.id != downloadId)
-          return;
-        // TODO(benjhayden) Move this cancel() into the download() callback
-        // after ensuring that DownloadItems are created before that callback
-        // is fired.
-        downloads.cancel(downloadId, chrome.test.callback(function() {
-        }));
-        downloads.onCreated.removeListener(createdListener);
-        createdCompleted();
-      }
-      downloads.onCreated.addListener(createdListener);
-
-      var changedCompleted = chrome.test.callbackAdded();
-      function changedListener(delta) {
-        // Ignore onChanged events for downloads besides our own, or events that
-        // signal any change besides interruption.
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_INTERRUPTED) ||
-            !delta.error ||
-            (delta.error.new != 40))
-          return;
-        downloads.onChanged.removeListener(changedListener);
-        changedCompleted();
-      }
-      downloads.onChanged.addListener(changedListener);
-
-      downloads.download(
-          {'url': NEVER_FINISH_URL},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
-    function downloadOnChanged() {
-      // Test that download completion is detectable by an onChanged event
-      // listener.
-      var downloadId = getNextId();
-      var callbackCompleted = chrome.test.callbackAdded();
-      function myListener(delta) {
-        if ((delta.id != downloadId) ||
-            !delta.state ||
-            (delta.state.new != downloads.STATE_COMPLETE))
-          return;
-        downloads.onChanged.removeListener(myListener);
-        callbackCompleted();
-      }
-      downloads.onChanged.addListener(myListener);
-      downloads.download(
-        {"url": SAFE_FAST_URL},
-        chrome.test.callback(function(id) {
-          chrome.test.assertEq(downloadId, id);
-      }));
-    },
-
     function downloadFilename() {
-      // Test that we can suggest a filename for a new download, and test that
-      // we can detect filename changes with an onChanged event listener.
-      var FILENAME = 'owiejtoiwjrfoiwjroiwjroiwjroiwjrfi';
-      var downloadId = getNextId();
-      var callbackCompleted = chrome.test.callbackAdded();
-      function myListener(delta) {
-        if ((delta.id != downloadId) ||
-            !delta.filename ||
-            (delta.filename.new.indexOf(FILENAME) == -1))
-          return;
-        downloads.onChanged.removeListener(myListener);
-        callbackCompleted();
-      }
-      downloads.onChanged.addListener(myListener);
       downloads.download(
-          {'url': SAFE_FAST_URL, 'filename': FILENAME},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
+          {'url': SAFE_FAST_URL, 'filename': 'foo'},
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
           }));
+      // TODO(benjhayden): Test the filename using onChanged.
     },
-
     function downloadOnCreated() {
-      // Test that the onCreated event fires when we start a download.
-      var downloadId = getNextId();
-      var createdCompleted = chrome.test.callbackAdded();
-      function createdListener(item) {
-        if (item.id == downloadId) {
-          createdCompleted();
-          downloads.onCreated.removeListener(createdListener);
-        }
-      };
-      downloads.onCreated.addListener(createdListener);
+      chrome.test.listenOnce(downloads.onCreated,
+                             chrome.test.callbackPass(function(item) {}));
       downloads.download(
           {'url': SAFE_FAST_URL},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
+          function(id) {
+            chrome.test.assertEq(getNextId(), id);
+          });
+    },
+    function downloadSubDirectoryFilename() {
+      downloads.download(
+          {'url': SAFE_FAST_URL, 'filename': 'foo/slow'},
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
           }));
+      // TODO(benjhayden): Test the filename using onChanged.
     },
-
     function downloadInvalidFilename() {
-      // Test that we disallow invalid filenames for new downloads.
       downloads.download(
           {'url': SAFE_FAST_URL, 'filename': '../../../../../etc/passwd'},
-          chrome.test.callbackFail(downloads.ERROR_GENERIC));
+          chrome.test.callbackFail(ERROR_GENERIC));
+      // TODO(benjhayden): Give a better error message.
     },
-
     function downloadEmpty() {
       assertThrows(('Invalid value for argument 1. Property \'url\': ' +
                     'Property is required.'),
                    downloads.download, {});
     },
-
     function downloadInvalidSaveAs() {
       assertThrows(('Invalid value for argument 1. Property \'saveAs\': ' +
                     'Expected \'boolean\' but got \'string\'.'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'saveAs': 'GOAT'});
     },
-
     function downloadInvalidHeadersOption() {
       assertThrows(('Invalid value for argument 1. Property \'headers\': ' +
                     'Expected \'array\' but got \'string\'.'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'headers': 'GOAT'});
     },
-
     function downloadInvalidURL() {
-      // Test that download() requires a valid url.
       downloads.download(
           {'url': 'foo bar'},
-          chrome.test.callbackFail(downloads.ERROR_INVALID_URL));
+          chrome.test.callbackFail(ERROR_INVALID_URL));
     },
-
     function downloadInvalidMethod() {
       assertThrows(('Invalid value for argument 1. Property \'method\': ' +
                     'Value must be one of: [GET, POST].'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'method': 'GOAT'});
     },
-
+    function downloadSimple() {
+      downloads.download(
+          {'url': SAFE_FAST_URL},
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
+          }));
+    },
+    function downloadPost() {
+      downloads.download(
+          {'url': getURL('files/post/downloads/a_zip_file.js'),
+           'method': 'POST',
+           'body': 'WOOHOO'},
+          chrome.test.callbackPass(function(id) {
+              chrome.test.assertEq(getNextId(), id);
+          }));
+    },
+    function downloadHeader() {
+      downloads.download(
+          {'url': SAFE_FAST_URL,
+           'headers': [{'name': 'Foo', 'value': 'bar'}]
+          },
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
+          }));
+    },
+    function downloadInterrupted() {
+      // TODO(benjhayden): Find a suitable URL and test that this id is
+      // eventually interrupted using onChanged.
+      downloads.download(
+          {'url': SAFE_FAST_URL},
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
+          }));
+    },
     function downloadInvalidHeader() {
-      // Test that download() disallows setting the Cookie header.
       downloads.download(
           {'url': SAFE_FAST_URL,
            'headers': [{ 'name': 'Cookie', 'value': 'fake'}]
         },
-        chrome.test.callbackFail(downloads.ERROR_GENERIC));
+        chrome.test.callbackFail(ERROR_GENERIC));
+      // TODO(benjhayden): Give a better error message.
     },
-
     function downloadGetFileIconInvalidOptions() {
       assertThrows(('Invalid value for argument 2. Property \'cat\': ' +
                     'Unexpected property.'),
                    downloads.getFileIcon,
                    -1, {cat: 'mouse'});
     },
-
     function downloadGetFileIconInvalidSize() {
       assertThrows(('Invalid value for argument 2. Property \'size\': ' +
                     'Value must be one of: [16, 32].'),
                    downloads.getFileIcon, -1, {size: 31});
     },
-
     function downloadGetFileIconInvalidId() {
       downloads.getFileIcon(-42, {size: 32},
-        chrome.test.callbackFail(downloads.ERROR_INVALID_OPERATION));
+        chrome.test.callbackFail(ERROR_INVALID_OPERATION));
+    },
+    function downloadNoComplete() {
+      // This is used partly to test cleanUp.
+      downloads.download(
+          {'url': NEVER_FINISH_URL},
+          chrome.test.callbackPass(function(id) {
+            chrome.test.assertEq(getNextId(), id);
+          }));
     },
-
     function downloadPauseInvalidId() {
-      downloads.pause(-42, chrome.test.callbackFail(
-            downloads.ERROR_INVALID_OPERATION));
+      downloads.pause(-42, chrome.test.callbackFail(ERROR_INVALID_OPERATION));
     },
-
     function downloadPauseInvalidType() {
       assertThrows(('Invalid value for argument 1. Expected \'integer\' ' +
                     'but got \'string\'.'),
                    downloads.pause,
                    'foo');
     },
-
     function downloadResumeInvalidId() {
-      downloads.resume(-42, chrome.test.callbackFail(
-            downloads.ERROR_INVALID_OPERATION));
+      downloads.resume(-42, chrome.test.callbackFail(ERROR_INVALID_OPERATION));
     },
-
     function downloadResumeInvalidType() {
       assertThrows(('Invalid value for argument 1. Expected \'integer\' ' +
                     'but got \'string\'.'),
                    downloads.resume,
                    'foo');
     },
-
     function downloadCancelInvalidId() {
       // Canceling a non-existent download is not considered an error.
-      downloads.cancel(-42, chrome.test.callback(function() {}));
+      downloads.cancel(-42, chrome.test.callbackPass(function() {}));
     },
-
     function downloadCancelInvalidType() {
       assertThrows(('Invalid value for argument 1. Expected \'integer\' ' +
                     'but got \'string\'.'),
                    downloads.cancel, 'foo');
     },
-
-    function downloadNoComplete() {
-      // This is used partly to test cleanUp.
-      var downloadId = getNextId();
-      downloads.download(
-          {'url': NEVER_FINISH_URL},
-          chrome.test.callback(function(id) {
-            chrome.test.assertEq(downloadId, id);
-          }));
-    },
-
     function cleanUp() {
       // cleanUp must come last. It clears out all in-progress downloads
       // so the browser can shutdown cleanly.
       for (var id = 0; id < nextId; ++id) {
-        downloads.cancel(id, chrome.test.callback(function() {}));
+        downloads.cancel(id, chrome.test.callbackPass(function() {}));
       }
     }
   ]);
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index f280333..12882b5 100755
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -909,6 +909,9 @@ class TestPageHandler(BasePageHandler):
     prefix = self.server.file_root_url
     if not self.path.startswith(prefix):
       return False
+    # Consume a request body if present.
+    if self.command == 'POST' or self.command == 'PUT' :
+      self.ReadRequestBody()
     return self._FileHandlerHelper(prefix)
 
   def PostOnlyFileHandler(self):
@@ -916,33 +919,12 @@ class TestPageHandler(BasePageHandler):
     prefix = urlparse.urljoin(self.server.file_root_url, 'post/')
     if not self.path.startswith(prefix):
       return False
+    self.ReadRequestBody()
     return self._FileHandlerHelper(prefix)
 
   def _FileHandlerHelper(self, prefix):
-    request_body = ''
-    if self.command == 'POST' or self.command == 'PUT':
-      # Consume a request body if present.
-      request_body = self.ReadRequestBody()
-
+    old_protocol_version = self.protocol_version
     _, _, url_path, _, query, _ = urlparse.urlparse(self.path)
-    query_dict = cgi.parse_qs(query)
-
-    expected_body = query_dict.get('expected_body', [])
-    if expected_body and request_body not in expected_body:
-      self.send_response(404)
-      self.end_headers()
-      self.wfile.write('')
-      return True
-
-    expected_headers = query_dict.get('expected_headers', [])
-    for expected_header in expected_headers:
-      header_name, expected_value = expected_header.split(':')
-      if self.headers.getheader(header_name) != expected_value:
-        self.send_response(404)
-        self.end_headers()
-        self.wfile.write('')
-        return True
-
     sub_path = url_path[len(prefix):]
     entries = sub_path.split('/')
     file_path = os.path.join(self.server.data_dir, *entries)
@@ -960,8 +942,6 @@ class TestPageHandler(BasePageHandler):
 
     data = self._ReplaceFileData(data, query)
 
-    old_protocol_version = self.protocol_version
-
     # If file.mock-http-headers exists, it contains the headers we
     # should send.  Read them in and parse them.
     headers_path = file_path + '.mock-http-headers'