Refactor Chrome Now Card Dismissal Stack to use Promises

chrome/browser/resources/google_now/background.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 The event page for Google Now for Chrome implementation.
  * The Google Now event page gets Google Now cards from the server and shows
  * them as Chrome notifications.
@@ skipping 632 matching lines @@
   console.log('requestCards @' + new Date());
   // LOCATION_REQUEST is a legacy histogram value when we requested location.
   // This corresponds to the extension attempting to request for cards.
   // We're keeping the name the same to keep our histograms in order.
   recordEvent(GoogleNowEvent.LOCATION_REQUEST);
   tasks.add(UPDATE_CARDS_TASK_NAME, function() {
     console.log('requestCards-task-begin');
     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();
           });
         });
       }
     });
   });
 }
 
 /**
  * Sends a server request to dismiss a card.
  * @param {ChromeNotificationId} chromeNotificationId chrome.notifications ID of
  *     the card.
  * @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));
 
   var dismissalAge = Date.now() - dismissalTimeMs;
 
   if (dismissalAge > MAXIMUM_DISMISSAL_AGE_MS) {
     callbackBoolean(true);
     return;
   }
 
   recordEvent(GoogleNowEvent.DISMISS_REQUEST_TOTAL);
 
   var requestParameters = 'notifications/' + dismissalData.notificationId +
       '?age=' + dismissalAge +
       '&chromeNotificationId=' + chromeNotificationId;
 
   for (var paramField in dismissalData.parameters)
     requestParameters += ('&' + paramField +
     '=' + dismissalData.parameters[paramField]);
 
   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);
 
     // A dismissal doesn't require further retries if it was successful or
     // doesn't have a chance for successful completion.
     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>} */
     recentDismissals: {}
   }).then(function(items) {
     console.log(
         'processPendingDismissals-storage-get ' + JSON.stringify(items));
 
     var dismissalsChanged = false;
 
     function onFinish(success) {
       if (dismissalsChanged) {
         chrome.storage.local.set({
           pendingDismissals: items.pendingDismissals,
           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();
   });
 }
 
 /**
  * Submits a task to send pending dismissals.
  */
 function retryPendingDismissals() {
   tasks.add(RETRY_DISMISS_TASK_NAME, function() {
     dismissalAttempts.planForNext(function() {
-      processPendingDismissals(function(success) {});
+      processPendingDismissals();
      });
   });
 }
 
 /**
  * Opens a URL in a new tab.
  * @param {string} url URL to open.
  */
 function openUrl(url) {
   instrumented.tabs.create({url: url}, function(tab) {
@@ skipping 76 matching lines @@
           dismissalData: dismissalResult.dismissals[i]
         };
         items.pendingDismissals.push(dismissal);
       }
 
       items.notificationsData[chromeNotificationId] =
           dismissalResult.notificationData;
 
       chrome.storage.local.set(items);
 
-      processPendingDismissals(function(success) {});
+      processPendingDismissals();
     });
   });
 }
 
 /**
  * Initializes the polling system to start fetching cards.
  */
 function startPollingCards() {
   console.log('startPollingCards');
   // Create an update timer for a case when for some reason requesting
@@ skipping 254 matching lines @@
             lastPollNowPayloads: items.lastPollNowPayloads,
             notificationGroups: items.notificationGroups
           });
 
           requestCards();
         }
       });
     });
   }
 });