Bug 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn

toolkit/components/jsdownloads/src/DownloadCore.jsm

@@ -1536,15 +1536,25 @@ DownloadLegacySaver.prototype = {
           aSetProgressBytesFn(0, this.request.contentLength);
         }
 
-        // The download implementation may not have created the target file if
-        // no data was received from the source.  In this case, ensure that an
-        // empty file is created as expected.
-        try {
-          // This atomic operation is more efficient than an existence check.
-          let file = yield OS.File.open(this.download.target.path,
-                                        { create: true });
-          yield file.close();
-        } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) { }
+        // If the component executing the download provides the path of a
+        // ".part" file, it means that it expects the listener to move the file
+        // to its final target path when the download succeeds.  In this case,
+        // an empty ".part" file is created even if no data was received from
+        // the source.
+        if (this.download.target.partFilePath) {
+          yield OS.File.move(this.download.target.partFilePath,
+                             this.download.target.path);
+        } else {
+          // The download implementation may not have created the target file if
+          // no data was received from the source.  In this case, ensure that an
+          // empty file is created as expected.
+          try {
+            // This atomic operation is more efficient than an existence check.
+            let file = yield OS.File.open(this.download.target.path,
+                                          { create: true });
+            yield file.close();
+          } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) { }
+        }
       } finally {
         // We don't need the reference to the request anymore.
         this.request = null;

toolkit/components/jsdownloads/test/unit/common_test_Download.js

@@ -204,6 +204,23 @@ add_task(function test_basic()
   yield promiseVerifyContents(download.target.path, TEST_DATA_SHORT);
 });
 
+/**
+ * Executes a download with the tryToKeepPartialData property set, and ensures
+ * that the file is saved correctly.  When testing DownloadLegacySaver, the
+ * download is executed using the nsIExternalHelperAppService component.
+ */
+add_task(function test_basic_tryToKeepPartialData()
+{
+  let download = yield promiseStartDownload_tryToKeepPartialData();
+  continueResponses();
+  yield promiseDownloadStopped(download);
+
+  // The target file should now have been created, and the ".part" file deleted.
+  yield promiseVerifyContents(download.target.path,
+                              TEST_DATA_SHORT + TEST_DATA_SHORT);
+  do_check_false(yield OS.File.exists(download.target.partFilePath));
+});
+
 /**
  * Checks the referrer for downloads.
  */
@@ -360,6 +377,36 @@ add_task(function test_empty_progress()
   do_check_eq((yield OS.File.stat(download.target.path)).size, 0);
 });
 
+/**
+ * Downloads a file with a "Content-Length" of 0 with the tryToKeepPartialData
+ * property set, and ensures that the file is saved correctly.
+ */
+add_task(function test_empty_progress_tryToKeepPartialData()
+{
+  // Start a new download and configure it to keep partially downloaded data.
+  let download;
+  if (!gUseLegacySaver) {
+    let targetFilePath = getTempFile(TEST_TARGET_FILE_NAME).path;
+    download = yield Downloads.createDownload({
+      source: httpUrl("empty.txt"),
+      target: { path: targetFilePath,
+                partFilePath: targetFilePath + ".part" },
+    });
+    download.tryToKeepPartialData = true;
+    download.start();
+  } else {
+    // Start a download using nsIExternalHelperAppService, that is configured
+    // to keep partially downloaded data by default.
+    download = yield promiseStartExternalHelperAppServiceDownload(
+                                                         httpUrl("empty.txt"));
+  }
+  yield promiseDownloadStopped(download);
+
+  // The target file should now have been created, and the ".part" file deleted.
+  do_check_eq((yield OS.File.stat(download.target.path)).size, 0);
+  do_check_false(yield OS.File.exists(download.target.partFilePath));
+});
+
 /**
  * Downloads an empty file with no "Content-Length" and checks the progress.
  */

toolkit/components/jsdownloads/test/unit/head.js

@@ -305,13 +305,18 @@ function promiseStartLegacyDownload(aSourceUrl, aOptions) {
  * it using the legacy nsITransfer interface.  The source of the download will
  * be "interruptible_resumable.txt" and partially downloaded data will be kept.
  *
+ * @param aSourceUrl
+ *        String containing the URI for the download source, or null to use
+ *        httpUrl("interruptible_resumable.txt").
+ *
  * @return {Promise}
  * @resolves The Download object created as a consequence of controlling the
  *           download through the legacy nsITransfer interface.
  * @rejects Never.  The current test fails in case of exceptions.
  */
-function promiseStartExternalHelperAppServiceDownload() {
-  let sourceURI = NetUtil.newURI(httpUrl("interruptible_resumable.txt"));
+function promiseStartExternalHelperAppServiceDownload(aSourceUrl) {
+  let sourceURI = NetUtil.newURI(aSourceUrl ||
+                                 httpUrl("interruptible_resumable.txt"));
 
   let deferred = Promise.defer();