Google Now Card Processing Pipeline Refactor

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.
  * The service performs periodic updating of Google Now cards.
  * Each updating of the cards includes 4 steps:
  * 1. Processing requests for cards dismissals that are not yet sent to the
  *    server.
  * 2. Making a server request.
  * 3. Showing the received cards as notifications.
  */
 
-// TODO(vadimt): Decide what to do in incognito mode.
-// TODO(vadimt): Figure out the final values of the constants.
+// TODO(robliao): Decide what to do in incognito mode.
 
 /**
  * Standard response code for successful HTTP requests. This is the only success
  * code the server will send.
  */
 var HTTP_OK = 200;
 var HTTP_NOCONTENT = 204;
 
 var HTTP_BAD_REQUEST = 400;
 var HTTP_UNAUTHORIZED = 401;
@@ skipping 294 matching lines @@
         resolve(request);
       }, false);
       request.send();
     });
     requestPromise.then(checkAuthenticationStatus(token));
     return requestPromise;
   });
 }
 
 /**
- * Shows parsed and combined cards as notifications.
+ * Shows the notification groups as notification cards.
  * @param {Object.<string, StoredNotificationGroup>} notificationGroups Map from
  *     group name to group information.
- * @param {Object.<ChromeNotificationId, CombinedCard>} cards Map from
- *     chromeNotificationId to the combined card, containing cards to show.
- * @param {function()} onSuccess Called on success.
- * @param {function(ReceivedNotification)=} onCardShown Optional parameter
+ * @param {function(ReceivedNotification)=} opt_onCardShown Optional parameter
  *     called when each card is shown.
+ * @return {Promise} A promise to show the notification groups as cards.
  */
