Requesting cards on push messages

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 98651bab5f283aa9d0befe0329f20336c1cd22be..aa5bd311d94ca283c049743196f1895231164bf8 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -81,12 +81,18 @@ var DISMISS_CARD_TASK_NAME = 'dismiss-card';
 var RETRY_DISMISS_TASK_NAME = 'retry-dismiss';
 var STATE_CHANGED_TASK_NAME = 'state-changed';
 var SHOW_ON_START_TASK_NAME = 'show-cards-on-start';
+var ON_PUSH_MESSAGE_START_TASK_NAME = 'on-push-message';
 
 var LOCATION_WATCH_NAME = 'location-watch';
 
 var WELCOME_TOAST_NOTIFICATION_ID = 'enable-now-toast';
 
 /**
+ * Chrome push messaging subchannel for messages causing an immediate poll.
+ */
+var SUBCHANNEL_ID_POLL_NOW = 0;
+
+/**
  * The indices of the buttons that are displayed on the welcome toast.
  * @enum {number}
  */
@@ -168,6 +174,7 @@ wrapper.instrumentChromeApiFunction(
     'preferencesPrivate.googleGeolocationAccessEnabled.onChange.addListener',
     0);
 wrapper.instrumentChromeApiFunction('permissions.contains', 1);
+wrapper.instrumentChromeApiFunction('pushMessaging.onMessage.addListener', 0);
 wrapper.instrumentChromeApiFunction('runtime.onInstalled.addListener', 0);
 wrapper.instrumentChromeApiFunction('runtime.onStartup.addListener', 0);
 wrapper.instrumentChromeApiFunction('tabs.create', 1);
@@ -522,18 +529,50 @@ function parseAndShowNotificationCards(response) {
 }
 
 /**
- * Requests notification cards from the server.
- * @param {Location} position Location of this computer.
+ * Requests notification cards from the server for specified groups.
+ * @param {Array.<string>} groupNames Names of groups that need to be refreshed.
  */
