Chrome Now notificationGroups Storage Race Condition Fix

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 1a35d4091e8ca6fc88008580a563cbb16a5ae176..a2782b90144380fc6da14eddebf6abc337f2d2b0 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -293,12 +293,16 @@ function setAuthorization(request, callbackBoolean) {
 
 /**
  * Shows parsed and combined cards as notifications.
+ * @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
  *     called when each card is shown.
  */
-function showNotificationCards(cards, onCardShown) {
+function showNotificationCards(
+    notificationGroups, cards, onSuccess, onCardShown) {
   console.log('showNotificationCards ' + JSON.stringify(cards));
 
   instrumented.notifications.getAll(function(notifications) {
@@ -320,9 +324,11 @@ function showNotificationCards(cards, onCardShown) {
       notificationsData[chromeNotificationId] = cardSet.update(
           chromeNotificationId,
           cards[chromeNotificationId],
+          notificationGroups,
           onCardShown);
     }
     chrome.storage.local.set({notificationsData: notificationsData});
+    onSuccess();
   });
 }
 
@@ -417,10 +423,12 @@ function scheduleNextPoll(groups, isOptedIn) {
  * 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.
  */
-function combineAndShowNotificationCards(notificationGroups, onCardShown) {
+function combineAndShowNotificationCards(
+    notificationGroups, onSuccess, onCardShown) {
   console.log('combineAndShowNotificationCards ' +
       JSON.stringify(notificationGroups));
   /** @type {Object.<ChromeNotificationId, CombinedCard>} */
@@ -429,7 +437,8 @@ function combineAndShowNotificationCards(notificationGroups, onCardShown) {
   for (var groupName in notificationGroups)
     combineGroup(combinedCards, notificationGroups[groupName]);
 
-  showNotificationCards(combinedCards, onCardShown);
+  showNotificationCards(
+      notificationGroups, combinedCards, onSuccess, onCardShown);
 }
 
 /**
@@ -531,12 +540,15 @@ function parseAndShowNotificationCards(response, onCardShown) {
         }
 
         scheduleNextPoll(updatedGroups, !parsedResponse.googleNowDisabled);
-        chrome.storage.local.set({
-          notificationGroups: updatedGroups,
-          recentDismissals: updatedRecentDismissals
-        });
-        combineAndShowNotificationCards(updatedGroups, onCardShown);
-        recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+        combineAndShowNotificationCards(updatedGroups,

[vadimt, 2013/12/13 20:07:40]

nit: please start "updatedGroups," from new line

[robliao, 2013/12/13 20:38:17]

Done.

+            function() {
+              chrome.storage.local.set({
+                notificationGroups: updatedGroups,
+                recentDismissals: updatedRecentDismissals
+              });
+              recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+            },
+            onCardShown);
       });
 }
 
@@ -905,42 +917,47 @@ function onNotificationClosed(chromeNotificationId, byUser) {
     dismissalAttempts.start();
 
     instrumented.storage.local.get(
-        ['pendingDismissals', 'notificationsData'], function(items) {
-      items = items || {};
-      /** @type {Array.<PendingDismissal>} */
-      items.pendingDismissals = items.pendingDismissals || [];
-      /** @type {Object.<string, NotificationDataEntry>} */
-      items.notificationsData = items.notificationsData || {};
-
-      /** @type {NotificationDataEntry} */
-      var notificationData = items.notificationsData[chromeNotificationId] || {
-        timestamp: Date.now(),
-        combinedCard: []
-      };
-
-      var dismissalResult =
-          cardSet.onDismissal(chromeNotificationId, notificationData);
-
-      for (var i = 0; i < dismissalResult.dismissals.length; i++) {
-        /** @type {PendingDismissal} */
-        var dismissal = {
-          chromeNotificationId: chromeNotificationId,
-          time: Date.now(),
-          dismissalData: dismissalResult.dismissals[i]
-        };
-        items.pendingDismissals.push(dismissal);
-      }
+        ['pendingDismissals', 'notificationsData', 'notificationGroups'],
+        function(items) {
+          items = items || {};
+          /** @type {Array.<PendingDismissal>} */
+          items.pendingDismissals = items.pendingDismissals || [];
+          /** @type {Object.<string, NotificationDataEntry>} */
+          items.notificationsData = items.notificationsData || {};
+          /** @type {Object.<string, StoredNotificationGroup>} */
+          items.notificationGroups = items.notificationGroups || {};
 
-      items.notificationsData[chromeNotificationId] =
-          dismissalResult.notificationData;
+          /** @type {NotificationDataEntry} */
+          var notificationData =
+              items.notificationsData[chromeNotificationId] ||
+              {
+                timestamp: Date.now(),
+                combinedCard: []
+              };
+
+          var dismissalResult =
+              cardSet.onDismissal(
+                  chromeNotificationId,
+                  notificationData,
+                  items.notificationGroups);
+
+          for (var i = 0; i < dismissalResult.dismissals.length; i++) {
+            /** @type {PendingDismissal} */
+            var dismissal = {
+              chromeNotificationId: chromeNotificationId,
+              time: Date.now(),
+              dismissalData: dismissalResult.dismissals[i]
+            };
+            items.pendingDismissals.push(dismissal);
+          }
 
-      chrome.storage.local.set({
-        pendingDismissals: items.pendingDismissals,
-        notificationsData: items.notificationsData
-      });
+          items.notificationsData[chromeNotificationId] =
+              dismissalResult.notificationData;
 
-      processPendingDismissals(function(success) {});
-    });
+          chrome.storage.local.set(items);
+
+          processPendingDismissals(function(success) {});
+        });
   });
 }
 
@@ -1125,7 +1142,7 @@ instrumented.runtime.onStartup.addListener(function() {
       /** @type {Object.<string, StoredNotificationGroup>} */
       items.notificationGroups = items.notificationGroups || {};
 
-      combineAndShowNotificationCards(items.notificationGroups);
+      combineAndShowNotificationCards(items.notificationGroups, function() {});

[vadimt, 2013/12/13 20:07:40]

We need to store notificationGroups here too, in case when all notifications in
the combined card have expired, and get deleted from groups.

[robliao, 2013/12/13 20:38:17]

Ah. I for some reason thought we didn't need this. That's easy enough.
On 2013/12/13 20:07:40, vadimt wrote:

     });
   });