Files app: VolumeManager's mountArchive and unmount return a Promise

Changed VolumeManager's methods mountArchive() and unmount() to make
them return a Promise instead of taking completion callbacks.

Change-Id: I9debd4c7edc8256f24f539f81bac088c44edd09a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1727653Reviewed-by: François Degros <fdegros@chromium.org>
Reviewed-by: Austin Tankiang <austinct@chromium.org>
Commit-Queue: François Degros <fdegros@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683027}

ui/file_manager/base/js/filtered_volume_manager.js

@@ -388,31 +388,16 @@ class FilteredVolumeManager extends cr.EventTarget {
     return volumeInfo;
   }
 
-  /**
-   * Requests to mount the archive file.
-   * @param {string} fileUrl The path to the archive file to be mounted.
-   * @param {function(VolumeInfo)} successCallback Called with the VolumeInfo
-   *     instance.
-   * @param {function(VolumeManagerCommon.VolumeError)} errorCallback Called
-   *     when an error occurs.
-   */
-  mountArchive(fileUrl, successCallback, errorCallback) {
-    this.ensureInitialized(() => {
-      this.volumeManager_.mountArchive(fileUrl, successCallback, errorCallback);
-    });
+  /** @override */
+  async mountArchive(fileUrl) {
+    await this.initialized_;
+    return this.volumeManager_.mountArchive(fileUrl);
   }
 
-  /**
-   * Requests unmount the specified volume.
-   * @param {!VolumeInfo} volumeInfo Volume to be unmounted.
-   * @param {function()} successCallback Called on success.
-   * @param {function(VolumeManagerCommon.VolumeError)} errorCallback Called
-   *     when an error occurs.
-   */
-  unmount(volumeInfo, successCallback, errorCallback) {
-    this.ensureInitialized(() => {
-      this.volumeManager_.unmount(volumeInfo, successCallback, errorCallback);
-    });
+  /** @override */
+  async unmount(volumeInfo) {
+    await this.initialized_;
+    return this.volumeManager_.unmount(volumeInfo);
   }
 
   /**

ui/file_manager/externs/volume_manager.js

@@ -37,20 +37,18 @@ class VolumeManager {
 
   /**
    * @param {string} fileUrl File url to the archive file.
-   * @param {function(VolumeInfo)} successCallback Success callback.
-   * @param {function(VolumeManagerCommon.VolumeError)} errorCallback Error
-   *     callback.
+   * @return {!Promise<!VolumeInfo>} Fulfilled on success, otherwise rejected
+   *     with a VolumeManagerCommon.VolumeError.
    */
-  mountArchive(fileUrl, successCallback, errorCallback) {}
+  mountArchive(fileUrl) {}
 
   /**
    * Unmounts a volume.
    * @param {!VolumeInfo} volumeInfo Volume to be unmounted.
-   * @param {function()} successCallback Success callback.
-   * @param {function(VolumeManagerCommon.VolumeError)} errorCallback Error
-   *     callback.
+   * @return {!Promise<void>} Fulfilled on success, otherwise rejected with a
+   *     VolumeManagerCommon.VolumeError.
    */
-  unmount(volumeInfo, successCallback, errorCallback) {}
+  unmount(volumeInfo) {}
 
   /**
    * Configures a volume.
@@ -132,4 +130,3 @@ class VolumeManager {
  * @typedef {!CustomEvent<!VolumeInfo>}
  */
 let ExternallyUnmountedEvent;
-

ui/file_manager/file_manager/background/js/mock_volume_manager.js

@@ -190,11 +190,11 @@ class MockVolumeManager {
     return volumeInfo;
   }
 
-  mountArchive(fileUrl, successCallback, errorCallback) {
+  async mountArchive(fileUrl) {
     throw new Error('Not implemented');
   }
 
-  unmount(volumeInfo, successCallback, errorCallback) {
+  async unmount(volumeInfo) {
     throw new Error('Not implemented');
   }
 

ui/file_manager/file_manager/background/js/volume_manager_impl.js

@@ -142,9 +142,9 @@ class VolumeManagerImpl extends cr.EventTarget {
     try {
       // Create VolumeInfo for each volume.
       await Promise.all(volumeMetadataList.map(async (volumeMetadata) => {
-        console.warn(`Initializing volume: ${volumeMetadata.volumeId}`);
+        console.warn(`Initializing volume '${volumeMetadata.volumeId}'`);
         const volumeInfo = await this.addVolumeMetadata_(volumeMetadata);
-        console.warn(`Initialized volume: ${volumeInfo.volumeId}`);
+        console.warn(`Initialized volume '${volumeInfo.volumeId}'`);
       }));
 
       console.warn(`Initialized all ${volumeMetadataList.length} volumes`);
@@ -187,7 +187,8 @@ class VolumeManagerImpl extends cr.EventTarget {
             break;
           }
 
-          console.error(`Cannot mount volume: ${event.status}`);
+          console.error(`Cannot mount '${event.volumeMetadata.sourcePath}': ${
+              event.status}`);
           this.finishRequest_(requestKey, event.status);
           break;
 
@@ -202,7 +203,7 @@ class VolumeManagerImpl extends cr.EventTarget {
               null;
 
           if (event.status === 'success' && !requested && volumeInfo) {
-            console.warn(`Unmounted volume without request: ${volumeId}`);
+            console.warn(`Unmounted '${volumeId}' without request`);
             this.dispatchEvent(
                 new CustomEvent('externally-unmounted', {detail: volumeInfo}));
           }
@@ -212,7 +213,6 @@ class VolumeManagerImpl extends cr.EventTarget {
             this.volumeInfoList.remove(event.volumeMetadata.volumeId);
           }
 
-          console.warn(`Unmounted volume: ${volumeId}`);
           break;
       }
     } finally {
@@ -234,19 +234,24 @@ class VolumeManagerImpl extends cr.EventTarget {
   }
 
   /** @override */
-  mountArchive(fileUrl, successCallback, errorCallback) {
-    chrome.fileManagerPrivate.addMount(fileUrl, sourcePath => {
-      console.info(`Mount request: url=${fileUrl}; sourcePath=${sourcePath}`);
-      const requestKey = this.makeRequestKey_('mount', sourcePath);
-      this.startRequest_(requestKey, successCallback, errorCallback);
+  async mountArchive(fileUrl) {
+    const path = await new Promise(resolve => {
+      chrome.fileManagerPrivate.addMount(fileUrl, resolve);
     });
+    console.warn(`Mounting '${path}'`);
+    const key = this.makeRequestKey_('mount', path);
+    const volumeInfo = await this.startRequest_(key);
+    console.warn(`Mounted '${path}' as '${volumeInfo.volumeId}'`);
+    return volumeInfo;
   }
 
   /** @override */
-  unmount(volumeInfo, successCallback, errorCallback) {
+  async unmount(volumeInfo) {
+    console.warn(`Unmounting '${volumeInfo.volumeId}'`);
     chrome.fileManagerPrivate.removeMount(volumeInfo.volumeId);
-    const requestKey = this.makeRequestKey_('unmount', volumeInfo.volumeId);
-    this.startRequest_(requestKey, successCallback, errorCallback);
+    const key = this.makeRequestKey_('unmount', volumeInfo.volumeId);
+    await this.startRequest_(key);
+    console.warn(`Unmounted '${volumeInfo.volumeId}'`);
   }
 
   /** @override */
@@ -442,13 +447,12 @@ class VolumeManagerImpl extends cr.EventTarget {
 
   /**
    * @param {string} key Key produced by |makeRequestKey_|.
-   * @param {function(VolumeInfo)} successCallback To be called when the request
-   *     finishes successfully.
-   * @param {function(VolumeManagerCommon.VolumeError)} errorCallback To be
-   *     called when the request fails.
+   * @return {!Promise<!VolumeInfo>} Fulfilled on success, otherwise rejected
+   *     with a VolumeManagerCommon.VolumeError.
    * @private
    */
-  startRequest_(key, successCallback, errorCallback) {
+  startRequest_(key) {
+    return new Promise((successCallback, errorCallback) => {
       if (key in this.requests_) {
         const request = this.requests_[key];
         request.successCallbacks.push(successCallback);
@@ -462,6 +466,7 @@ class VolumeManagerImpl extends cr.EventTarget {
               this.onTimeout_.bind(this, key), volumeManagerUtil.TIMEOUT)
         };
       }
+    });
   }
 
   /**

ui/file_manager/file_manager/background/js/volume_manager_unittest.js

@@ -189,11 +189,9 @@ function testMountArchiveAndUnmount(callback) {
     const numberOfVolumes = volumeManager.volumeInfoList.length;
 
     // Mount an archive
-    const mounted = new Promise(
-        (resolve, reject) => volumeManager.mountArchive(
+    const mounted = volumeManager.mountArchive(
         'filesystem:chrome-extension://extensionid/external/' +
-                'Downloads-test/foobar.zip',
-            resolve, reject));
+        'Downloads-test/foobar.zip');
 
     mockChrome.fileManagerPrivate.onMountCompleted.dispatchEvent({
       eventType: 'mount',
@@ -219,9 +217,7 @@ function testMountArchiveAndUnmount(callback) {
     const entry =
         new MockFileEntry(new MockFileSystem('archive:foobar.zip'), '/foo.txt');
     const volumeInfo = volumeManager.getVolumeInfo(entry);
-    await new Promise(
-        (resolve, reject) =>
-            volumeManager.unmount(volumeInfo, resolve, reject));
+    await volumeManager.unmount(volumeInfo);
 
     assertEquals(numberOfVolumes, volumeManager.volumeInfoList.length);
   };

ui/file_manager/file_manager/background/js/volume_manager_util.js

@@ -97,9 +97,7 @@ volumeManagerUtil.createVolumeInfo = volumeMetadata => {
       break;
   }
 
-  console.warn(
-      'Requesting file system: ' + volumeMetadata.volumeType + ' ' +
-      volumeMetadata.volumeId);
+  console.warn(`Getting file system '${volumeMetadata.volumeId}'`);
   return util
       .timeoutPromise(
           new Promise((resolve, reject) => {
@@ -132,7 +130,7 @@ volumeManagerUtil.createVolumeInfo = volumeMetadata => {
                         if (chrome.runtime.lastError) {
                           reject(chrome.runtime.lastError.message);
                         } else if (!entries[0]) {
-                          reject('Resolving for external context failed.');
+                          reject('Resolving for external context failed');
                         } else {
                           resolve(entries[0].filesystem);
                         }
@@ -145,7 +143,7 @@ volumeManagerUtil.createVolumeInfo = volumeMetadata => {
       .then(
           /** @param {!FileSystem} fileSystem */
           fileSystem => {
-            console.warn('File system obtained: ' + volumeMetadata.volumeId);
+            console.warn(`Got file system '${volumeMetadata.volumeId}'`);
             if (volumeMetadata.volumeType ===
                 VolumeManagerCommon.VolumeType.DRIVE) {
               // After file system is mounted, we "read" drive grand root
@@ -156,7 +154,7 @@ volumeManagerUtil.createVolumeInfo = volumeMetadata => {
               fileSystem.root.createReader().readEntries(
                   () => {/* do nothing */}, error => {
                     console.warn(
-                        'Triggering full feed fetch has failed: ' + error.name);
+                        `Triggering full feed fetch has failed: ${error.name}`);
                   });
             }
             return new VolumeInfoImpl(
@@ -178,9 +176,8 @@ volumeManagerUtil.createVolumeInfo = volumeMetadata => {
       .catch(
           /** @param {*} error */
           error => {
-            console.warn(
-                'Failed to mount a file system: ' + volumeMetadata.volumeId +
-                ' because of: ' + (error.stack || error));
+            console.error(`Cannot mount file system '${
+                volumeMetadata.volumeId}': ${error.stack || error}`);
 
             // TODO(crbug/847729): Report a mount error via UMA.
 

ui/file_manager/file_manager/foreground/js/file_manager_commands.js

@@ -544,7 +544,12 @@ CommandHandler.COMMANDS_ = {};
  * Unmounts external drive.
  */
 CommandHandler.COMMANDS_['unmount'] = new class extends Command {
-  execute(event, fileManager) {
+  /**
+   * @param {!Event} event Command event.
+   * @param {!CommandHandlerDeps} fileManager CommandHandlerDeps.
+   * @private
+   */
+  async executeImpl_(event, fileManager) {
     /** @param {VolumeManagerCommon.VolumeType=} opt_volumeType */
     const errorCallback = opt_volumeType => {
       if (opt_volumeType === VolumeManagerCommon.VolumeType.REMOVABLE) {
@@ -556,11 +561,6 @@ CommandHandler.COMMANDS_['unmount'] = new class extends Command {
       }
     };
 
-    const successCallback = () => {
-      const msg = strf('A11Y_VOLUME_EJECT', label);
-      fileManager.ui.speakA11yMessage(msg);
-    };
-
     // Find volumes to unmount.
     let volumes = [];
     let label = '';
@@ -587,11 +587,22 @@ CommandHandler.COMMANDS_['unmount'] = new class extends Command {
     }
 
     // Eject volumes of which there may be multiple.
-    for (let i = 0; i < volumes.length; i++) {
-      fileManager.volumeManager.unmount(
-          volumes[i], (i == volumes.length - 1) ? successCallback : () => {},
-          errorCallback.bind(null, volumes[i].volumeType));
+    const promises = volumes.map(async (volume) => {
+      try {
+        await fileManager.volumeManager.unmount(volume);
+      } catch (error) {
+        console.error(
+            `Cannot unmount ${volume.volumeId}: ${error.stack || error}`);
+        errorCallback(volume.volumeType);
       }
+    });
+
+    await Promise.all(promises);
+    fileManager.ui.speakA11yMessage(strf('A11Y_VOLUME_EJECT', label));
+  }
+
+  execute(event, fileManager) {
+    this.executeImpl_(event, fileManager);
   }
 
   /** @override */

ui/file_manager/file_manager/foreground/js/file_tasks.js

@@ -886,44 +886,45 @@ class FileTasks {
   }
 
   /**
-   * The core implementation of mounts archives.
+   * The core implementation of mount archives.
    * @private
    */
-  mountArchivesInternal_() {
+  async mountArchivesInternal_() {
     const tracker = this.directoryModel_.createDirectoryChangeTracker();
     tracker.start();
-
+    try {
       // TODO(mtomasz): Move conversion from entry to url to custom bindings.
       // crbug.com/345527.
       const urls = util.entriesToURLs(this.entries_);
-    for (let index = 0; index < urls.length; ++index) {
-      // TODO(mtomasz): Pass Entry instead of URL.
-      this.volumeManager_.mountArchive(urls[index], volumeInfo => {
+      const promises = urls.map(async (url) => {
+        try {
+          const volumeInfo = await this.volumeManager_.mountArchive(url);
           if (tracker.hasChanged) {
-          tracker.stop();
             return;
           }
-        volumeInfo.resolveDisplayRoot(
-            displayRoot => {
+
+          try {
+            const displayRoot = await volumeInfo.resolveDisplayRoot();
             if (tracker.hasChanged) {
-                tracker.stop();
               return;
             }
+
             this.directoryModel_.changeDirectoryEntry(displayRoot);
-            },
-            () => {
-              console.warn(
-                  'Failed to resolve the display root after mounting.');
-              tracker.stop();
-            });
-      }, ((url, error) => {
-           tracker.stop();
+          } catch (error) {
+            console.error('Cannot resolve display root after mounting:', error);
+          }
+        } catch (error) {
           const path = util.extractFilePath(url);
           const namePos = path.lastIndexOf('/');
           this.ui_.alertDialog.show(
               strf('ARCHIVE_MOUNT_FAILED', path.substr(namePos + 1), error),
               null, null);
-         }).bind(null, urls[index]));
+        }
+      });
+
+      await Promise.all(promises);
+    } finally {
+      tracker.stop();
     }
   }