Bug 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Wed, 07 Aug 2013 13:09:04 +0200
changeset 154571 a0b47189f0a60f5bf189fc1332cd325fbe652f6f
parent 154570 daf88d34960c94fcbd77e9eff76db9d2c4b7b1bb
child 154572 74f983165c02cd0fca42412157d080d443f42671
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn
bugs901563
milestone26.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 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn
toolkit/components/jsdownloads/src/DownloadCore.jsm
toolkit/components/jsdownloads/test/unit/common_test_Download.js
toolkit/components/jsdownloads/test/unit/head.js
--- a/toolkit/components/jsdownloads/src/DownloadCore.jsm
+++ b/toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ -1531,25 +1531,35 @@ DownloadLegacySaver.prototype = {
         // report the value of "Content-Length", if available, even if the
         // download didn't generate any progress events.
         if (!this.progressWasNotified &&
             this.request instanceof Ci.nsIChannel &&
             this.request.contentLength >= 0) {
           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;
         // Allow the download to restart through a DownloadCopySaver.
         this.firstExecutionFinished = true;
       }
     }.bind(this));
   },
--- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js
+++ b/toolkit/components/jsdownloads/test/unit/common_test_Download.js
@@ -200,16 +200,33 @@ add_task(function test_basic()
 
   // Check additional properties on the finished download.
   do_check_true(download.source.referrer === null);
 
   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.
  */
 add_task(function test_referrer()
 {
   let sourcePath = "/test_referrer.txt";
   let sourceUrl = httpUrl("test_referrer.txt");
   let targetPath = getTempFile(TEST_TARGET_FILE_NAME).path;
 
@@ -356,16 +373,46 @@ add_task(function test_empty_progress()
 
   // We should have received the content type even for an empty file.
   do_check_eq(download.contentType, "text/plain");
 
   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.
  */
 add_task(function test_empty_noprogress()
 {
   let sourcePath = "/test_empty_noprogress.txt";
   let sourceUrl = httpUrl("test_empty_noprogress.txt");
   let deferRequestReceived = Promise.defer();
 
--- a/toolkit/components/jsdownloads/test/unit/head.js
+++ b/toolkit/components/jsdownloads/test/unit/head.js
@@ -300,23 +300,28 @@ function promiseStartLegacyDownload(aSou
   return deferred.promise;
 }
 
 /**
  * Starts a new download using the nsIHelperAppService interface, and controls
  * 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();
 
   Downloads.getPublicDownloadList().then(function (aList) {
     // Temporarily register a view that will get notified when the download we
     // are controlling becomes visible in the list of downloads.
     aList.addView({
       onDownloadAdded: function (aDownload) {