Using notifications.getAll

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 ab3e7b1518feee2253e7bfc86b6088539cd0a443..83e54e51fdc9ef904d890227c35fd1d26bbd929b 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -64,7 +64,6 @@ var DISMISS_RETENTION_TIME_MS = 20 * 60 * 1000;  // 20 minutes
  */
 var UPDATE_CARDS_TASK_NAME = 'update-cards';
 var DISMISS_CARD_TASK_NAME = 'dismiss-card';
-var CARD_CLICKED_TASK_NAME = 'card-clicked';
 var RETRY_DISMISS_TASK_NAME = 'retry-dismiss';
 
 var LOCATION_WATCH_NAME = 'location-watch';
@@ -102,6 +101,7 @@ var tasks = buildTaskManager(areTasksConflicting);
 tasks.instrumentApiFunction(chrome.location.onLocationUpdate, 'addListener', 0);
 tasks.instrumentApiFunction(chrome.notifications, 'create', 2);
 tasks.instrumentApiFunction(chrome.notifications, 'update', 2);
+tasks.instrumentApiFunction(chrome.notifications, 'getAll', 0);
 tasks.instrumentApiFunction(
     chrome.notifications.onButtonClicked, 'addListener', 0);
 tasks.instrumentApiFunction(chrome.notifications.onClicked, 'addListener', 0);
