Bug 1245600 - Implement chrome.downloads.onChanged for state r?kmag draft
authorAndrew Swan <aswan@mozilla.com>
Fri, 04 Mar 2016 12:18:11 -0800
changeset 337162 425b8dfc57e2ec54dc55626af2f92f4b4cf27754
parent 337011 b479a3b0fde9a3dfc67dfb6dbf35d3c958fd1a9c
child 515596 99eb3912fac2765a21efd230c1f6f14b6f244419
push id12283
push useraswan@mozilla.com
push dateSat, 05 Mar 2016 03:26:47 +0000
reviewerskmag
bugs1245600
milestone47.0a1
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) {