Chrome Now notificationGroups Storage Race Condition Fix

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.
@@ skipping 275 matching lines @@
         originalOnLoadEnd(event);
       }
     });
 
     callbackBoolean(true);
   });
 }
 
 /**
  * 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) {
     console.log('showNotificationCards-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] || [];
     }
 
     /** @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});

[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:

   });
 }
 
 /**
  * Removes all cards and card state on Google Now close down.
  * For example, this occurs when the geolocation preference is unchecked in the
  * content settings.
  */
 function removeAllCards() {
   console.log('removeAllCards');
@@ skipping 86 matching lines @@
  */
 function combineAndShowNotificationCards(notificationGroups, onCardShown) {
   console.log('combineAndShowNotificationCards ' +
       JSON.stringify(notificationGroups));
   /** @type {Object.<ChromeNotificationId, CombinedCard>} */
   var combinedCards = {};
 
   for (var groupName in notificationGroups)
     combineGroup(combinedCards, notificationGroups[groupName]);
 
-  showNotificationCards(combinedCards, onCardShown);
+  showNotificationCards(notificationGroups, combinedCards, onCardShown);
 }
 
 /**
  * Parses JSON response from the notification server, shows notifications and
  * schedules next update.
  * @param {string} response Server response.
  * @param {function(ReceivedNotification)=} onCardShown Optional parameter
  *     called when each card is shown.
  */
 function parseAndShowNotificationCards(response, onCardShown) {
@@ skipping 81 matching lines @@
           // cards updates.
           if (receivedGroup.nextPollSeconds !== undefined) {
             storedGroup.nextPollTime =
                 now + receivedGroup.nextPollSeconds * MS_IN_SECOND;
           }
 
           updatedGroups[groupName] = storedGroup;
         }
 
         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:

       });
 }
 
 /**
  * Update Location Cards Shown Count.
  * @param {ReceivedNotification} receivedNotification Notification as it was
  *     received from the server.
  */
 function countLocationCard(receivedNotification) {
   if (receivedNotification.locationBased) {
@@ skipping 666 matching lines @@
             lastPollNowPayloads: items.lastPollNowPayloads,
             notificationGroups: items.notificationGroups
           });
 
           updateNotificationsCards();
         }
       });
     });
   }
 });