-function showNotificationCards(
-    notificationGroups, cards, onSuccess, onCardShown) {
-  console.log('showNotificationCards ' + JSON.stringify(cards));
+function showNotificationGroups(notificationGroups, opt_onCardShown) {
+  var cards = calculateCards(notificationGroups);
+  console.log('showNotificationGroups ' + JSON.stringify(cards));
 
-  instrumented.notifications.getAll(function(notifications) {
-    console.log('showNotificationCards-getAll ' +
-        JSON.stringify(notifications));
-    notifications = notifications || {};
+  return new Promise(function(resolve) {
+    instrumented.notifications.getAll(function(notifications) {
+      console.log('showNotificationGroups-getAll ' +
+          JSON.stringify(notifications));
+      notifications = notifications || {};
 
-    // Mark notifications that didn't receive an update as having received
-    // an empty update.
-    for (var chromeNotificationId in notifications) {
-      cards[chromeNotificationId] = cards[chromeNotificationId] || [];
-    }
+      // Mark notifications that didn't receive an update as having received
+      // an empty update.
+      for (var chromeNotificationId in notifications) {
+        cards[chromeNotificationId] = cards[chromeNotificationId] || [];
+      }
 
-    /** @type {Object.<string, NotificationDataEntry>} */
-    var notificationsData = {};
+      /** @type {Object.<string, NotificationDataEntry>} */
+      var notificationsData = {};
 
-    // Create/update/delete notifications.
-    for (var chromeNotificationId in cards) {
-      notificationsData[chromeNotificationId] = cardSet.update(
-          chromeNotificationId,
-          cards[chromeNotificationId],
-          notificationGroups,
-          onCardShown);
-    }
-    chrome.storage.local.set({notificationsData: notificationsData});
-    onSuccess();
+      // Create/update/delete notifications.
+      for (var chromeNotificationId in cards) {
+        notificationsData[chromeNotificationId] = cardSet.update(
+            chromeNotificationId,
+            cards[chromeNotificationId],
+            notificationGroups,
+            opt_onCardShown);
+      }
+      chrome.storage.local.set({notificationsData: notificationsData});
+      resolve();
+    });
   });
 }
 
 /**
  * Removes all cards and card state on Google Now close down.
  */
 function removeAllCards() {
   console.log('removeAllCards');
 
   // TODO(robliao): Once Google Now clears its own checkbox in the
@@ skipping 68 matching lines @@
         'GoogleNow', function(params) {
       var optinPollPeriodSeconds =
           parseInt(params && params.optinPollPeriodSeconds, 10) ||
           DEFAULT_OPTIN_CHECK_PERIOD_SECONDS;
       updateCardsAttempts.start(optinPollPeriodSeconds);
     });
   }
 }
 
 /**
  * Combines notification groups into a set of Chrome notifications and shows

[rgustafson, 2014/04/01 23:51:48]

minus "and shows them"

[robliao, 2014/04/02 00:42:05]

Done.

  * them.
  * @param {Object.<string, StoredNotificationGroup>} notificationGroups Map from
  *     group name to group information.
- * @param {function()} onSuccess Called on success.
- * @param {function(ReceivedNotification)=} onCardShown Optional parameter
- *     called when each card is shown.
+ * @return {Object.<ChromeNotificationId, CombinedCard>} Cards to show.
  */
-function combineAndShowNotificationCards(
-    notificationGroups, onSuccess, onCardShown) {
-  console.log('combineAndShowNotificationCards ' +
-      JSON.stringify(notificationGroups));
+function calculateCards(notificationGroups) {

[rgustafson, 2014/04/01 23:51:48]

Not liking the name of this.

I would like combineCards but there's confusion with combineGroup. The
distinction here is between a single group and multiple groups. More verbose,
but possible combineCardsFromGroups?

[robliao, 2014/04/02 00:42:05]

Done.

+  console.log('calculateCards ' + JSON.stringify(notificationGroups));
   /** @type {Object.<ChromeNotificationId, CombinedCard>} */
   var combinedCards = {};
 
   for (var groupName in notificationGroups)
     combineGroup(combinedCards, notificationGroups[groupName]);
 
-  showNotificationCards(
-      notificationGroups, combinedCards, onSuccess, onCardShown);
+  return combinedCards;
 }
 
 /**
  * Based on a response from the notification server, shows notifications and

[rgustafson, 2014/04/01 23:51:48]

doesn't show notifications, update comment

[robliao, 2014/04/02 00:42:05]

Done.

  * schedules next update.
  * @param {ServerResponse} response Server response.
- * @param {function(ReceivedNotification)=} onCardShown Optional parameter
- *     called when each card is shown.
+ * @return {Promise} A promise to process the server response and provide
+ *     updated groups. Rejects if the server response shouldn't be processed.
  */
-function processServerResponse(response, onCardShown) {
+function processServerResponse(response) {
   console.log('processServerResponse ' + JSON.stringify(response));
 
   if (response.googleNowDisabled) {
     chrome.storage.local.set({googleNowEnabled: false});
-    // TODO(vadimt): Remove the line below once the server stops sending groups
+    // TODO(robliao): Remove the line below once the server stops sending groups
     // with 'googleNowDisabled' responses.
     response.groups = {};
     // Google Now was enabled; now it's disabled. This is a state change.
     onStateChange();
+    return Promise.reject();
   }
 
   var receivedGroups = response.groups;
 
-  fillFromChromeLocalStorage({
+  return fillFromChromeLocalStorage({
     /** @type {Object.<string, StoredNotificationGroup>} */
     notificationGroups: {},
     /** @type {Object.<NotificationId, number>} */
     recentDismissals: {}
   }).then(function(items) {
     console.log('processServerResponse-get ' + JSON.stringify(items));
 
     // Build a set of non-expired recent dismissals. It will be used for
     // client-side filtering of cards.
     /** @type {Object.<NotificationId, number>} */
@@ skipping 52 matching lines @@
       // cards updates.
       if (receivedGroup.nextPollSeconds !== undefined) {
         storedGroup.nextPollTime =
             now + receivedGroup.nextPollSeconds * MS_IN_SECOND;
       }
 
       updatedGroups[groupName] = storedGroup;
     }
 
     scheduleNextPoll(updatedGroups, !response.googleNowDisabled);
-    combineAndShowNotificationCards(
-        updatedGroups,
-        function() {
-          chrome.storage.local.set({
-            notificationGroups: updatedGroups,
-            recentDismissals: updatedRecentDismissals
-          });
-          recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
-        },
-        onCardShown);
+    return {
+      updatedGroups: updatedGroups,
+      recentDismissals: updatedRecentDismissals
+    };
   });
 }
 
 /**
  * Update the Explanatory Total Cards Shown Count.
  */
 function countExplanatoryCard() {
   localStorage['explanatoryCardsShown']++;
 }
 
 /**
+ * Determines if cards should have an explanation link.
+ * @return {boolean} true if an explanatory card should be shown.
+ */
+function shouldShowExplanatoryCard() {
+  var isBelowThreshold =

[rgustafson, 2014/04/01 23:51:48]

need the extra var?

[robliao, 2014/04/02 00:42:05]

Yup. Seemed the lesser of three evils:

// Breaks on lint due to line break after return.
return
    localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD;

// Just looks ugly
return localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD;


On 2014/04/01 23:51:48, rgustafson wrote:

[robliao, 2014/04/02 00:43:01]

Huh, that's supposed to be...
// Just looks ugly
return localStorage['explanatoryCardsShown'] < 
    EXPLANATORY_CARDS_LINK_THRESHOLD;

On 2014/04/02 00:42:05, robliao wrote:

+      localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD;
+  return isBelowThreshold;
+}
+
+/**
  * Requests notification cards from the server for specified groups.
  * @param {Array.<string>} groupNames Names of groups that need to be refreshed.
+ * @return {Promise} A promise to request the specified notification groups.
  */
-function requestNotificationGroups(groupNames) {
-  console.log('requestNotificationGroups from ' + NOTIFICATION_CARDS_URL +
+function requestNotificationGroupsFromServer(groupNames) {
+  console.log(
+      'requestNotificationGroupsFromServer from ' + NOTIFICATION_CARDS_URL +
       ', groupNames=' + JSON.stringify(groupNames));
 
   recordEvent(GoogleNowEvent.REQUEST_FOR_CARDS_TOTAL);
 
   var requestParameters = '?timeZoneOffsetMs=' +
     (-new Date().getTimezoneOffset() * MS_IN_MINUTE);
 
-  var cardShownCallback = undefined;
-  var belowExplanatoryThreshold =
-      localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD;
-  if (belowExplanatoryThreshold) {
+  if (shouldShowExplanatoryCard()) {
     requestParameters += '&cardExplanation=true';
-    cardShownCallback = countExplanatoryCard;
   }
 
   groupNames.forEach(function(groupName) {
     requestParameters += ('&requestTypes=' + groupName);
   });
 
   requestParameters += '&uiLocale=' + navigator.language;
 
-  console.log('requestNotificationGroups: request=' + requestParameters);
+  console.log(
+      'requestNotificationGroupsFromServer: request=' + requestParameters);
 
-  requestFromServer('GET', 'notifications' + requestParameters).then(
+  return requestFromServer('GET', 'notifications' + requestParameters).then(
     function(request) {
-      console.log('requestNotificationGroups-received ' + request.status);
+      console.log(
+          'requestNotificationGroupsFromServer-received ' + request.status);
       if (request.status == HTTP_OK) {
         recordEvent(GoogleNowEvent.REQUEST_FOR_CARDS_SUCCESS);
-        processServerResponse(
-            JSON.parse(request.responseText), cardShownCallback);
+        return JSON.parse(request.responseText);
       }
     });
 }
 
 /**
- * Requests the account opted-in state from the server.
- * @param {function()} optedInCallback Function that will be called if
- *     opted-in state is 'true'.
+ * Requests the account opted-in state from the server and updates any
+ * state as necessary.
+ * @return {Promise} A promise to request and update the opted-in state.
+ *     The promise resolves if the opt-in state is true.
  */
-function requestOptedIn(optedInCallback) {
+function requestAndUpdateOptedIn() {
   console.log('requestOptedIn from ' + NOTIFICATION_CARDS_URL);
 
-  requestFromServer('GET', 'settings/optin').then(function(request) {
+  return requestFromServer('GET', 'settings/optin').then(function(request) {
     console.log(
         'requestOptedIn-received ' + request.status + ' ' + request.response);
     if (request.status == HTTP_OK) {
       var parsedResponse = JSON.parse(request.responseText);
-      if (parsedResponse.value) {
-        chrome.storage.local.set({googleNowEnabled: true});
-        optedInCallback();
-        // Google Now was disabled, now it's enabled. This is a state change.
-        onStateChange();
-      } else {
-        scheduleNextPoll({}, false);
-      }
+      return parsedResponse.value;
+    } else {
+      return Promise.reject();
+    }
+  }).then(function(optedIn) {
+    if (optedIn) {
+      chrome.storage.local.set({googleNowEnabled: true});
+      // Google Now was disabled, now it's enabled. This is a state change.
+      onStateChange();
+      return Promise.resolve();
+    } else {
+      scheduleNextPoll({}, false);
+      return Promise.reject();
     }
   });
 }
 
 /**
- * Requests notification cards from the server.
+ * Determines the groups that need to be requested right now.
+ * @return {Promise} A promise to determine the groups to request.
  */
-function requestNotificationCards() {
-  console.log('requestNotificationCards');
-
-  fillFromChromeLocalStorage({
+function getGroupsToRequest() {
+  return fillFromChromeLocalStorage({
     /** @type {Object.<string, StoredNotificationGroup>} */
-    notificationGroups: {},
-    googleNowEnabled: false
+    notificationGroups: {}
   }).then(function(items) {
     console.log(
-        'requestNotificationCards-storage-get ' + JSON.stringify(items));
-
+        'getGroupsToRequest-storage-get ' + JSON.stringify(items));

[rgustafson, 2014/04/01 23:51:48]

move onto previous line

[robliao, 2014/04/02 00:42:05]

Done.

     var groupsToRequest = [];
-
     var now = Date.now();
 
     for (var groupName in items.notificationGroups) {
       var group = items.notificationGroups[groupName];
       if (group.nextPollTime !== undefined && group.nextPollTime <= now)
         groupsToRequest.push(groupName);
     }
-
-    if (items.googleNowEnabled) {
-      requestNotificationGroups(groupsToRequest);
-    } else {
-      requestOptedIn(function() {
-        requestNotificationGroups(groupsToRequest);
-      });
-    }
+    return groupsToRequest;
   });
 }
 
+/**
+ * Requests notification cards from the server.
+ * @return {Promise} A promise to request the notification cards.
+ *     Rejects if the cards won't be requested.
+ */
+function requestNotificationCards() {
+  console.log('requestNotificationCards');
+
+  return isGoogleNowEnabled().then(function(googleNowEnabled) {

[rgustafson, 2014/04/01 23:51:48]

The indentation on all the thens is off. Also easier to read if all of the thens
start on a new line.

(I realize now that there's been skimping on indentation from .s, but this one
makes it really obvious)

[robliao, 2014/04/02 00:42:05]

After a fun discussion, we're going to punt the issue by putting the first then
on the next line.
On 2014/04/01 23:51:48, rgustafson wrote:

+    return googleNowEnabled ? Promise.resolve() : requestAndUpdateOptedIn();
+  }).then(getGroupsToRequest)
+  .then(requestNotificationGroupsFromServer)
+  .then(processServerResponse)
+  .then(function(processedResponse) {
+    var onCardShown =
+        shouldShowExplanatoryCard() ? countExplanatoryCard : undefined;
+    return showNotificationGroups(processedResponse.updatedGroups, onCardShown)
+        .then(function() {
+          chrome.storage.local.set({
+            notificationGroups: processedResponse.updatedGroups,
+            recentDismissals: processedResponse.updatedRecentDismissals
+          });
+          recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+        }
+      );
+  });
+}
+
 /**
  * Requests and shows notification cards.
  */
 function requestCards() {
   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() {
@@ skipping 220 matching lines @@
 }
 
 /**
  * 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
   // cards gets stuck.
   updateCardsAttempts.start(MAXIMUM_POLLING_PERIOD_SECONDS);
-
   requestCards();
 }
 
 /**
  * Stops all machinery in the polling system.
  */
 function stopPollingCards() {
   console.log('stopPollingCards');
   updateCardsAttempts.stop();
   removeAllCards();
-  // Mark the Google Now as disabled to start with checking the opt-in state
-  // next time startPollingCards() is called.
-  chrome.storage.local.set({googleNowEnabled: false});
+  // Since we're stopping everything, clear all storage too.
+  chrome.storage.local.clear();
 }
 
 /**
  * Initializes the event page on install or on browser startup.
  */
 function initialize() {
   recordEvent(GoogleNowEvent.EXTENSION_START);
   onStateChange();
 }
 
@@ skipping 162 matching lines @@
  */
 function pollOptedIn() {
   /**
    * Cleans up any state used to recheck the opt-in poll.
    */
   function clearPollingState() {
     localStorage.removeItem('optedInCheckCount');
     optInCheckAttempts.stop();
   }
 
-  /**
-   * Performs the actual work for checking the opt-in state and requesting cards
-   * on opted-in.
-   */
-  function checkOptedIn() {
-    // Limit retries to 5.
-    if (localStorage.optedInCheckCount < 5) {
-      console.log(new Date() +
-          ' checkOptedIn Attempt ' + localStorage.optedInCheckCount);
-      localStorage.optedInCheckCount++;
-      requestOptedIn(function() {
-        clearPollingState();
-        requestCards();
-      });
-    } else {
-      clearPollingState();
-    }
-  }
-
   if (localStorage.optedInCheckCount === undefined) {
     localStorage.optedInCheckCount = 0;
     optInCheckAttempts.start();
-    checkOptedIn();
-  } else {
-    optInCheckAttempts.planForNext(checkOptedIn);
   }
+
+  console.log(new Date() +
+      ' checkOptedIn Attempt ' + localStorage.optedInCheckCount);
+
+  requestAndUpdateOptedIn().then(function() {
+    clearPollingState();
+    requestCards();
+  }).catch(function() {
+    localStorage.optedInCheckCount++;

[rgustafson, 2014/04/01 23:51:48]

Why outside the if? also, counting before = one less.

[robliao, 2014/04/02 00:42:05]

:-D. Nice catch. This was fixed but never uploaded.
On 2014/04/01 23:51:48, rgustafson wrote:

+    if (localStorage.optedInCheckCount < 5) {
+      optInCheckAttempts.planForNext(function() {});

[rgustafson, 2014/04/01 23:51:48]

Make the param opt?

[robliao, 2014/04/02 00:42:05]

Maybe later. I want to rework the alarms.
On 2014/04/01 23:51:48, rgustafson wrote:

+    } else {
+      clearPollingState();
+    }
+  });
 }
 
 instrumented.runtime.onInstalled.addListener(function(details) {
   console.log('onInstalled ' + JSON.stringify(details));
   if (details.reason != 'chrome_update') {
     initialize();
   }
 });
 
 instrumented.runtime.onStartup.addListener(function() {
   console.log('onStartup');
 
   // Show notifications received by earlier polls. Doing this as early as
   // possible to reduce latency of showing first notifications. This mimics how
   // persistent notifications will work.
   tasks.add(SHOW_ON_START_TASK_NAME, function() {
     fillFromChromeLocalStorage({
       /** @type {Object.<string, StoredNotificationGroup>} */
       notificationGroups: {}
     }).then(function(items) {
       console.log('onStartup-get ' + JSON.stringify(items));
 
-      combineAndShowNotificationCards(items.notificationGroups, function() {
+      showNotificationGroups(items.notificationGroups).then(function() {
         chrome.storage.local.set(items);
       });
     });
   });
 
   initialize();
 });
 
 authenticationManager.addListener(function() {
   console.log('signIn State Change');
@@ skipping 71 matching lines @@
             lastPollNowPayloads: items.lastPollNowPayloads,
             notificationGroups: items.notificationGroups
           });
 
           pollOptedIn();
         }
       });
     });
   }
 });