Task Management - Handle Promises "Then" or "Catch" callback case.

chrome/browser/resources/google_now/utility.js

 // Copyright (c) 2013 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';
 
 /**
  * @fileoverview Utility objects and functions for Google Now extension.
  * Most important entities here:
  * (1) 'wrapper' is a module used to add error handling and other services to
@@ skipping 419 matching lines @@
 })();
 
 wrapper.instrumentChromeApiFunction('alarms.get', 1);
 wrapper.instrumentChromeApiFunction('alarms.onAlarm.addListener', 0);
 wrapper.instrumentChromeApiFunction('identity.getAuthToken', 1);
 wrapper.instrumentChromeApiFunction('identity.onSignInChanged.addListener', 0);
 wrapper.instrumentChromeApiFunction('identity.removeCachedAuthToken', 1);
 wrapper.instrumentChromeApiFunction('webstorePrivate.getBrowserLogin', 0);
 
 /**
- * Add task tracking support to Promise.then.
- * @override
+ * Promise adapter for all JS promises to the task manager.
  */
-Promise.prototype.then = function() {
+function registerPromiseAdapter() {
   var originalThen = Promise.prototype.then;
-  return function(callback) {
-    return originalThen.call(this, wrapper.wrapCallback(callback, false));
+  var originalCatch = Promise.prototype.catch;
+
+  /**
+   * Takes a promise and adds the callback tracker to it.
+   * @param {object} promise Promise that receives the callback tracker.
+   */
+  function instrumentPromise(promise) {
+    if (promise.__tracker === undefined) {
+      promise.__tracker = createPromiseCallbackTracker(promise);
+    }
   }
-}();
 
-/**
- * Add task tracking support to Promise.catch.
- * @override
- */
-Promise.prototype.catch = function() {
-  var originalCatch = Promise.prototype.catch;
-  return function(callback) {
-    return originalCatch.call(this, wrapper.wrapCallback(callback, false));
+  Promise.prototype.then = function(onResolved, onRejected) {
+    instrumentPromise(this);
+    return this.__tracker.handleThen(onResolved, onRejected);
   }
-}();
+
+  Promise.prototype.catch = function(onRejected) {
+    instrumentPromise(this);
+    return this.__tracker.handleCatch(onRejected);
+  }
+
+  /**
+   * Promise Callback Tracker.
+   * Handles coordination of 'then' and 'catch' callbacks in a task
+   * manager compatible way. For an individual promise, either the 'then'
+   * arguments or the 'catch' arguemnts will be processed, never both.
+   *
+   * Example:
+   *     var p = new Promise([Function]);
+   *     p.then([ThenA]);
+   *     p.then([ThenB]);
+   *     p.catch([CatchA]);
+   *     On resolution, [ThenA] and [ThenB] will be used. [CatchA] is discarded.
+   *     On rejection, vice versa.
+   *
+   * Clarification:
+   *     Chained promises create a new promise that is tracked separately from
+   *     the originaing promise, as the example below demonstrates:
+   *
+   *     var p = new Promise([Function]));
+   *     p.then([ThenA]).then([ThenB]).catch([CatchA]);
+   *         ^             ^             ^
+   *         |             |             + Returns a new promise.
+   *         |             + Returns a new promise.
+   *         + Returns a new promise.
+   *
+   *     Four promises exist in the above statement, each with its own
+   *     resolution and rejection state. However, by default, this state is
+   *     chained to the previous previous promise's resolution or rejection
+   *     state.
+   *
+   *     If p resolves, then the then calls will execute until all the 'then'
+   *     clauses are executed. If the result of either [ThenA] or [ThenB] is a
+   *     promise, then that execution state will guide the remaining chain.
+   *     Similarly, if [CatchA] returns a promise, it can also guide the
+   *     remaining chain. In this specific case, the chain ends, so there
+   *     is nothing left to do.
+   * @param {object} promise Promise being tracked.
+   * @return {object} A promise callback tracker.
+   */
+  function createPromiseCallbackTracker(promise) {
+    /**
+     * Callback Tracker. Holds an array of callbacks created for this promise.
+     * The indirection allows quick checks against the array and clearing the
+     * array without ugly splicing and copying.
+     * @typedef {{
+     *   callback: array.<Function>
+     * }}
+     */
+    var CallbackTracker;
+
+    /** @type {CallbackTracker} */
+    var thenTracker = {callbacks: []};
+    /** @type {CallbackTracker} */
+    var catchTracker = {callbacks: []};
+
+    /**
+     * Returns true if the specified value is callable.
+     * @param {*} value Value to check.
+     * @return {boolean} True if the value is a callable.
+     */
+    function isCallable(value) {
+      return typeof value === 'function';
+    }
+
+    /**
+     * Takes a tracker and clears its callbacks in a manner consistent with
+     * the task manager.
+     * @param {CallbackTracker} tracker Tracker to clear.
+     */
+    function clearTracker(tracker) {
+      if (tracker.callbacks.length > 0) {
+        var callbacksToClear = tracker.callbacks;
+        tracker.callbacks = [];

[vadimt, 2014/02/25 23:20:22]

It's worth adding a comment that this is done to actually prevent these callback
from working.

[robliao, 2014/02/25 23:39:54]

Done.

+        // Do not wrap the setTimeout callback!
+        // It will call wrapped callbacks.
+        setTimeout(function() {
+          for (var i = 0; i < callbacksToClear.length; i++) {
+            callbacksToClear[i]();

[vadimt, 2014/02/25 22:36:49]

It's not clear why we need to invoke all callbacks from a function named
"clearXXX". Could you add a comment? Or may be, a better name for the function?

[robliao, 2014/02/25 22:43:14]

The comment says it clears the tracker in a manner consistent with the task
manager.

The task manager requires that all callbacks be called, and hence to properly
clear it, we have to do so here.
On 2014/02/25 22:36:49, vadimt wrote:

[vadimt, 2014/02/25 23:20:22]

Cool, could you add that in the comment?

[robliao, 2014/02/25 23:39:54]

Added to the banner comment above.

+          }
+        }, 0);
+      }
+    }
+
+    /**
+     * Takes the argument to a 'then' or 'catch' function and applies
+     * a wrapping to callables consistent to ECMA promises.
+     * @param {*} maybeCallback Argument to 'then' or 'catch'.
+     * @param {CallbackTracker} sameTracker Tracker for the call type.
+     *     Example: If the argument is from a 'then' call, use thenTracker.
+     * @param {CallbackTracker} otherTracker Tracker for the opposing call type.
+     *     Example: If the argument is from a 'then' call, use catchTracker.
+     * @return {*} Consumable argument with necessary wrapping applied.
+     */
+    function registerAndWrapMaybeCallback(
+          maybeCallback, sameTracker, otherTracker) {

[vadimt, 2014/02/25 22:36:49]

maybeCallback can be either function of undefined, correct?
If so, can we be more specific both in the type of the argument, and with how we
check that this is a function (!== undefined).
This would help readability.

[robliao, 2014/02/25 22:43:14]

maybeCallback is *, as documented above. It can be a function, a promise, a
value, a string, anything!
On 2014/02/25 22:36:49, vadimt wrote:

+      if (isCallable(maybeCallback)) {
+        var handler = wrapper.wrapCallback(function() {

[vadimt, 2014/02/25 22:36:49]

Are you calling wrapCallback for each then and catch?
If so, how do you avoid getting errors when, say then called, and catch didn't?
Is this why you call all callbacks from the clearTracker()?
(What I was suggesting was to call wrapCallback once per promise, and pass the
result of it to all actual thens and catches.)

[robliao, 2014/02/25 22:43:14]

This is why clearTracker calls all callbacks it the given callback tracker.

There are a few reasons why each then/catch gets one callback:
1) I don't have to get into the business of resolving the value of each
then/catch. I can leave that to the promise itself. This isn't as simple as
returning the previous value. If the resolved value is a promise, for example,
we have to do extra work.
2) We only care about if one then or one catch is called. This will correctly
clear the other set and we don't have to iterate and rewrap callbacks ourselves
for each then.
3) This lets the promise handle invocation of each then as it would normally do.

On 2014/02/25 22:36:49, vadimt wrote:

[robliao, 2014/02/25 22:51:13]

Also, calling wrapCallback once would also miscount the number of callbacks that
could occur.

If each promise only had one wrapCallback, but two thens were called, then the
number of pendingCallbacks would be incorrect.

The then callbacks would also need a wrappedCallback environment, but they can't
use our single wrappedCallback since that would clear it from the pending
callback.

On 2014/02/25 22:43:14, robliao wrote:

+          if (sameTracker.callbacks.length > 0) {

[vadimt, 2014/02/25 23:20:22]

If you place this check outside of the wrapped part, you can eliminate using the
setTimeout() in clearTracker.

[robliao, 2014/02/25 23:39:54]

This check must be done in the wrapped part. It is critical in no-oping this
callback. We don't know at this point if then or catch will be called.

clearTracker should be responsible for handling the correctness conditions of
calling within a callback. Most everywhere else, we assume that calls are safe
within a callback. setTimeout is a common way of deferring execution.
On 2014/02/25 23:20:22, vadimt wrote:

+            clearTracker(otherTracker);
+            maybeCallback.apply(null, arguments);
+          }
+        }, false);
+        sameTracker.callbacks.push(handler);
+        return handler;
+      } else {
+        return maybeCallback;
+      }
+    }
+
+    /**
+     * Tracks then calls equivalent to Promise.prototype.then.
+     * @param {*} onResolved Argument to use if the promise is resolved.
+     * @param {*} onRejected Argument to use if the promise is rejected.
+     * @return {object} Promise resulting from the 'then' call.
+     */
+    function handleThen(onResolved, onRejected) {

[vadimt, 2014/02/25 23:20:22]

'handle' is not concrete enough. Add? Track? Register?

[robliao, 2014/02/25 23:39:54]

We have precedence for this in Chromium (handleFocus, handleKeyDown,
handleClick, etc.). This actually handles the then call. It takes care of
registration and forwards it to the real then call.
On 2014/02/25 23:20:22, vadimt wrote:

+      var resolutionHandler =
+          registerAndWrapMaybeCallback(onResolved, thenTracker, catchTracker);
+      var rejectionHandler =
+          registerAndWrapMaybeCallback(onRejected, catchTracker, thenTracker);
+      return originalThen.call(promise, resolutionHandler, rejectionHandler);
+    }
+
+    /**
+     * Tracks then calls equivalent to Promise.prototype.catch.
+     * @param {*} onRejected Argument to use if the promise is rejected.
+     * @return {object} Promise resulting from the 'catch' call.
+     */
+    function handleCatch(onRejected) {
+      var rejectionHandler =
+          registerAndWrapMaybeCallback(onRejected, catchTracker, thenTracker);
+      return originalCatch.call(promise, rejectionHandler);
+    }
+
+    // Seeds this promise with at least one 'then' and 'catch' so that we always
+    // know which set of callbacks will not occur.
+    handleThen(function() {});
+    handleCatch(function() {});
+
+    return {
+      handleThen: handleThen,
+      handleCatch: handleCatch
+    };
+  }
+}
+
+registerPromiseAdapter();
 
 /**
  * Builds the object to manage tasks (mutually exclusive chains of events).
  * @param {function(string, string): boolean} areConflicting Function that
  *     checks if a new task can't be added to a task queue that contains an
  *     existing task.
  * @return {Object} Task manager interface.
  */
 function buildTaskManager(areConflicting) {
   /**
@@ skipping 353 matching lines @@
   // One hour is just an arbitrary amount of time chosen.
   chrome.alarms.create(alarmName, {periodInMinutes: 60});
 
   return {
     addListener: addListener,
     getAuthToken: getAuthToken,
     isSignedIn: isSignedIn,
     removeToken: removeToken
   };
 }