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..1345c3657570be4cf15b7b44a022fbd5c341bc18 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -293,12 +293,14 @@ 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(ReceivedNotification)=} onCardShown Optional parameter
  *     called when each card is shown.
  */
-function showNotificationCards(cards, onCardShown) {
+function showNotificationCards(notificationGroups, cards, onCardShown) {
   console.log('showNotificationCards ' + JSON.stringify(cards));
 
   instrumented.notifications.getAll(function(notifications) {
@@ -320,6 +322,7 @@ function showNotificationCards(cards, onCardShown) {
       notificationsData[chromeNotificationId] = cardSet.update(
           chromeNotificationId,
           cards[chromeNotificationId],
+          notificationGroups,
           onCardShown);
     }
     chrome.storage.local.set({notificationsData: notificationsData});

[vadimt, 2013/12/13 18:20:54]

Please set notificationGroups here. getAll is an async call, and setting it from
parseAndShowNotificationCards will set groups not yet updated by
cardSet.update() calls.

[robliao, 2013/12/13 19:31:26]

Took a different approach here and delegated that responsibility to the caller.
On 2013/12/13 18:20:54, vadimt wrote:

@@ -429,7 +432,7 @@ function combineAndShowNotificationCards(notificationGroups, onCardShown) {
   for (var groupName in notificationGroups)
     combineGroup(combinedCards, notificationGroups[groupName]);
 
-  showNotificationCards(combinedCards, onCardShown);
+  showNotificationCards(notificationGroups, combinedCards, onCardShown);
 }
 
 /**
@@ -531,11 +534,11 @@ function parseAndShowNotificationCards(response, onCardShown) {
         }
 
         scheduleNextPoll(updatedGroups, !parsedResponse.googleNowDisabled);
+        combineAndShowNotificationCards(updatedGroups, onCardShown);
         chrome.storage.local.set({
           notificationGroups: updatedGroups,
           recentDismissals: updatedRecentDismissals
         });
-        combineAndShowNotificationCards(updatedGroups, onCardShown);
         recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);

[rgustafson, 2013/12/12 22:52:27]

tiny comment: This should probably be before the storage.set.

[robliao, 2013/12/12 23:01:21]

Indeed. I was going to save this for the unchained refactor.
On 2013/12/12 22:52:27, rgustafson wrote:

[robliao, 2013/12/13 19:31:26]

Well, now it does.
On 2013/12/12 23:01:21, robliao wrote:

       });
 }