Convert Google Now's use of chrome.local.storage.get to use Promises

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 305 matching lines @@
         verify(!isInWrappedCallback, 'Re-entering instrumented callback');
         isInWrappedCallback = true;
 
         if (!opt_isEventListener)
           delete pendingCallbacks[callbackId];
 
         if (wrapperPluginInstance)
           wrapperPluginInstance.prologue();
 
         // Call the original callback.
-        callback.apply(null, arguments);
+        var returnValue = callback.apply(null, arguments);
 
         if (wrapperPluginInstance)
           wrapperPluginInstance.epilogue();
 
         verify(isInWrappedCallback,
                'Instrumented callback is not instrumented upon exit');
         isInWrappedCallback = false;
+
+        return returnValue;
       } catch (error) {
         reportError(error);
       }
     };
   }
 
   /**
    * Returns an instrumented function.
    * @param {!Array.<string>} functionIdentifierParts Path to the chrome.*
    *     function.
@@ skipping 83 matching lines @@
     checkInWrappedCallback: checkInWrappedCallback,
     debugGetStateString: debugGetStateString
   };
 })();
 
 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('storage.local.get', 1);
 wrapper.instrumentChromeApiFunction('webstorePrivate.getBrowserLogin', 0);
 
 /**
  * Promise adapter for all JS promises to the task manager.
  */
 function registerPromiseAdapter() {
   var originalThen = Promise.prototype.then;
   var originalCatch = Promise.prototype.catch;
 
   /**
@@ skipping 165 matching lines @@
     return {
       handleThen: handleThen,
       handleCatch: handleCatch
     };
   }
 }
 
 registerPromiseAdapter();
 
 /**
+ * Control promise rejection.
+ * @enum {boolean}
+ */
+var PromiseRejection = {
+  /** Disallow promise rejection */
+  DISALLOW: false,
+  /** Allow promise rejection */
+  ALLOW: true
+};
+
+/**
+ * Provides the promise equivalent of instrumented.storage.local.get.
+ * @param {Object} defaultStorageObject Default storage object to fill.
+ * @param {boolean} opt_allowPromiseRejection If PromiseRejection.ALLOW,

[rgustafson, 2014/02/27 21:32:10]

document param as optional

[robliao, 2014/02/27 23:52:36]

Done.

+ *     allow promise rejection on errors, otherwise the default storage
+ *     object is resolved.
+ * @return {Promise} A promise that fills the default storage object. On
+ *     failure, if promise rejection is allowed, the promise is rejected,
+ *     otherwise it is resolved to the default storage object.
+ */
+function fillFromChromeLocalStorage(
+    defaultStorageObject,
+    opt_allowPromiseRejection) {
+  return new Promise(function(resolve, reject) {
+    instrumented.storage.local.get(defaultStorageObject, function(items) {
+      if (items) {
+        resolve(items);
+      } else if (opt_allowPromiseRejection === PromiseRejection.ALLOW) {

[vadimt, 2014/02/27 20:20:36]

Why not "if (opt_allowPromiseRejection)"?

[robliao, 2014/02/27 23:52:36]

We're comparing against an enum, so we should do an equality comparison.
On 2014/02/27 20:20:36, vadimt wrote:

[vadimt, 2014/02/28 20:21:05]

You don't use the fact that enum values are booleans, which is confusing. I
believe you should either:
1. Define the constants as 0 and 1
OR
2. if (opt_allowPromiseRejection)

Your choice.

[robliao, 2014/02/28 20:39:49]

We can use the number, but for enums it shouldn't matter what type they are.
They are enums and comparisons should be done against them to respect the
abstraction.
On 2014/02/28 20:21:05, vadimt wrote:

+        reject();
+      } else {
+        resolve(defaultStorageObject);
+      }
+    });
+  });
+}
+
+/**
  * 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) {
   /**
    * Queue of scheduled tasks. The first element, if present, corresponds to the
    * currently running task.
@@ skipping 220 matching lines @@
     chrome.alarms.clear(alarmName);
     chrome.storage.local.remove(currentDelayStorageKey);
   }
 
   /**
    * Plans for the next attempt.
    * @param {function()} callback Completion callback. It will be invoked after
    *     the planning is done.
    */
   function planForNext(callback) {
-    instrumented.storage.local.get(currentDelayStorageKey, function(items) {
-      if (!items) {
-        items = {};
-        items[currentDelayStorageKey] = maximumDelaySeconds;
-      }
+    var request = {};
+    request[currentDelayStorageKey] = maximumDelaySeconds;

[vadimt, 2014/02/27 20:20:36]

This changes the semantics. If items retrieved, but don't contains
currentDelayStorageKey, the old semantics was to leave it undefined, and the new
one is to assign maximumDelaySeconds.
Is this the intent?

[robliao, 2014/02/27 23:52:36]

Gotcha. Fixed.
On 2014/02/27 20:20:36, vadimt wrote:

+    fillFromChromeLocalStorage(request).then(function(items) {
       console.log('planForNext-get-storage ' + JSON.stringify(items));
       scheduleNextAttempt(items[currentDelayStorageKey]);
       callback();
     });
   }
 
   instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       isRunning(function(running) {
         if (running)
@@ skipping 73 matching lines @@
   function addListener(callback) {
     listeners.push(callback);
   }
 
   /**
    * Checks if the last signed in state matches the current one.
    * If it doesn't, it notifies the listeners of the change.
    */
   function checkAndNotifyListeners() {
     isSignedIn().then(function(signedIn) {
-      instrumented.storage.local.get('lastSignedInState', function(items) {
-        items = items || {};
-        if (items.lastSignedInState != signedIn) {
-          chrome.storage.local.set(
-              {lastSignedInState: signedIn});
-          listeners.forEach(function(callback) {
-            callback();
-          });
-        }
+      fillFromChromeLocalStorage({lastSignedInState: undefined})
+          .then(function(items) {
+            if (items.lastSignedInState != signedIn) {
+              chrome.storage.local.set(
+                  {lastSignedInState: signedIn});
+              listeners.forEach(function(callback) {
+                callback();
+              });
+            }
+        });
       });
-    });
   }
 
   instrumented.identity.onSignInChanged.addListener(function() {
     checkAndNotifyListeners();
   });
 
   instrumented.alarms.onAlarm.addListener(function(alarm) {
     if (alarm.name == alarmName)
       checkAndNotifyListeners();
   });
 
   // Poll for the sign in state every hour.
   // 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
   };
 }