Using notifications.getAll

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'; TODO(vadimt): Uncomment once crbug.com/237617 is fixed.
 
 /**
  * @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 46 matching lines @@
 /**
  * Time we keep dismissals after successful server dismiss requests.
  */
 var DISMISS_RETENTION_TIME_MS = 20 * 60 * 1000;  // 20 minutes
 
 /**
  * Names for tasks that can be created by the extension.
  */
 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';
 
 /**
  * Checks if a new task can't be scheduled when another task is already
  * scheduled.
  * @param {string} newTaskName Name of the new task.
  * @param {string} scheduledTaskName Name of the scheduled task.
  * @return {boolean} Whether the new task conflicts with the existing task.
@@ skipping 17 matching lines @@
 
   return false;
 }
 
 var tasks = buildTaskManager(areTasksConflicting);
 
 // Add error processing to API calls.
 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);
 tasks.instrumentApiFunction(chrome.notifications.onClosed, 'addListener', 0);
 tasks.instrumentApiFunction(chrome.runtime.onInstalled, 'addListener', 0);
 tasks.instrumentApiFunction(chrome.runtime.onStartup, 'addListener', 0);
 tasks.instrumentApiFunction(chrome.tabs, 'create', 1);
 tasks.instrumentApiFunction(storage, 'get', 1);
 
 var updateCardsAttempts = buildAttemptManager(
@@ skipping 38 matching lines @@
 
   chrome.metricsPrivate.recordValue(metricDescription, event);
 }
 
 /**
  * Shows a notification and remembers information associated with it.
  * @param {Object} card Google Now card represented as a set of parameters for
  *     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.
       chrome.notifications.create(
           card.notificationId,
           card.notification,
           function(notificationId) {
             if (!notificationId || chrome.runtime.lastError) {
               var errorMessage =
                   chrome.runtime.lastError && chrome.runtime.lastError.message;
@@ skipping 51 matching lines @@
     callback();
     return;
   }
 
   if (typeof parsedResponse.expiration_timestamp_seconds != 'number') {
     callback();
     return;
   }
 
   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);
+      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);
+      // 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();
   });
 }
 
 /**
  * Requests notification cards from the server.
  * @param {Location} position Location of this computer.
  * @param {function()} callback Completion callback.
  */
 function requestNotificationCards(position, callback) {
   console.log('requestNotificationCards ' + JSON.stringify(position) +
@@ skipping 155 matching lines @@
   });
 }
 
 /**
  * Opens URL corresponding to the clicked part of the notification.
  * @param {string} notificationId Unique identifier of the notification.
  * @param {function(Object): string} selector Function that extracts the url for
  *     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 || {};
+  storage.get('notificationsData', function(items) {
+    items.notificationsData = items.notificationsData || {};
 
-      var actionUrls = items.activeNotifications[notificationId].actionUrls;
-      if (typeof actionUrls != 'object') {
-        callback();
-        return;
-      }
+    var notificationData = items.notificationsData[notificationId];
 
-      var url = selector(actionUrls);
+    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;
+    }
 
-      if (typeof url != 'string') {
-        callback();
-        return;
-      }
+    var actionUrls = notificationData.actionUrls;
+    if (typeof actionUrls != 'object') {
+      return;
+    }
 
-      chrome.tabs.create({url: url}, function(tab) {
-        if (!tab)
-          chrome.windows.create({url: url});
-      });
-      callback();
+    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});
     });
   });
 }
 
 /**
  * Callback for chrome.notifications.onClosed event.
  * @param {string} notificationId Unique identifier of the notification.
  * @param {boolean} byUser Whether the notification was closed by the user.
  */
 function onNotificationClosed(notificationId, byUser) {
@@ skipping 27 matching lines @@
 }
 
 /**
  * Initializes the event page on install or on browser startup.
  */
 function initialize() {
   // Create an update timer for a case when for some reason location request
   // gets stuck.
   updateCardsAttempts.start(MAXIMUM_POLLING_PERIOD_SECONDS);
 
-  var initialStorage = {
-    activeNotifications: {}
-  };
-  storage.set(initialStorage);
-
   requestLocation();
 }
 
 chrome.runtime.onInstalled.addListener(function(details) {
   console.log('onInstalled ' + JSON.stringify(details));
   if (details.reason != 'chrome_update') {
     initialize();
   }
 });
 
@@ skipping 26 matching lines @@
 
 chrome.location.onLocationUpdate.addListener(function(position) {
   recordEvent(DiagnosticEvent.LOCATION_UPDATE);
   updateNotificationsCards(position);
 });
 
 chrome.omnibox.onInputEntered.addListener(function(text) {
   localStorage['server_url'] = NOTIFICATION_CARDS_URL = text;
   initialize();
 });