@@ -160,20 +160,20 @@ function recordEvent(event) {
  *     showing a Chrome notification.
  * @param {Object} notificationsData Map from notification id to the data
  *     associated with a notification.
- * @param {number} previousVersion The version of the shown card with this id,
- *     if it exists, undefined otherwise.
+ * @param {number=} opt_previousVersion The version of the shown card with this
+ *     id, if it exists, undefined otherwise.
  */
-function showNotification(card, notificationsData, previousVersion) {
+function showNotification(card, notificationsData, opt_previousVersion) {
   console.log('showNotification ' + JSON.stringify(card) + ' ' +
-             previousVersion);
+             opt_previousVersion);
 
   if (typeof card.version != 'number') {
     console.error('card.version is not a number');
     // Fix card version.
-    card.version = previousVersion !== undefined ? previousVersion : 0;
+    card.version = opt_previousVersion !== undefined ? opt_previousVersion : 0;
   }
 
-  if (previousVersion !== card.version) {
+  if (opt_previousVersion !== card.version) {
     try {
       // Delete a notification with the specified id if it already exists, and
       // then create a notification.
@@ -245,66 +245,75 @@ function parseAndShowNotificationCards(response, callback) {
   }
 
   tasks.debugSetStepName('parseAndShowNotificationCards-storage-get');
-  storage.get(['activeNotifications', 'recentDismissals'], function(items) {
+  storage.get(['notificationsData', 'recentDismissals'], function(items) {
     console.log('parseAndShowNotificationCards-get ' + JSON.stringify(items));
-    items.activeNotifications = items.activeNotifications || {};
+    items.notificationsData = items.notificationsData || {};

[rgustafson, 2013/05/31 19:07:25]

btw, did you see the suggestion on an already submitted cl to do:
storage.get({
  'activeNotifications': {},
  'recentDismissals': {}
}, function(items) {
  ...
});
for the defaults?

[vadimt, 2013/05/31 19:24:05]

I didn't want to complicate this CR with unrelated changes (Note that I'll also
show it to dewittj@).

I'm saving this for the CL where I'll process storage.get errors; and that one
will be also shown to the author of this proposal.

[rgustafson, 2013/05/31 20:48:12]

Okay, making sure you didn't miss it cause it's a good move.

     items.recentDismissals = items.recentDismissals || {};
 
-    // Build a set of non-expired recent dismissals. It will be used for
-    // client-side filtering of cards.
-    var updatedRecentDismissals = {};
-    var currentTimeMs = Date.now();
-    for (var notificationId in items.recentDismissals) {
-      if (currentTimeMs - items.recentDismissals[notificationId] <
-          DISMISS_RETENTION_TIME_MS) {
-        updatedRecentDismissals[notificationId] =
-            items.recentDismissals[notificationId];
+    tasks.debugSetStepName(
+        'parseAndShowNotificationCards-notifications-getAll');
+    chrome.notifications.getAll(function(notifications) {
+      console.log('parseAndShowNotificationCards-getAll ' +
+          JSON.stringify(notifications));
+      // Build a set of non-expired recent dismissals. It will be used for
+      // client-side filtering of cards.
+      var updatedRecentDismissals = {};
+      var currentTimeMs = Date.now();
+      for (var notificationId in items.recentDismissals) {
+        if (currentTimeMs - items.recentDismissals[notificationId] <
+            DISMISS_RETENTION_TIME_MS) {
+          updatedRecentDismissals[notificationId] =
+              items.recentDismissals[notificationId];
+        }
       }
-    }
 
-    // Mark existing notifications that received an update in this server
-    // response.
-    for (var i = 0; i < cards.length; ++i) {
-      var notificationId = cards[i].notificationId;
-      if (!(notificationId in updatedRecentDismissals) &&
-          notificationId in items.activeNotifications) {
-        items.activeNotifications[notificationId].hasUpdate = true;
+      // Mark existing notifications that received an update in this server
+      // response.
+      var updatedNotifications = {};
+
+      for (var i = 0; i < cards.length; ++i) {
+        var notificationId = cards[i].notificationId;
+        if (!(notificationId in updatedRecentDismissals) &&
+            notificationId in notifications) {
+          updatedNotifications[notificationId] = true;
+        }
       }
-    }
 
-    // Delete notifications that didn't receive an update.
-    for (var notificationId in items.activeNotifications) {
-      console.log('parseAndShowNotificationCards-delete-check ' +
-                  notificationId);
-      if (!items.activeNotifications[notificationId].hasUpdate) {
-        console.log('parseAndShowNotificationCards-delete ' + notificationId);
-        chrome.notifications.clear(
-            notificationId,
-            function() {});
+      // Delete notifications that didn't receive an update.
+      for (var notificationId in notifications) {
+        console.log('parseAndShowNotificationCards-delete-check ' +
+                    notificationId);
+        if (!(notificationId in updatedNotifications)) {
+          console.log('parseAndShowNotificationCards-delete ' + notificationId);
+          chrome.notifications.clear(
+              notificationId,
+              function() {});
+        }
       }
-    }
 
-    recordEvent(DiagnosticEvent.CARDS_PARSE_SUCCESS);
-
-    // Create/update notifications and store their new properties.
-    var notificationsData = {};
-    for (var i = 0; i < cards.length; ++i) {
-      var card = cards[i];
-      if (!(card.notificationId in updatedRecentDismissals)) {
-        var activeNotification = items.activeNotifications[card.notificationId];
-        showNotification(card,
-                         notificationsData,
-                         activeNotification && activeNotification.version);
+      recordEvent(DiagnosticEvent.CARDS_PARSE_SUCCESS);
+
+      // Create/update notifications and store their new properties.
+      var newNotificationsData = {};
+      for (var i = 0; i < cards.length; ++i) {
+        var card = cards[i];
+        if (!(card.notificationId in updatedRecentDismissals)) {
+          var notificationData = items.notificationsData[card.notificationId];
+          var version = notifications[card.notificationId] &&

[rgustafson, 2013/05/31 01:05:36]

previousVersion to make this more clear

[vadimt, 2013/05/31 01:39:44]

Done.

+                        notificationData &&
+                        notificationData.version;

[skare_, 2013/05/30 23:24:33]

do you want this to be 0 or undefined otherwise? would "|| 0" help?

[vadimt, 2013/05/31 00:45:34]

I don't want it to be 0 if first 2 conditions are false. This could cause
showNotification to go to the "update" branch, attempting to update a
non-existing notification, which will fail.

+          showNotification(card, newNotificationsData, version);
+        }
       }
-    }
 
-    updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
+      updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
 
-    storage.set({
-      activeNotifications: notificationsData,
-      recentDismissals: updatedRecentDismissals
+      storage.set({
+        notificationsData: newNotificationsData,
+        recentDismissals: updatedRecentDismissals
+      });
+      callback();
     });
-    callback();
   });
 }
 
@@ -480,29 +489,31 @@ function retryPendingDismissals() {
  *     the clicked area from the button action URLs info.
  */
 function onNotificationClicked(notificationId, selector) {
-  tasks.add(CARD_CLICKED_TASK_NAME, function(callback) {
-    tasks.debugSetStepName('onNotificationClicked-get-activeNotifications');
-    storage.get('activeNotifications', function(items) {
-      items.activeNotifications = items.activeNotifications || {};
-
-      var actionUrls = items.activeNotifications[notificationId].actionUrls;
-      if (typeof actionUrls != 'object') {
-        callback();
-        return;
-      }
+  storage.get('notificationsData', function(items) {
+    items.notificationsData = items.notificationsData || {};
 
-      var url = selector(actionUrls);
+    var notificationData = items.notificationsData[notificationId];
 
-      if (typeof url != 'string') {
-        callback();
-        return;
-      }
+    if (!notificationData) {

[rgustafson, 2013/05/31 01:05:36]

Even though this wasn't really added in this CL, clicking a notification, having
nothing happen, and then having it disappear (since deletion is only case this
can happen with) would piss me off as a user. Is there really no way to minimize
this?

This is a flat out bug imo and should be tracked as such, even if notification
data is the only way to get it. Just a matter of time before it gets reported by
someone else.

[vadimt, 2013/05/31 01:39:44]

We could imagine retaining deleted notifications' data for some period of time,
but this looks like on overkill. Probability of getting into this situation is
very small, namely: the callback for chrome.notifications.getAll needs to
squeeze between user's click and the corresponding callback. My estimation is
that the probability is less than 0.001%.
Retaining deleted notifications will probably hurt the quality of the extension
and the test matrix more than the harm from extremely rate no-op clicks, in my
opinion.

[rgustafson, 2013/05/31 19:07:25]

I have to agree that adding the complexity for retaining is not worth it. Guess
it's just something we'll keep in mind then.

[vadimt, 2013/05/31 19:24:05]

Thanks for the note anyways!

+      // 'notificationsData' in storage may not match the actual list of
+      // notifications.
+      return;
+    }
 
-      chrome.tabs.create({url: url}, function(tab) {
-        if (!tab)
-          chrome.windows.create({url: url});
-      });
-      callback();
+    var actionUrls = notificationData.actionUrls;
+    if (typeof actionUrls != 'object') {
+      return;
+    }
+
+    var url = selector(actionUrls);
+
+    if (typeof url != 'string') {
+      return;

[skare_, 2013/05/30 23:24:33]

(from prior CL) -- this seems to be guaranteed to be a string in the
chrome.notifications api docs. Have you seen otherwise?

[vadimt, 2013/05/31 00:45:34]

This is the data that we've received from the server, not the data we got from
chrome.notifications API. We verify all the data we receive from the server.

+    }
+
+    chrome.tabs.create({url: url}, function(tab) {
+      if (!tab)
+        chrome.windows.create({url: url});
     });
   });
 }
@@ -550,11 +561,6 @@ function initialize() {
   // gets stuck.
   updateCardsAttempts.start(MAXIMUM_POLLING_PERIOD_SECONDS);
 
-  var initialStorage = {
-    activeNotifications: {}
-  };
-  storage.set(initialStorage);
-
   requestLocation();
 }