Bug 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn a=gavin
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 06 Aug 2013 07:16:00 +0200
changeset 153709 d3e60eab3da57de72cb3d4d89cc8cfb738ff77f6
parent 153708 4d652113720b02df038cd87fdd51160e8191caa9
child 153710 87d11537f679b5482fc64337cff5c6a377b35bc1
push id2859
push userakeybl@mozilla.com
push dateMon, 16 Sep 2013 19:14:59 +0000
treeherdermozilla-beta@87d3c51cd2bf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn, gavin
bugs901563
milestone25.0a2
Bug 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn a=gavin
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) {