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 fb5551a)

TBR=mtomasz

Review URL: https://codereview.chromium.org/964533003

Cr-Commit-Position: refs/branch-heads/2311@{#38}
Cr-Branched-From: 09b7de5-refs/heads/master@{#317474}

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.
  */

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.<string>} fileNames
  * @return {!Array.<!Entry>}
@@ -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,

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);
   }
 

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,15 +181,15 @@ 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);
 
   var whenDone = new Promise(function(resolve) {
     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);
+}