Bug 1254327 (Part 2) Use filename callback from chrome.downloads.download() r?kmag draft
authorAndrew Swan <aswan@mozilla.com>
Tue, 29 Mar 2016 15:27:14 -0700
changeset 345691 a6ccd1e564097780b1bd8d8b65dda32182c2d64b
parent 345690 300a334f49a62653cc9513dc830ebc3f3ce894b3
child 517240 379e6a1b53ba0c1c421914ee1034f65f3e5ec898
push id14141
push useraswan@mozilla.com
push dateWed, 30 Mar 2016 03:01:53 +0000
reviewerskmag
bugs1254327
milestone48.0a1
Bug 1254327 (Part 2) Use filename callback from chrome.downloads.download() r?kmag MozReview-Commit-ID: 6W4U2Uy1IVD
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/test/mochitest/chrome.ini
toolkit/components/extensions/test/mochitest/redirect.sjs
toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -37,17 +37,23 @@ const DOWNLOAD_ITEM_CHANGE_FIELDS = ["en
                                      "error", "exists"];
 
 
 class DownloadItem {
   constructor(id, download, extension) {
     this.id = id;
     this.download = download;
     this.extension = extension;
-    this.prechange = {};
+    this.prechange = {
+      state: null,
+      paused: false,
+      canResume: false,
+      error: null,
+      exists: false,
+    };
   }
 
   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; }
