From 417ae4e540cfbb3570b2687c1544d59e73f640ce Mon Sep 17 00:00:00 2001 From: Steve McKay Date: Thu, 26 Feb 2015 12:03:43 -0800 Subject: [PATCH] Files.app: Fix task cancellation. - Fix the task queue so that cancelling a task doesn't leave it hanging out in the queue, which then will jam everything up. - Fix the error code so that errors are handled consistently (and are non-terminal, as they should be) - Rename UpdateType.SUCCESS to UpdateType.COMPLETE. This better reflects the fact that errors can occur during an import task but the task still runs to completion. BUG=460597 Review URL: https://codereview.chromium.org/947583002 Cr-Commit-Position: refs/heads/master@{#317538} (cherry picked from commit fb5551a709a997209e1504d0eacc8562c1740f03) TBR=mtomasz Review URL: https://codereview.chromium.org/964533003 Cr-Commit-Position: refs/branch-heads/2311@{#38} Cr-Branched-From: 09b7de5dd7254947cd4306de907274fa63373d48-refs/heads/master@{#317474} --- .../background/js/media_import_handler.js | 35 ++++---- .../js/media_import_handler_unittest.js | 81 +++++++++++++++++-- .../file_manager/background/js/task_queue.js | 7 +- .../background/js/task_queue_unittest.js | 49 ++++++++--- 4 files changed, 134 insertions(+), 38 deletions(-) diff --git a/ui/file_manager/file_manager/background/js/media_import_handler.js b/ui/file_manager/file_manager/background/js/media_import_handler.js index 9065e05b120e1..1ccb473db899e 100644 --- a/ui/file_manager/file_manager/background/js/media_import_handler.js +++ b/ui/file_manager/file_manager/background/js/media_import_handler.js @@ -109,8 +109,6 @@ importer.MediaImportHandler.prototype.onTaskProgress_ = item.id = task.taskId; // TODO(kenobi): Might need a different progress item type here. item.type = ProgressItemType.COPY; - item.message = - strf('CLOUD_IMPORT_ITEMS_REMAINING', task.remainingFilesCount); item.progressMax = task.totalBytes; item.cancelCallback = function() { task.requestCancel(); @@ -119,18 +117,20 @@ importer.MediaImportHandler.prototype.onTaskProgress_ = switch (updateType) { case UpdateType.PROGRESS: - item.progressValue = task.processedBytes; item.message = strf('CLOUD_IMPORT_ITEMS_REMAINING', task.remainingFilesCount); - break; - case UpdateType.SUCCESS: + item.progressValue = task.processedBytes; + item.state = ProgressItemState.PROGRESSING; + break; + case UpdateType.COMPLETE: item.message = ''; item.progressValue = item.progressMax; item.state = ProgressItemState.COMPLETED; break; case UpdateType.ERROR: - item.message = ''; - item.progressValue = item.progressMax; + item.message = + strf('CLOUD_IMPORT_ITEMS_REMAINING', task.remainingFilesCount); + item.progressValue = task.processedBytes; item.state = ProgressItemState.ERROR; break; case UpdateType.CANCELED: @@ -337,7 +337,8 @@ importer.MediaImportHandler.ImportTask.prototype.importOne_ = return this.copy_(entry, destinationDirectory); } }.bind(this)) - .then(completionCallback); + // Regardless of the result of this copy, push on to the next file. + .then(completionCallback, completionCallback); }; /** @@ -401,7 +402,12 @@ importer.MediaImportHandler.ImportTask.prototype.copy_ = /** @this {importer.MediaImportHandler.ImportTask} */ var onError = function(error) { this.cancelCallback_ = null; - this.onError_(error); + this.errorCount_++; + // Log the bytes as processed in spite of the error. This ensures + // completion of the progress bar. + this.processedBytes_ -= currentBytes; + this.processedBytes_ += entry.size; + this.notify(importer.TaskQueue.UpdateType.ERROR); resolver.reject(error); }; @@ -474,20 +480,11 @@ importer.MediaImportHandler.ImportTask.prototype.markAsImported_ = /** @private */ importer.MediaImportHandler.ImportTask.prototype.onSuccess_ = function() { - this.notify(importer.TaskQueue.UpdateType.SUCCESS); + this.notify(importer.TaskQueue.UpdateType.COMPLETE); this.tracker_.send(metrics.ImportEvents.ENDED); this.sendImportStats_(); }; -/** - * @param {DOMError} error - * @private - */ -importer.MediaImportHandler.ImportTask.prototype.onError_ = function(error) { - this.notify(importer.TaskQueue.UpdateType.ERROR); - this.errorCount_++; -}; - /** * Sends import statistics to analytics. */ diff --git a/ui/file_manager/file_manager/background/js/media_import_handler_unittest.js b/ui/file_manager/file_manager/background/js/media_import_handler_unittest.js index 35886a7b1fccb..3d0a374f71e2b 100644 --- a/ui/file_manager/file_manager/background/js/media_import_handler_unittest.js +++ b/ui/file_manager/file_manager/background/js/media_import_handler_unittest.js @@ -115,7 +115,7 @@ function testImportMedia(callback) { */ function(updateType, task) { switch (updateType) { - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: resolve(); break; case importer.TaskQueue.UpdateType.ERROR: @@ -162,7 +162,7 @@ function testImportMediaWithDuplicateFilenames(callback) { */ function(updateType, task) { switch (updateType) { - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: resolve(); break; case importer.TaskQueue.UpdateType.ERROR: @@ -210,7 +210,7 @@ function testKeepAwakeDuringImport(callback) { // Assert that keepAwake is set while the task is active. assertTrue(chrome.power.requestKeepAwakeStatus); switch (updateType) { - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: resolve(); break; case importer.TaskQueue.UpdateType.ERROR: @@ -253,7 +253,7 @@ function testUpdatesHistoryAfterImport(callback) { */ function(updateType, task) { switch (updateType) { - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: resolve(); break; case importer.TaskQueue.UpdateType.ERROR: @@ -361,7 +361,7 @@ function testImportWithDuplicates(callback) { */ function(updateType, task) { switch (updateType) { - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: resolve(); break; case importer.TaskQueue.UpdateType.ERROR: @@ -395,6 +395,62 @@ function testImportWithDuplicates(callback) { scanResult.finalize(); } +function testImportWithErrors(callback) { + var media = setupFileSystem([ + '/DCIM/photos0/IMG00001.jpg', + '/DCIM/photos0/IMG00002.jpg', + '/DCIM/photos0/IMG00003.jpg', + '/DCIM/photos1/IMG00004.jpg', + '/DCIM/photos1/IMG00005.jpg', + '/DCIM/photos1/IMG00006.jpg' + ]); + + /** @const {number} */ + var EXPECTED_COPY_COUNT = 5; + + var scanResult = new TestScanResult(media); + var importTask = mediaImporter.importFromScanResult( + scanResult, + importer.Destination.GOOGLE_DRIVE, + destinationFactory); + var whenImportDone = new Promise( + function(resolve, reject) { + importTask.addObserver( + /** + * @param {!importer.TaskQueue.UpdateType} updateType + * @param {!importer.TaskQueue.Task} task + */ + function(updateType, task) { + if (updateType === importer.TaskQueue.UpdateType.COMPLETE) { + resolve(); + } + }); + }); + + // Simulate an error after 3 imports. + var copyCount = 0; + importTask.addObserver(function(updateType) { + if (updateType === + importer.MediaImportHandler.ImportTask.UpdateType.ENTRY_CHANGED) { + copyCount++; + if (copyCount === 3) { + mockCopier.simulateOneError(); + } + } + }); + + // Verify that the error didn't result in only 3 files being imported. + reportPromise( + whenImportDone.then( + function() { + var copiedEntries = destinationFileSystem.root.getAllChildren(); + assertEquals(EXPECTED_COPY_COUNT, copiedEntries.length); + }), + callback); + + scanResult.finalize(); +} + /** * @param {!Array.} fileNames * @return {!Array.} @@ -421,6 +477,8 @@ function MockCopyTo() { // Replace with test function. fileOperationUtil.copyTo = this.copyTo_.bind(this); + this.simulateError_ = false; + this.entryChangedCallback_ = null; this.progressCallback_ = null; this.successCallback_ = null; @@ -436,6 +494,13 @@ function MockCopyTo() { */ MockCopyTo.CopyInfo; +/** + * Makes the mock copier simulate an error the next time copyTo_ is called. + */ +MockCopyTo.prototype.simulateOneError = function() { + this.simulateError_ = true; +}; + /** * A mock to replace fileOperationUtil.copyTo. See the original for details. * @param {Entry} source @@ -454,6 +519,12 @@ MockCopyTo.prototype.copyTo_ = function(source, parent, newName, this.successCallback_ = successCallback; this.errorCallback_ = errorCallback; + if (this.simulateError_) { + this.simulateError_ = false; + this.errorCallback_(new Error('test error')); + return; + } + // Log the copy, then copy the file. this.copiedFiles.push({ source: source, diff --git a/ui/file_manager/file_manager/background/js/task_queue.js b/ui/file_manager/file_manager/background/js/task_queue.js index 26d1ac042e0fc..c39022e3004dc 100644 --- a/ui/file_manager/file_manager/background/js/task_queue.js +++ b/ui/file_manager/file_manager/background/js/task_queue.js @@ -40,7 +40,7 @@ importer.TaskQueue = function() { */ importer.TaskQueue.UpdateType = { PROGRESS: 'PROGRESS', - SUCCESS: 'SUCCESS', + COMPLETE: 'COMPLETE', ERROR: 'ERROR', CANCELED: 'CANCELED' }; @@ -103,7 +103,8 @@ importer.TaskQueue.prototype.onTaskUpdate_ = function(task, updateType) { // If the task update is a terminal one, move on to the next task. var UpdateType = importer.TaskQueue.UpdateType; - if (updateType === UpdateType.SUCCESS || updateType === UpdateType.ERROR) { + if (updateType === UpdateType.COMPLETE || + updateType === UpdateType.CANCELED) { // Assumption: the currently running task is at the head of the queue. console.assert(this.tasks_[0] === task, 'Only tasks that are at the head of the queue should be active'); @@ -212,7 +213,7 @@ importer.TaskQueue.BaseTask.prototype.run = function() {}; importer.TaskQueue.BaseTask.prototype.notify = function(updateType, opt_data) { switch (updateType) { case importer.TaskQueue.UpdateType.CANCELED: - case importer.TaskQueue.UpdateType.SUCCESS: + case importer.TaskQueue.UpdateType.COMPLETE: this.finishedResolver_.resolve(updateType); } diff --git a/ui/file_manager/file_manager/background/js/task_queue_unittest.js b/ui/file_manager/file_manager/background/js/task_queue_unittest.js index d75a3a3438e7c..1e45087c208c2 100644 --- a/ui/file_manager/file_manager/background/js/task_queue_unittest.js +++ b/ui/file_manager/file_manager/background/js/task_queue_unittest.js @@ -59,9 +59,14 @@ TestTask.prototype.notifyError = function() { this.notify(importer.TaskQueue.UpdateType.ERROR); }; -/** Sends a quick success notification. */ -TestTask.prototype.notifySuccess = function() { - this.notify(importer.TaskQueue.UpdateType.SUCCESS); +/** Sends a quick completion notification. */ +TestTask.prototype.notifyComplete = function() { + this.notify(importer.TaskQueue.UpdateType.COMPLETE); +}; + +/** Sends a quick cancelled notification. */ +TestTask.prototype.notifyCanceled = function() { + this.notify(importer.TaskQueue.UpdateType.CANCELED); }; /** Sends a quick progress notification. */ @@ -86,9 +91,9 @@ function testRunsTasks(callback) { var task0 = new TestTask('task0'); var task1 = new TestTask('task1'); - // Make the tasks call Task#notifySuccess when they are run. - task0.whenRun().then(function(task) { task.notifySuccess(); }); - task1.whenRun().then(function(task) { task.notifySuccess(); }); + // Make the tasks call Task#notifyComplete when they are run. + task0.whenRun().then(function(task) { task.notifyComplete(); }); + task1.whenRun().then(function(task) { task.notifyComplete(); }); // Enqueue both tasks, and then verify that they were run. queue.queueTask(task0); @@ -121,7 +126,7 @@ function testOnActiveCalled(callback) { function testOnIdleCalled(callback) { var task = new TestTask('task0'); - task.whenRun().then(function(task) { task.notifySuccess(); }); + task.whenRun().then(function(task) { task.notifyComplete(); }); // Make a promise that resolves when the idle callback is triggered // (i.e. after all queued tasks have finished running). @@ -152,7 +157,7 @@ function testProgressUpdate(callback) { }) .then( function(task) { - task.notifySuccess(); + task.notifyComplete(); return task; }); @@ -176,7 +181,7 @@ function testSuccessUpdate(callback) { var task = new TestTask('task0'); // Get the task to report success when it's run. - task.whenRun().then(function(task) { task.notifySuccess(); }); + task.whenRun().then(function(task) { task.notifyComplete(); }); queue.queueTask(task); @@ -184,7 +189,7 @@ function testSuccessUpdate(callback) { queue.setIdleCallback( function() { // Verify that the done callback was called. - assertEquals(1, updates[importer.TaskQueue.UpdateType.SUCCESS]); + assertEquals(1, updates[importer.TaskQueue.UpdateType.COMPLETE]); resolve(); }); }); @@ -197,7 +202,13 @@ function testErrorUpdate(callback) { var task = new TestTask('task0'); // Get the task to report an error when it's run. - task.whenRun().then(function(task) { task.notifyError(); }); + task.whenRun().then( + function(task) { + task.notifyError(); + // Errors are not terminal; still need to signal task completion + // otherwise the test hangs. + task.notifyComplete(); + }); queue.queueTask(task); @@ -212,3 +223,19 @@ function testErrorUpdate(callback) { reportPromise(whenDone, callback); } + +function testOnTaskCancelled(callback) { + var task0 = new TestTask('task0'); + var task1 = new TestTask('task1'); + + // Make the tasks call Task#notifyComplete when they are run. + task0.whenRun().then(function(task) {task.notifyCanceled(); }); + task1.whenRun().then(function(task) {task.notifyComplete(); }); + + // Enqueue both tasks, and then verify that they were run. + queue.queueTask(task0); + queue.queueTask(task1); + reportPromise( + Promise.all([task0.whenRun(), task1.whenRun()]), + callback); +}