Restoring notifications on startup

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 da293eaaf19e29c258896a91f6a69ccff6a4a0c0..4c6732f8ff43ddb42ae042ce84f247300377f903 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -20,7 +20,6 @@
 // TODO(vadimt): Decide what to do in incognito mode.
 // TODO(vadimt): Figure out the final values of the constants.
 // TODO(vadimt): Remove 'console' calls.
-// TODO(vadimt): Consider sending JS stacks for malformed server responses.
 
 /**
  * Standard response code for successful HTTP requests. This is the only success
@@ -81,6 +80,7 @@ var UPDATE_CARDS_TASK_NAME = 'update-cards';
 var DISMISS_CARD_TASK_NAME = 'dismiss-card';
 var RETRY_DISMISS_TASK_NAME = 'retry-dismiss';
 var STATE_CHANGED_TASK_NAME = 'state-changed';
+var SHOW_ON_START_TASK_NAME = 'show-cards-on-start';
 
 var LOCATION_WATCH_NAME = 'location-watch';
 
@@ -317,8 +317,6 @@ function showNotificationCards(cards) {
                 previousVersion);
           }
 
-          recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
-
           chrome.storage.local.set({
             notificationsData: newNotificationsData,
             recentDismissals: updatedRecentDismissals
@@ -447,6 +445,26 @@ function scheduleNextPoll(groups) {
 }
 
 /**
+ * Merges stored notification groups into a set of Chrome notifications and
+ * shows them.
+ */
+function mergeAndShowNotificationCards() {
+  instrumented.storage.local.get('notificationGroups', function(items) {
+    console.log('mergeAndShowNotificationCards-get ' + JSON.stringify(items));
+    items = items || {};
+    items.notificationGroups = items.notificationGroups || {};
+
+    // Merge cards from all groups into one set.
+    var mergedCards = {};
+
+    for (var groupName in items.notificationGroups)
+      mergeGroup(mergedCards, items.notificationGroups[groupName]);
+
+    showNotificationCards(mergedCards);
+  });
+}
+
+/**
  * Parses JSON response from the notification server, shows notifications and
  * schedules next update.
  * @param {string} response Server response.
@@ -455,13 +473,13 @@ function parseAndShowNotificationCards(response) {
   console.log('parseAndShowNotificationCards ' + response);
   var parsedResponse = JSON.parse(response);
 
-  var groups = parsedResponse.groups;
+  var receivedGroups = parsedResponse.groups;
 
   // Populate groups with corresponding cards.
   if (parsedResponse.notifications) {
     for (var i = 0; i != parsedResponse.notifications.length; ++i) {
       var card = parsedResponse.notifications[i];
-      var group = groups[card.groupName];
+      var group = receivedGroups[card.groupName];
       group.cards = group.cards || [];
       group.cards.push(card);
     }
@@ -474,12 +492,9 @@ function parseAndShowNotificationCards(response) {
 
     var now = Date.now();
 
-    // Build updated set of groups and merge cards from all groups into one set.
-    var updatedGroups = {};
-    var mergedCards = {};
-
-    for (var groupName in groups) {
-      var receivedGroup = groups[groupName];
+    // Build updated set of groups.
+    for (var groupName in receivedGroups) {
+      var receivedGroup = receivedGroups[groupName];
       var storageGroup = items.notificationGroups[groupName] || {
         cards: [],
         cardsTimestamp: undefined,
@@ -501,16 +516,13 @@ function parseAndShowNotificationCards(response) {
             now + receivedGroup.nextPollSeconds * MS_IN_SECOND;
       }
 
-      updatedGroups[groupName] = storageGroup;
-
-      mergeGroup(mergedCards, storageGroup);
+      items.notificationGroups[groupName] = storageGroup;
     }
 
-    scheduleNextPoll(updatedGroups);
-
-    chrome.storage.local.set({notificationGroups: updatedGroups});
-
-    showNotificationCards(mergedCards);
+    scheduleNextPoll(items.notificationGroups);
+    chrome.storage.local.set({notificationGroups: items.notificationGroups});
+    mergeAndShowNotificationCards();

[robliao, 2013/10/14 21:05:31]

Seems like overkill to set notificationGroups and then immediately read it again
in this function.

A better way would be to abstract the get out of mergeAndShowNotificationCards
and have it accept a parameter that is simply the notificationGroups.

[vadimt, 2013/10/14 23:51:57]

I'd not do serious code refactorings, assuming that persistent notifications are
coming. And the perf benefit would be not noticeable, given that we do this only
once per Chrome's process life (per session).

[robliao, 2013/10/14 23:56:08]

This rearrangement only affects new code, namely mergeAndShowNotificationCards.

mergeAndShowNotificationCards can simply remove the get call and receive the
cards as an argument. 

On startup, you can add GetMergeAndShowNotificaitonCards that does the get step
and calls mergeAndShowNotificationCards.

This makes the startup path obvious that we're obtaining state and avoids the
whole set re-get code.

On 2013/10/14 23:51:57, vadimt wrote:

[vadimt, 2013/10/15 01:09:44]

Done.

+    recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
   });
 }
 
@@ -876,12 +888,6 @@ function stopPollingCards() {
  */
 function initialize() {
   recordEvent(GoogleNowEvent.EXTENSION_START);
-
-  // Alarms persist across chrome restarts. This is undesirable since it
-  // prevents us from starting up everything (alarms are a heuristic to
-  // determine if we are already running). To mitigate this, we will
-  // shut everything down on initialize before starting everything up.
-  stopPollingCards();
   onStateChange();
 }
 
@@ -1085,6 +1091,10 @@ instrumented.runtime.onInstalled.addListener(function(details) {
 
 instrumented.runtime.onStartup.addListener(function() {
   console.log('onStartup');
+
+  // Show notifications received by earlier polls.
+  tasks.add(SHOW_ON_START_TASK_NAME, mergeAndShowNotificationCards);

[robliao, 2013/10/14 21:05:31]

This should occur after initialize.

[vadimt, 2013/10/14 23:51:57]

Why? Imagine that persistent notifications are implemented. Then, cards would be
restored very early, may be, before any JS code ran at all.

[robliao, 2013/10/14 23:56:08]

It is non-obvious to an outside reader why anything should occur before an
initialize step.
On 2013/10/14 23:51:57, vadimt wrote:

[vadimt, 2013/10/15 01:09:44]

Done.

+
   initialize();
 });