@@ -164,21 +170,17 @@ const DownloadMap = {
             }
           },
 
           onDownloadChanged(download) {
             const item = self.byDownload.get(download);
             if (item == null) {
               Cu.reportError("Got onDownloadChanged for unknown download object");
             } else {
-              // We get the first one of these when the download is started.
-              // In this case, don't emit anything, just initialize prechange.
-              if (Object.keys(item.prechange).length > 0) {
-                self.emit("change", item);
-              }
+              self.emit("change", item);
               item._change();
             }
           },
         }).then(() => list.getAll())
           .then(downloads => {
             downloads.forEach(download => {
               this.newFromDownload(download, null);
             });
@@ -412,67 +414,78 @@ extensions.registerSchemaAPI("downloads"
           }
         }
 
         if (options.conflictAction == "prompt") {
           // TODO
           return Promise.reject({message: "conflictAction prompt not yet implemented"});
         }
 
-        function createTarget(downloadsDir) {
-          // TODO
-          // if (options.saveAs) { }
-
-          let target;
-          if (options.filename) {
-            target = OS.Path.join(downloadsDir, options.filename);
-          } else {
-            let uri = NetUtil.newURI(options.url).QueryInterface(Ci.nsIURL);
-            target = OS.Path.join(downloadsDir, uri.fileName);
-          }
+        // The flow of control here is a little messy...  Creating the
+        // download is an asynchronous operation which calls createTarget()
+        // asynchronously.  We need to wait for createTarget() to complete
+        // before adding the new download object to a DownloadList (this is
+        // a constraint of DownloadList).  But since we don't directly call
+        // createTarget(), we have to wrap it with targetPromise to find out
+        // when it is done.
+        let createTarget;
+        let targetPromise = new Promise(resolve => {
+          createTarget = function(downloadsDir, request) {
+            // TODO
+            // if (options.saveAs) { }
 
-          // This has a race, something else could come along and create
-          // the file between this test and them time the download code
-          // creates the target file.  But we can't easily fix it without
-          // modifying DownloadCore so we live with it for now.
-          return OS.File.exists(target).then(exists => {
-            if (exists) {
-              switch (options.conflictAction) {
-                case "uniquify":
-                default:
-                  target = DownloadPaths.createNiceUniqueFile(new FileUtils.File(target)).path;
-                  break;
+            let target;
+            if (options.filename) {
+              target = OS.Path.join(downloadsDir, options.filename);
+            } else {
+              let uri = request.URI;
+              uri.QueryInterface(Ci.nsIURL);
+              let filename = uri.fileName;
+              if (filename == "") {
+                filename = "download";
+              }
+              target = OS.Path.join(downloadsDir, filename);
+            }
 
-                case "overwrite":
-                  break;
-              }
-            }
-            return target;
-          });
-        }
+            let promise = OS.File.exists(target).then(exists => {
+              if (exists) {
+                switch (options.conflictAction) {
+                  case "uniquify":
+                  default:
+                    target = DownloadPaths.createNiceUniqueFile(new FileUtils.File(target)).path;
+                    break;
 
-        let download;
-        return Downloads.getPreferredDownloadsDirectory()
-          .then(downloadsDir => createTarget(downloadsDir))
-          .then(target => Downloads.createDownload({
-            source: options.url,
-            target: {
-              path: target,
-              partFilePath: target + ".part",
-            },
-          })).then(dl => {
-            download = dl;
-            return DownloadMap.getDownloadList();
-          }).then(list => {
+                  case "overwrite":
+                    break;
+                }
+              }
+              return target;
+            });
+            promise.then(resolve);
+            return promise;
+          };
+        });
+
+        let downloadPromise = Downloads.getPreferredDownloadsDirectory()
+            .then(downloadsDir => Downloads.createDownload({
+              source: options.url,
+              target: {
+                path: downloadsDir,
+                filenameCallback: req => createTarget(downloadsDir, req),
+              },
+            })).then(download => {
+              // This is necessary to make pause/resume work.
+              download.tryToKeepPartialData = true;
+              download.start();
+              return download;
+            });
+
+        return Promise.all([downloadPromise, targetPromise, DownloadMap.getDownloadList()])
+          .then(([download, unused, list]) => {
             list.add(download);
-
-            // This is necessary to make pause/resume work.
-            download.tryToKeepPartialData = true;
-            download.start();
-
             const item = DownloadMap.newFromDownload(download, extension);
             return item.id;
           });
       },
 
       removeFile(id) {
         return DownloadMap.lazyInit().then(() => {
           let item;
--- a/toolkit/components/extensions/test/mochitest/chrome.ini
+++ b/toolkit/components/extensions/test/mochitest/chrome.ini
@@ -1,13 +1,14 @@
 [DEFAULT]
 support-files =
   file_download.html
   file_download.txt
   interruptible.sjs
+  redirect.sjs
   file_sample.html
 
 [test_chrome_ext_downloads_download.html]
 [test_chrome_ext_downloads_misc.html]
 skip-if = 1 # Currently causes too many intermittent failures.
 [test_chrome_ext_downloads_search.html]
 [test_chrome_ext_eventpage_warning.html]
 [test_chrome_ext_contentscript_unrecognizedprop_warning.html]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/mochitest/redirect.sjs
@@ -0,0 +1,10 @@
+const TARGET = "file_download.txt";
+
+// A handler that generates an HTTP redirect to TARGET
+function handleRequest(request, response) {
+  response.setStatusLine(request.httpVersion, 301, "Moved Permanently");
+
+  let newpath = request.path.slice(0, request.path.lastIndexOf('/'));
+  let target = `${request.scheme}://${request.host}:${request.port}${newpath}/${TARGET}`;
+  response.setHeader("Location", target, false);
+}
--- a/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
+++ b/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_download.html
@@ -126,16 +126,19 @@ add_task(function* test_downloads() {
   yield testDownload({url: FILE_URL}, FILE_NAME, FILE_LEN, "just source");
 
   // Call download() with a filename property.
   yield testDownload({
     url: FILE_URL,
     filename: "newpath.txt",
   }, "newpath.txt", FILE_LEN, "source and filename");
 
+  // Call download() without a filename on a URL that is redirected
+  yield testDownload({url: BASE + "/redirect.sjs"}, FILE_NAME, FILE_LEN, "redirected source");
+
   // Check conflictAction of "uniquify".
   touch(FILE_NAME);
   yield testDownload({
     url: FILE_URL,
     conflictAction: "uniquify",
   }, FILE_NAME_UNIQUE, FILE_LEN, "conflictAction=uniquify");
   // todo check that preexisting file was not modified?
   remove(FILE_NAME);