Bug 1245600 - Implement chrome.downloads.onChanged for state. r=kmag
authorAndrew Swan <aswan@mozilla.com>
Fri, 04 Mar 2016 12:18:11 -0800
changeset 325104 0c220ff8ce852e1c9a53cdbfb9989e8e4b7bbe69
parent 325103 9cc5bb93e4e9bf299d69b9619c59ea29f1ac9a7e
child 325105 d298fc97819e99e21d242dd6ff1c143771985493
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1245600
milestone47.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1245600 - Implement chrome.downloads.onChanged for state. r=kmag MozReview-Commit-ID: BaAyU1dgMB7
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/schemas/downloads.json
toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -9,34 +9,39 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "DownloadPaths",
                                   "resource://gre/modules/DownloadPaths.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OS",
                                   "resource://gre/modules/osfile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
                                   "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
                                   "resource://gre/modules/NetUtil.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter",
+                                  "resource://devtools/shared/event-emitter.js");
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 const {
   ignoreEvent,
+  runSafeSync,
+  SingletonEventManager,
 } = ExtensionUtils;
 
 const DOWNLOAD_ITEM_FIELDS = ["id", "url", "referrer", "filename", "incognito",
                               "danger", "mime", "startTime", "endTime",
                               "estimatedEndTime", "state", "canResume",
                               "error", "bytesReceived", "totalBytes",
                               "fileSize", "exists",
                               "byExtensionId", "byExtensionName"];
 
 class DownloadItem {
   constructor(id, download, extension) {
     this.id = id;
     this.download = download;
     this.extension = extension;
+    this.prechange = {};
   }
 
   get url() { return this.download.source.url; }
   get referrer() { return this.download.source.referrer; }
   get filename() { return this.download.target.path; }
   get incognito() { return this.download.source.isPrivate; }
   get danger() { return "safe"; } // TODO
   get mime() { return this.download.contentType; }
@@ -98,16 +103,26 @@ class DownloadItem {
     for (let field of DOWNLOAD_ITEM_FIELDS) {
       obj[field] = this[field];
     }
     if (obj.startTime) {
       obj.startTime = obj.startTime.toISOString();
     }
     return obj;
   }
+
+  // When a change event fires, handlers can look at how an individual
+  // field changed by comparing item.fieldname with item.prechange.fieldname.
+  // After all handlers have been invoked, this gets called to store the
+  // current values of all fields ahead of the next event.
+  _change() {
+    for (let field of DOWNLOAD_ITEM_FIELDS) {
+      this.prechange[field] = this[field];
+    }
+  }
 }
 
 
 // DownloadMap maps back and forth betwen the numeric identifiers used in
 // the downloads WebExtension API and a Download object from the Downloads jsm.
 // todo: make id and extension info persistent (bug 1247794)
 const DownloadMap = {
   currentId: 0,
@@ -116,30 +131,41 @@ const DownloadMap = {
   // Maps numeric id -> DownloadItem
   byId: new Map(),
 
   // Maps Download object -> DownloadItem
   byDownload: new WeakMap(),
 
   lazyInit() {
     if (this.loadPromise == null) {
+      EventEmitter.decorate(this);
       this.loadPromise = Downloads.getList(Downloads.ALL).then(list => {
         let self = this;
         return list.addView({
           onDownloadAdded(download) {
             self.newFromDownload(download, null);
           },
 
           onDownloadRemoved(download) {
             const item = self.byDownload.get(download);
             if (item != null) {
               self.byDownload.delete(download);
               self.byId.delete(item.id);
             }
           },
+
+          onDownloadChanged(download) {
+            const item = self.byDownload.get(download);
+            if (item == null) {
+              Cu.reportError("Got onDownloadChanged for unknown download object");
+            } else {
+              self.emit("change", item);
+              item._change();
+            }
+          },
         }).then(() => list.getAll())
           .then(downloads => {
             downloads.forEach(download => {
               this.newFromDownload(download, null);
             });
           })
           .then(() => list);
       });
@@ -417,15 +443,37 @@ extensions.registerSchemaAPI("downloads"
       // open(downloadId) {
       //   if (!extension.hasPermission("downloads.open")) {
       //     throw new context.cloneScope.Error("Permission denied because 'downloads.open' permission is missing.");
       //   }
       //   ...
       // }
       // likewise for setShelfEnabled() and the "download.shelf" permission
 
+      onChanged: new SingletonEventManager(context, "downloads.onChanged", fire => {
+        const handler = (what, item) => {
+          if (item.state != item.prechange.state) {
+            runSafeSync(context, fire, {
+              id: item.id,
+              state: {
+                previous: item.prechange.state || null,
+                current: item.state,
+              },
+            });
+          }
+        };
+
+        let registerPromise = DownloadMap.getDownloadList().then(() => {
+          DownloadMap.on("change", handler);
+        });
+        return () => {
+          registerPromise.then(() => {
+            DownloadMap.off("change", handler);
+          });
+        };
+      }).api(),
+
       onCreated: ignoreEvent(context, "downloads.onCreated"),
       onErased: ignoreEvent(context, "downloads.onErased"),
-      onChanged: ignoreEvent(context, "downloads.onChanged"),
       onDeterminingFilename: ignoreEvent(context, "downloads.onDeterminingFilename"),
     },
   };
 });
--- a/toolkit/components/extensions/schemas/downloads.json
+++ b/toolkit/components/extensions/schemas/downloads.json
@@ -684,17 +684,16 @@
             "type": "integer"
           }
         ],
         "type": "function"
       },
       {
         "name": "onChanged",
         "description": "When any of a <a href='#type-DownloadItem'>DownloadItem</a>'s properties except <code>bytesReceived</code> changes, this event fires with the <code>downloadId</code> and an object containing the properties that changed.",
-        "unsupported": true,
         "parameters": [
           {
             "name": "downloadDelta",
             "type": "object",
             "properties": {
               "id": {
                 "description": "The <code>id</code> of the <a href='#type-DownloadItem'>DownloadItem</a> that changed.",
                 "type": "integer"
--- a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html
+++ b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_search.html
@@ -27,16 +27,38 @@ const TXT_FILE = "file_download.txt";
 const TXT_URL = BASE + "/" + TXT_FILE;
 const TXT_LEN = 46;
 const HTML_FILE = "file_download.html";
 const HTML_URL = BASE + "/" + HTML_FILE;
 const HTML_LEN = 117;
 const BIG_LEN = 1000;  // something bigger both TXT_LEN and HTML_LEN
 
 function backgroundScript() {
+  let complete = new Map();
+
+  function waitForComplete(id) {
+    if (complete.has(id)) {
+      return complete.get(id).promise;
+    }
+
+    let promise = new Promise(resolve => {
+      complete.set(id, {resolve});
+    });
+    complete.get(id).promise = promise;
+    return promise;
+  }
+
+  browser.downloads.onChanged.addListener(change => {
+    if (change.state && change.state.current == "complete") {
+      // Make sure we have a promise.
+      waitForComplete(change.id);
+      complete.get(change.id).resolve();
+    }
+  });
+
   browser.test.onMessage.addListener(function(msg) {
     // extension functions throw on bad arguments, we can remove the extra
     // promise when bug 1250223 is fixed.
     if (msg == "download.request") {
       Promise.resolve().then(() => browser.downloads.download(arguments[1]))
                        .then(id => {
                          browser.test.sendMessage("download.done", {status: "success", id});
                        })
@@ -46,44 +68,35 @@ function backgroundScript() {
     } else if (msg == "search.request") {
       Promise.resolve().then(() => browser.downloads.search(arguments[1]))
                        .then(downloads => {
                          browser.test.sendMessage("search.done", {status: "success", downloads});
                        })
                        .catch(error => {
                          browser.test.sendMessage("search.done", {status: "error", errmsg: error.message});
                        });
+    } else if (msg == "waitForComplete.request") {
+      waitForComplete(arguments[1]).then(() => {
+        browser.test.sendMessage("waitForComplete.done");
+      });
     }
   });
 
   browser.test.sendMessage("ready");
 }
 
 function clearDownloads(callback) {
   return Downloads.getList(Downloads.ALL).then(list => {
     return list.getAll().then(downloads => {
       return Promise.all(downloads.map(download => list.remove(download)))
                     .then(() => downloads);
     });
   });
 }
 
-// This function is a bit of a sledgehammer, it looks at every download
-// the browser knows about and waits for all active downloads to complete.
-// But we only start one at a time and only do a handful in total.
-// Replace this when we have onChanged (bug 1245600)
-function waitForDownloads() {
-  return Downloads.getList(Downloads.ALL)
-                  .then(list => list.getAll())
-                  .then(downloads => {
-                    let inprogress = downloads.filter(dl => !dl.stopped);
-                    return Promise.all(inprogress.map(dl => dl.whenSucceeded()));
-                  });
-}
-
 add_task(function* test_search() {
   const nsIFile = Ci.nsIFile;
   let downloadDir = FileUtils.getDir("TmpD", ["downloads"]);
   downloadDir.createUnique(nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
   info(`downloadDir ${downloadDir.path}`);
 
   function downloadPath(filename) {
     let path = downloadDir.clone();
@@ -109,17 +122,27 @@ add_task(function* test_search() {
     background: `(${backgroundScript})()`,
     manifest: {
       permissions: ["downloads"],
     },
   });
 
   function download(options) {
     extension.sendMessage("download.request", options);
-    return extension.awaitMessage("download.done");
+    return extension.awaitMessage("download.done").then(result => {
+      let promise;
+      if (result.status == "success") {
+        info(`wait for onChanged event to indicate ${result.id} is complete`);
+        extension.sendMessage("waitForComplete.request", result.id);
+        promise = extension.awaitMessage("waitForComplete.done");
+      } else {
+        promise = Promise.resolve();
+      }
+      return promise.then(() => result);
+    });
   }
 
   function search(query) {
     extension.sendMessage("search.request", query);
     return extension.awaitMessage("search.done");
   }
 
   yield extension.startup();
@@ -147,18 +170,16 @@ add_task(function* test_search() {
 
   const HTML_FILE2 = "renamed.html";
   msg = yield download({url: HTML_URL, filename: HTML_FILE2});
   is(msg.status, "success", "download() succeeded");
   downloadIds.html2 = msg.id;
 
   const time3 = new Date();
 
-  yield waitForDownloads();
-
   // Search for each individual download and check
   // the corresponding DownloadItem.
   function* checkDownloadItem(id, expect) {
     let msg = yield search({id});
     is(msg.status, "success", "search() succeeded");
     is(msg.downloads.length, 1, "search() found exactly 1 download");
 
     Object.keys(expect).forEach(function(field) {