Refactor Chrome Now Card Dismissal Stack 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 7dca5fef50d265b0c500000c0cd5d382edc207e0..6f69a95a3bca986f885a6c9a57ea2eb84737d7f7 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -650,11 +650,9 @@ function requestCards() {
     updateCardsAttempts.isRunning(function(running) {
       if (running) {
         updateCardsAttempts.planForNext(function() {
-          processPendingDismissals(function(success) {
-            if (success) {
-              // The cards are requested only if there are no unsent dismissals.
-              requestNotificationCards();
-            }
+          processPendingDismissals().then(function() {

[skare_, 2014/03/07 01:57:51]

does
.then(requestNotificationCards)
work?

[robliao, 2014/03/07 18:14:47]

Yup! That should work.
On 2014/03/07 01:57:51, Travis Skare wrote:

+            // The cards are requested only if there are no unsent dismissals.
+            requestNotificationCards();
           });
         });
       }
@@ -669,11 +667,10 @@ function requestCards() {
  * @param {number} dismissalTimeMs Time of the user's dismissal of the card in
  *     milliseconds since epoch.
  * @param {DismissalData} dismissalData Data to build a dismissal request.
- * @param {function(boolean)} callbackBoolean Completion callback with 'done'
- *     parameter.
+ * @return {Promise} A promise to request the card dimissal, rejects on error.

[rgustafson, 2014/03/10 17:43:33]

dismissal

[robliao, 2014/03/10 19:46:14]

Done.

  */
 function requestCardDismissal(
-    chromeNotificationId, dismissalTimeMs, dismissalData, callbackBoolean) {
+    chromeNotificationId, dismissalTimeMs, dismissalData) {
   console.log('requestDismissingCard ' + chromeNotificationId +
       ' from ' + NOTIFICATION_CARDS_URL +
       ', dismissalData=' + JSON.stringify(dismissalData));
@@ -697,7 +694,7 @@ function requestCardDismissal(
 
   console.log('requestCardDismissal: requestParameters=' + requestParameters);
 
-  requestFromServer('DELETE', requestParameters).then(function(request) {
+  return requestFromServer('DELETE', requestParameters).then(function(request) {
     console.log('requestDismissingCard-onloadend ' + request.status);
     if (request.status == HTTP_NOCONTENT)
       recordEvent(GoogleNowEvent.DISMISS_REQUEST_SUCCESS);
@@ -707,19 +704,17 @@ function requestCardDismissal(
     var done = request.status == HTTP_NOCONTENT ||
         request.status == HTTP_BAD_REQUEST ||
         request.status == HTTP_METHOD_NOT_ALLOWED;
-    callbackBoolean(done);
-  }).catch(function() {
-    callbackBoolean(false);
+    return done ? Promise.resolve() : Promise.reject();
   });
 }
 
 /**
  * Tries to send dismiss requests for all pending dismissals.
- * @param {function(boolean)} callbackBoolean Completion callback with 'success'
- *     parameter. Success means that no pending dismissals are left.
+ * @return {Promise} A promise to process the pending dismissals.
+ *     The promise is rejected if a problem was encountered.
  */
-function processPendingDismissals(callbackBoolean) {
-  fillFromChromeLocalStorage({
+function processPendingDismissals() {
+  return fillFromChromeLocalStorage({

[skare_, 2014/03/07 01:57:51]

general comment on this pattern: this does make sense but a return statement
covering almost 50 lines was a little confusing on first read. Maybe cache in a
local?

I'll defer to a second opinion though.

[robliao, 2014/03/07 18:14:47]

Would we do the same if we were returning a very long function?
On 2014/03/07 01:57:51, Travis Skare wrote:

     /** @type {Array.<PendingDismissal>} */
     pendingDismissals: [],
     /** @type {Object.<NotificationId, number>} */
@@ -737,39 +732,35 @@ function processPendingDismissals(callbackBoolean) {
           recentDismissals: items.recentDismissals
         });
       }
-      callbackBoolean(success);
+      return success ? Promise.resolve() : Promise.reject();
     }
 
     function doProcessDismissals() {
       if (items.pendingDismissals.length == 0) {
         dismissalAttempts.stop();
-        onFinish(true);
-        return;
+        return onFinish(true);
       }
 
       // Send dismissal for the first card, and if successful, repeat
       // recursively with the rest.
       /** @type {PendingDismissal} */
       var dismissal = items.pendingDismissals[0];
-      requestCardDismissal(
+      return requestCardDismissal(
           dismissal.chromeNotificationId,
           dismissal.time,
-          dismissal.dismissalData,
-          function(done) {
-            if (done) {
-              dismissalsChanged = true;
-              items.pendingDismissals.splice(0, 1);
-              items.recentDismissals[
-                  dismissal.dismissalData.notificationId] =
-                  Date.now();
-              doProcessDismissals();
-            } else {
-              onFinish(false);
-            }
+          dismissal.dismissalData).then(function() {
+            dismissalsChanged = true;
+            items.pendingDismissals.splice(0, 1);
+            items.recentDismissals[
+                dismissal.dismissalData.notificationId] =

[rgustafson, 2014/03/10 17:43:33]

move this onto previous line

[robliao, 2014/03/10 19:46:14]

Done.

+                Date.now();
+            return doProcessDismissals();
+          }).catch(function() {
+            return onFinish(false);
           });
     }
 
-    doProcessDismissals();
+    return doProcessDismissals();
   });
 }
 
@@ -779,7 +770,7 @@ function processPendingDismissals(callbackBoolean) {
 function retryPendingDismissals() {
   tasks.add(RETRY_DISMISS_TASK_NAME, function() {
     dismissalAttempts.planForNext(function() {
-      processPendingDismissals(function(success) {});
+      processPendingDismissals();
      });
   });
 }
@@ -876,7 +867,7 @@ function onNotificationClosed(chromeNotificationId, byUser) {
 
       chrome.storage.local.set(items);
 
-      processPendingDismissals(function(success) {});
+      processPendingDismissals();
     });
   });
 }