From 54a6b3b931bae541432f5796aec3a9fba79bb3b9 Mon Sep 17 00:00:00 2001 From: Paolo Amadini Date: Wed, 7 Aug 2013 13:09:04 +0200 Subject: [PATCH] Bug 901563 - DownloadLegacySaver should move the ".part" file to the target. r=enn --- .../jsdownloads/src/DownloadCore.jsm | 28 +++++++---- .../test/unit/common_test_Download.js | 47 +++++++++++++++++++ .../components/jsdownloads/test/unit/head.js | 9 +++- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/toolkit/components/jsdownloads/src/DownloadCore.jsm b/toolkit/components/jsdownloads/src/DownloadCore.jsm index e3dbe109e0c4..cb34f66ff1d9 100644 --- a/toolkit/components/jsdownloads/src/DownloadCore.jsm +++ b/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; diff --git a/toolkit/components/jsdownloads/test/unit/common_test_Download.js b/toolkit/components/jsdownloads/test/unit/common_test_Download.js index 6638982272c3..962392702a53 100644 --- a/toolkit/components/jsdownloads/test/unit/common_test_Download.js +++ b/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. */ diff --git a/toolkit/components/jsdownloads/test/unit/head.js b/toolkit/components/jsdownloads/test/unit/head.js index 181652a17bc7..a7bed79d8f5e 100644 --- a/toolkit/components/jsdownloads/test/unit/head.js +++ b/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();