Convert Google Now's State Change Gathering Mechanism to use Promises

chrome/browser/resources/google_now/background.js

Index: chrome/browser/resources/google_now/background.js
diff --git a/chrome/browser/resources/google_now/background.js b/chrome/browser/resources/google_now/background.js
index 400409e630bec2dc23485853748e5ab47a3f700a..bd8042299efed252bff972ff4b32512bca1efd19 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -1087,34 +1087,80 @@ function updateRunningState(
  */
 function onStateChange() {
   tasks.add(STATE_CHANGED_TASK_NAME, function() {
+    Promise.all([
+        isSignedIn(),

[skare_, 2014/02/11 20:07:44]

consider
.all([
  new Promise(isSignedIn),
  new Promise(isGeolocationEnabled),
  ...]);

where the functions are along the lines of
function isX(onSuccess) {
  onSuccess(...);
}

?

IMO from a first pass, that's fewer lines of code, a little easier to read/write
and maybe a little easier to unit test individual functions though that's not a
big deal in this case.

if not, there's is a comment below.

[robliao, 2014/02/11 21:53:21]

An async operation hands out promises to encapsulate the async behavior. This
way you can group them together (like this code does) or chain them (which we
will see later).

Wrapping everything with 'new Promise' will get out of hand with chain comes
along because you'll have to do this:
new Promise(calculateResultFunc)
    .then(function(result) { return new Promise(consumeResultFunc(result)); }
    .then(function(result) { return new Promise(setResultFunc(result)); }

instead of
calculateResult()
    .then(consumeResult)
    .then(setResult);


On 2014/02/11 20:07:44, Travis Skare wrote:

[skare_, 2014/02/12 19:55:58]

ok if it makes things easier later; the suggestion was just based on this patch
in isolation

since this is new and there are different implementations, what docs are you
referencing? I was looking at
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
and since that's on MDN want to make sure my comments are compatible :)

On 2014/02/11 21:53:21, robliao wrote:

[robliao, 2014/02/12 20:57:25]

That's one of them. The other is here:
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts

http://wiki.ecmascript.org/lib/exe/fetch.php?id=harmony%3Aspecification_draft...

Section 25.4 covers promises.
On 2014/02/12 19:55:58, Travis Skare wrote:

+        isGeolocationEnabled(),
+        canEnableBackground(),
+        isNotificationsEnabled(),
+        isGoogleNowEnabled()])
+        .then(function(results) {
+          updateRunningState.apply(null, results);
+        });
+  });
+}
+
+/**
+ * Determines if the user is signed in.
+ * @return {Promise} A promise to evaluate the signed in state.
+ */
+function isSignedIn() {
+  return new Promise(function(onSuccess) {
     authenticationManager.isSignedIn(function(signedIn) {
-      instrumented.metricsPrivate.getVariationParams(
-          'GoogleNow',
-          function(response) {
-            var canEnableBackground =
-                (!response || (response.canEnableBackground != 'false'));
-            instrumented.notifications.getPermissionLevel(function(level) {
-              var notificationEnabled = (level == 'granted');
-              instrumented.
-                preferencesPrivate.
-                googleGeolocationAccessEnabled.
-                get({}, function(prefValue) {
-                  var geolocationEnabled = !!prefValue.value;
-                  instrumented.storage.local.get(
-                      'googleNowEnabled',
-                      function(items) {
-                        var googleNowEnabled =
-                            items && !!items.googleNowEnabled;
-                        updateRunningState(
-                            signedIn,
-                            geolocationEnabled,
-                            canEnableBackground,
-                            notificationEnabled,
-                            googleNowEnabled);
-                      });
-                });
-            });
-          });
+      onSuccess(signedIn);
+    });
+  });
+}
+
+/**
+ * Gets the geolocation enabled preference.
+ * @return {Promise} A promise to get the geolocation enabled preference.
+ */
+function isGeolocationEnabled() {

[skare_, 2014/02/11 20:07:44]

I don't know what the best chrome/google promises style is, maybe because this
might be the first component extension to use them, but  would

var isGeolocationEnabled = new Promise(function(foo) {

});

work to remove the wrapper and isGeolocationEnabled() in the caller?

[robliao, 2014/02/11 21:53:21]

A promise only provides one evaluation. Each new evaluation will require a new
promise.
On 2014/02/11 20:07:44, Travis Skare wrote:

+  return new Promise(function(onSuccess) {
+    instrumented.preferencesPrivate.googleGeolocationAccessEnabled.get(
+        {},
+        function(prefValue) {
+          onSuccess(!!prefValue.value);
+        });
+  });
+}
+
+/**
+ * Determines if background mode should be requested.
+ * @return {Promise} A promise to determine if background can be enabled.
+ */
+function canEnableBackground() {
+  return new Promise(function(onSuccess) {
+    instrumented.metricsPrivate.getVariationParams(
+        'GoogleNow',
+        function(response) {
+          onSuccess(!response || (response.canEnableBackground != 'false'));
+        });
+  });
+}
+
+/**
+ * Checks if Google Now is enabled in the notifications center.
+ * @return {Promise} A promise to determine if Google Now is enabled
+ *     in the notifications center.
+ */
+function isNotificationsEnabled() {
+  return new Promise(function(onSuccess) {
+    instrumented.notifications.getPermissionLevel(function(level) {
+      onSuccess(level == 'granted');
+    });
+  });
+}
+
+/**
+ * Gets the previous Google Now opt-in state.
+ * @return {Promise} A promise to determine the previous Google Now
+ *     opt-in state.
+ */
+function isGoogleNowEnabled() {
+  return new Promise(function(onSuccess) {
+    instrumented.storage.local.get('googleNowEnabled', function(items) {
+      onSuccess(items && !!items.googleNowEnabled);
     });
   });
 }