-function requestNotificationCards(position) {
-  console.log('requestNotificationCards ' + JSON.stringify(position) +
-      ' from ' + NOTIFICATION_CARDS_URL);
+function requestNotificationGroups(groupNames) {
+  console.log('requestNotificationGroups from ' + NOTIFICATION_CARDS_URL +
+      ', groupNames=' + JSON.stringify(groupNames));
 
   if (!NOTIFICATION_CARDS_URL)
     return;
 
   recordEvent(GoogleNowEvent.REQUEST_FOR_CARDS_TOTAL);
 
+  var requestParameters = '?timeZoneOffsetMs=' +
+      (-new Date().getTimezoneOffset() * MS_IN_MINUTE);
+
+  groupNames.forEach(function(groupName) {
+    requestParameters += ('&requestTypes=' + groupName);
+  });
+
+  console.log('requestNotificationGroups: request=' + requestParameters);
+
+  var request = buildServerRequest('GET', 'notifications' + requestParameters);
+
+  request.onloadend = function(event) {
+    console.log('requestNotificationGroups-onloadend ' + request.status);
+    if (request.status == HTTP_OK) {
+      recordEvent(GoogleNowEvent.REQUEST_FOR_CARDS_SUCCESS);
+      parseAndShowNotificationCards(request.response);
+    }
+  };
+
+  setAuthorization(request, function(success) {

[rgustafson, 2013/10/15 20:56:54]

But the signed-in condition isn't missing, it's right here. What am I missing?
Making another auth request doesn't change the behavior.

[vadimt, 2013/10/15 21:14:09]

I'm not saying I'm changing the behavior. The check there is for readability and
solidity of the code.

[rgustafson, 2013/10/16 17:14:32]

There are two problems here:
(1) Duplicate code between polling entry points if your goal is to also add NC
checkbox code to there and another place for normally starting polling. See
below comment.
(2) Adding code that doesn't change any behavior -- you shouldn't be adding that
code until it does change something. The behavior DOES change if you use it to
enable bg mode, etc as below.

[vadimt, 2013/10/16 20:10:20]

I've changed implementation to use updateCardsAttempts.isRunning(). Thanks for
the comments. This matches updateNotificationsCards().
updateCardsAttempts.isRunning() is our standard indication that we are in a
polling state.
Note that both are used in almost identical situations: (1) alarm for poll gets
fired; (2) signal for poll arrives from the server.
Checking for updateCardsAttempts.isRunning() may be currently doesn't change the
behavior, but will, once we start taking into account the NC flag.

+    if (success)
+      request.send();
+  });
+}
+
+/**
+ * Requests notification cards from the server.
+ * @param {Location} position Location of this computer.
+ */
+function requestNotificationCards(position) {
+  console.log('requestNotificationCards ' + JSON.stringify(position));
+
   instrumented.storage.local.get('notificationGroups', function(items) {
     console.log('requestNotificationCards-storage-get ' +
                 JSON.stringify(items));
@@ -542,33 +581,19 @@ function requestNotificationCards(position) {
     var requestParameters = '?timeZoneOffsetMs=' +

[rgustafson, 2013/10/15 20:56:54]

Remove this. It was moved.

[vadimt, 2013/10/15 21:14:09]

Done.

         (-new Date().getTimezoneOffset() * MS_IN_MINUTE);
 
+    var groupsToRequest = [];
+
     if (items.notificationGroups) {
       var now = Date.now();
 
       for (var groupName in items.notificationGroups) {
         var group = items.notificationGroups[groupName];
         if (group.nextPollTime <= now)
-          requestParameters += ('&requestTypes=' + groupName);
+          groupsToRequest.push(groupName);
       }
     }
 
-    console.log('requestNotificationCards: request=' + requestParameters);
-
-    var request = buildServerRequest('GET',
-                                     'notifications' + requestParameters);
-
-    request.onloadend = function(event) {
-      console.log('requestNotificationCards-onloadend ' + request.status);
-      if (request.status == HTTP_OK) {
-        recordEvent(GoogleNowEvent.REQUEST_FOR_CARDS_SUCCESS);
-        parseAndShowNotificationCards(request.response);
-      }
-    };
-
-    setAuthorization(request, function(success) {
-      if (success)
-        request.send();
-    });
+    requestNotificationGroups(groupsToRequest);
   });
 }
 
@@ -1152,3 +1177,21 @@ instrumented.omnibox.onInputEntered.addListener(function(text) {
   localStorage['server_url'] = NOTIFICATION_CARDS_URL = text;
   initialize();
 });
+
+instrumented.pushMessaging.onMessage.addListener(function(message) {
+  console.log('pushMessaging.onMessage ' + JSON.stringify(message));
+  if (message.subchannelId == SUBCHANNEL_ID_POLL_NOW && message.payload) {
+    tasks.add(ON_PUSH_MESSAGE_START_TASK_NAME, function() {
+      instrumented.storage.local.get('lastPollNowPayload', function(items) {
+        if (items && items.lastPollNowPayload != message.payload) {

[rgustafson, 2013/10/15 17:57:51]

What are you expecting in the payload and why does it need to be different?

[vadimt, 2013/10/15 19:41:43]

When the extension start for the first time, you get a message with an empty
payload. If you sign out and sign in, you get a message with the latest payload.
To filter out this noise and to identify real messages, the server should send
timestamps as a payload, and the client is supposed to check that the payload is
not empty, and is different from last received one.

[rgustafson, 2013/10/15 20:56:54]

Some comments of that explanation, at least what you're expecting as the value
of the payload, would go a very long way to let readers of this code understand.

[vadimt, 2013/10/15 21:14:09]

Done.

+          chrome.storage.local.set({lastPollNowPayload: message.payload});
+
+          authenticationManager.isSignedIn(function(token) {

[rgustafson, 2013/10/15 17:57:51]

You can receive push messages without being signed in? I thought that was
already a prereq. It also already calls setAuthorization soon after this. Is
this necessary?

[vadimt, 2013/10/15 19:41:43]

It's more for readability and solidity of the code. At the end, we'll poll if NC
checkbox is on AND we are signed in. It will be strange if one of these
conditions is missing.

+            if (token)
+              requestNotificationGroups([]);

[rgustafson, 2013/10/16 17:14:32]

Why isn't this (and the surrounding sign in check) onStateChange instead? In
this case, it should be checking that all of the prereqs for polling are
fulfilled and turning on the background permission to get into the same state as
starting normal polling, right?

[vadimt, 2013/10/16 20:10:20]

The fact that we received a push notification doesn't indicate changing of the
state. This is similar to the fact that firing a poll alarm doesn't indicate a
state change.
The state remains the same. OnStateChange will be called once
'googleNowDisabled' in server responses changes.

+          });
+        }
+      });
+    });
+  }
+});