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

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

Index: chrome/browser/resources/file_manager/background/js/drive_sync_handler.js
diff --git a/chrome/browser/resources/file_manager/background/js/drive_sync_handler.js b/chrome/browser/resources/file_manager/background/js/drive_sync_handler.js
index 2ba9672fe47fbc2617defb2261ebed1a5b0fd885..603d86201a7d9611941409b3518a333a3d62587a 100644
--- a/chrome/browser/resources/file_manager/background/js/drive_sync_handler.js
+++ b/chrome/browser/resources/file_manager/background/js/drive_sync_handler.js
@@ -27,8 +27,7 @@ function DriveSyncHandler(progressCenter) {
 
   /**
    * 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_ = {};
@@ -102,25 +101,27 @@ DriveSyncHandler.prototype.onFileTransfersUpdated_ = function(statusList) {
  * @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));
 };
 
 /**
@@ -129,13 +130,17 @@ DriveSyncHandler.prototype.updateItem_ = function(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];
 };
 
 /**