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 = combineCardsFromGroups(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
- * them.
+ * Combines notification groups into a set of Chrome notifications.
  * @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 combineCardsFromGroups(notificationGroups) {
+  console.log('combineCardsFromGroups ' + 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
- * schedules next update.
+ * Processes a server response for consumption by showNotificationGroups.
  * @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();
+    // Allow this to continue. We still need to do work here to clear

[rgustafson, 2014/04/02 22:23:09]

On first read, I thought this data would be cleared by stopPollingCards, but
that never gets called. Why not build card data removal into onStateChange when
we will get into the opt in request state? Most of the work lower in this
function and all work in the next then() does not need to be performed.

Here and line 658 require scheduleNextPoll(anything, false) functionality. I'd
tear the false part of the if statement into a separate function, call that and
then continue to reject rather than this fall through.

[robliao, 2014/04/02 23:24:10]

I fell into the same trap. The optin portions here are tricky (and worth
clearing up).

It's an excellent thing to do on a future refactor.

On 2014/04/02 22:23:09, rgustafson wrote:

+    // the cards and schedule the next opt-in poll.
   }
 
   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 =
+      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));
-
+    console.log('getGroupsToRequest-storage-get ' + JSON.stringify(items));
     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) {
+        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() {
     console.log('requestCards-task-begin');
@@ skipping 219 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();

[skare_, 2014/04/03 00:11:59]

this doesn't clear localStorage, correct? They are stored in different places
IIRC and chrome.storage.local is richer, but I'm not 100% on the edge cases and
interactions.

if localStorage is affected, do we want to keep explanatoryCardsShown?
if localStorage is unaffected, maybe update the comment as to what it clears.

more generally would this be unexpected for future developers?

[robliao, 2014/04/03 00:43:22]

localStorage is unaffected with this clearing.

The code currently stores runtime information in chrome.storage.local (cards,
timer info, signed in info).

localStorage, on the other hand, has been used for persistent things (number of
explanatory cards, debug info) and for counters.

I've added a comment.
On 2014/04/03 00:11:59, Travis Skare wrote:

 }
 
 /**
  * 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() {
+    if (localStorage.optedInCheckCount < 5) {
+      localStorage.optedInCheckCount++;
+      optInCheckAttempts.planForNext(function() {});
+    } 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();
         }
       });
     });
   }
 });