Files.app; Serializes event handler calls for particular URLs.

chrome/browser/resources/file_manager/background/js/drive_sync_handler.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * Handler of the background page for the drive sync events.
  * @param {ProgressCenter} progressCenter Progress center to submit the
  *     progressing items.
  * @constructor
  */
 function DriveSyncHandler(progressCenter) {
   /**
    * Progress center to submit the progressng item.
    * @type {ProgressCenter}
    * @private
    */
   this.progressCenter_ = progressCenter;
 
   /**
    * Counter for progress ID.
    * @type {number}
    * @private
    */
   this.idCounter_ = 0;
 
   /**
    * Progressing file names.

[mtomasz, 2014/01/29 10:34:29]

nit: Please update this comment, eg. that this is a map from a url to a promise
of creating an item representing the sync operation.

[hirono, 2014/01/30 04:51:47]

Done.

-   * @type {Object.<string, ProgressCenterItem>} Map a file URL and a progress
-   *     center item.
+   * @type {Object.<string, Promise>} Map a file URL and promise.

[mtomasz, 2014/01/29 10:34:29]

Hm. This solution is cool, but I'm a little bit concerned if the extra
complexity is needed here.

Another solution would be creating a big class-wide queue, and wrap updateItem
and removeItem with it. This is very simple and often used. In Java this pattern
is built in as 'synchronized methods'.

But, I don't have very strong feelings here, so feel free to go with Premise if
you prefer.

WDYT?

[hirono, 2014/01/30 04:51:47]

Just do it simple! Thanks!

    * @private
    */
   this.items_ = {};
 
   // Register events.
   chrome.fileBrowserPrivate.onFileTransfersUpdated.addListener(
       this.onFileTransfersUpdated_.bind(this));
   chrome.fileBrowserPrivate.onDriveSyncError.addListener(
       this.onDriveSyncError_.bind(this));
 }
@@ skipping 53 matching lines @@
     }
   }
 };
 
 /**
  * Updates the item involved with the given status.
  * @param {FileTransferStatus} status Transfer status.
  * @private
  */
 DriveSyncHandler.prototype.updateItem_ = function(status) {
-  webkitResolveLocalFileSystemURL(status.fileUrl, function(entry) {
-    var item;
-    if (!this.items_[status.fileUrl]) {
-      item = new ProgressCenterItem();
-      item.id = DriveSyncHandler.PROGRESS_ITEM_ID_PREFIX + (this.idCounter_++);
-      item.type = ProgressItemType.SYNC;
-      item.quiet = true;
-      item.message = strf('SYNC_FILE_NAME', entry.name);
-      item.cancelCallback = this.requestCancel_.bind(this, entry);
-      this.items_[status.fileUrl] = item;
-    } else {
-      item = this.items_[status.fileUrl];
-    }
+  if (!this.items_[status.fileUrl]) {
+    // Promise also handles the error case of webkitResolveLocalFileSystemURL
+    // and skips the following instructions until the catach method is called.
+    this.items_[status.fileUrl] =
+        new Promise(webkitResolveLocalFileSystemURL.bind(null, status.fileUrl)).
+        then(function(entry) {
+          var item = new ProgressCenterItem();
+          item.id =
+              DriveSyncHandler.PROGRESS_ITEM_ID_PREFIX + (this.idCounter_++);
+          item.type = ProgressItemType.SYNC;
+          item.quiet = true;
+          item.message = strf('SYNC_FILE_NAME', entry.name);
+          item.cancelCallback = this.requestCancel_.bind(this, entry);
+          return item;
+        }.bind(this));

[mtomasz, 2014/01/29 10:34:29]

Please print a warning to console that resolving failed.

[hirono, 2014/01/30 04:51:47]

The error case of webkitResolveLocalFileSystemURL was handled by Promise, in
this case. The constructor of promise takes a function having two arguments:
onSuccess and onFailure.
If onSuccess is called, the promise proceeds the following instructions, and if
onFailure is called, the promise skips the following instructions until the
catch method is called.

[mtomasz, 2014/01/30 05:11:13]

My concern was, that the error in resolving was shown after removing, what is a
little bit too late. I think we should report errors as close to the root cause
as possible.

+  }
+  this.items_[status.fileUrl].then(function(item) {
     item.progressValue = status.processed || 0;
     item.progressMax = status.total || 1;
     this.progressCenter_.updateItem(item);
-  }.bind(this), function() {
-    console.error('Cannot resolve the URL: ' + status.fileUrl);
-  });
+  }.bind(this));
 };
 
 /**
  * Removes the item involved with the given status.
  * @param {FileTransferStatus} status Transfer status.
  * @private
  */
 DriveSyncHandler.prototype.removeItem_ = function(status) {
-  var item = this.items_[status.fileUrl];
-  delete this.items_[status.fileUrl];
-  if (item) {
+  if (!this.items_[status.fileUrl])
+    return;
+  this.items_[status.fileUrl].then(function(item) {
     item.state = status.transferState === 'completed' ?
         ProgressItemState.COMPLETED : ProgressItemState.CANCELED;
     this.progressCenter_.updateItem(item);
-  }
+    return null;
+  }.bind(this)).catch (function(error) {
+    console.error(error);

[mtomasz, 2014/01/29 10:34:29]

Can you please handle this catch in updateItem_, where the resolving takes
place, instead?

[hirono, 2014/01/30 04:51:47]

The position of the catch method was important because if resolving the entry
failed at first, we should skip the following instructions until the sync
completes.

[Success]
updateItem (with resolving URL)
updateItem
updateItem
...
removeItem

[Failure]
updateItem (with resolving URL)
updateItem - skipped
updateItem - skipped
...        - skipped
removeItem - catch the error and output it.

We usually add error checks at the head of functions when using AsyncUtil.Queue.
Promise did the same thing automatically.

[mtomasz, 2014/01/30 05:11:13]

Interesting. What would happen if we called catch() in updateItem_? Like.
new Promise().then(fun() { success }).catch(fun() { error }). Would removeItem()
misbehave in such situation?

The Pomise class is super cool, and it seems that we could use it in several
places to simplify things, like that maybeFinish() methods with a callback
counter inside, or AsyncUtil.Group() when dependencies are not needed. But on
the other hand, the produced code can be sometimes difficult to understand. I
need to read more about it.

+  });
+  delete this.items_[status.fileUrl];
 };
 
 /**
  * Requests to cancel for the given files' drive sync.
  * @param {Entry} entry Entry to be canceled.
  * @private
  */
 DriveSyncHandler.prototype.requestCancel_ = function(entry) {
   chrome.fileBrowserPrivate.cancelFileTransfers([entry.toURL()], function() {});
 };
@@ skipping 19 matching lines @@
       case 'service_unavailable':
         item.message = str('SYNC_SERVICE_UNAVAILABLE_ERROR');
         break;
       case 'misc':
         item.message = strf('SYNC_MISC_ERROR', entry.name);
         break;
     }
     this.progressCenter_.updateItem(item);
   }.bind(this));
 };