Handling errors from chrome APIs

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 787c3329dc3cd645d454dbfd2a013156656c31f4..d32d5da3588da801b63ec77b937adc6dda6d7062 100644
--- a/chrome/browser/resources/google_now/background.js
+++ b/chrome/browser/resources/google_now/background.js
@@ -23,8 +23,7 @@
 // TODO(vadimt): Honor the flag the enables Google Now integration.
 // TODO(vadimt): Figure out the final values of the constants.
 // TODO(vadimt): Remove 'console' calls.
-// TODO(vadimt): Consider sending JS stacks for chrome.* API errors and
-// malformed server responses.
+// TODO(vadimt): Consider sending JS stacks for malformed server responses.
 
 /**
  * Standard response code for successful HTTP requests. This is the only success
@@ -346,6 +345,7 @@ function removeAllCards() {
   // notifications center and bug 260376 is fixed, the below clearing
   // code is no longer necessary.
   instrumented.notifications.getAll(function(notifications) {
+    notifications = notifications || {};
     for (var notificationId in notifications) {
       chrome.notifications.clear(notificationId, function() {});
     }
@@ -405,7 +405,7 @@ function requestLocation() {
   console.log('requestLocation');
   recordEvent(GoogleNowEvent.LOCATION_REQUEST);
   // TODO(vadimt): Figure out location request options.
-  chrome.metricsPrivate.getVariationParams('GoogleNow', function(params) {
+  instrumented.metricsPrivate.getVariationParams('GoogleNow', function(params) {
     var minDistanceInMeters =
         parseInt(params && params.minDistanceInMeters, 10) ||
         100;
@@ -582,7 +582,8 @@ function retryPendingDismissals() {
  */
 function onNotificationClicked(notificationId, selector) {
   instrumented.storage.local.get('notificationsData', function(items) {
-    items.notificationsData = items.notificationsData || {};
+    if (!items || !items.notificationsData)
+      return;
 
     var notificationData = items.notificationsData[notificationId];

[skare_, 2013/08/21 23:12:32]

no action required but consider
var nd = items && items.notificationsData && items.notificationsData[nId];

[rgustafson, 2013/08/22 00:46:14]

+1, will return below anyways.

[vadimt, 2013/08/22 01:24:28]

Done.

 
@@ -598,8 +599,7 @@ function onNotificationClicked(notificationId, selector) {
     }
 
     var url = selector(actionUrls);
-
-    if (typeof url != 'string')
+    if (!url)

[rgustafson, 2013/08/22 00:46:14]

This is verified already in the selector. if statement not needed.

[vadimt, 2013/08/22 01:24:28]

Not in selector in 'instrumented.notifications.onClicked.addListener'. We are
tolerant to clicking at a message body with no URL. (We are not tolerant for
buttons).

Deep comment though.

       return;
 
     instrumented.tabs.create({url: url}, function(tab) {
@@ -886,7 +886,8 @@ function onStateChange() {
                   instrumented.storage.local.get(
                       'userRespondedToToast',
                       function(items) {
-                        var userRespondedToToast = !!items.userRespondedToToast;
+                        var userRespondedToToast =
+                            !items || !!items.userRespondedToToast;

[rgustafson, 2013/08/22 00:46:14]

Could this get the user in a weird state if there was a sporadic failure? 

[vadimt, 2013/08/22 01:24:28]

Deep comment again. If we are polling, the final state machine will keep polling
and will try to set the value to true.
If we were showing toast, we'll hide the toast and turn off own notifications.
The user will be able to enable notifications (and the rest) manually.

[rgustafson, 2013/08/22 19:07:32]

We'll hide the toast and disable ourselves for just that session or permanently?
If this is something that changes state permanently, it definitely seems like a
storage failure shouldnt be taken into account with state change. Potentially
move this to a separate bug.

[vadimt, 2013/08/22 19:34:55]

https://code.google.com/p/chromium/issues/detail?id=277767
Note, though, that we do quite a safe and conservative thing: disabling the
toast after a disaster. (storage.get errors happen only after something really
bad happens, like DB corruption)

                         updateRunningState(
                             signedIn,
                             geolocationEnabled,
@@ -974,9 +975,8 @@ instrumented.notifications.onButtonClicked.addListener(
         chrome.metricsPrivate.recordUserAction(
             'GoogleNow.ButtonClicked' + buttonIndex);
         onNotificationClicked(notificationId, function(actionUrls) {
-          if (!Array.isArray(actionUrls.buttonUrls))
-            return undefined;
-
+          verify(actionUrls.buttonUrls[buttonIndex],

[rgustafson, 2013/08/22 00:46:14]

pull actionUrls.buttonUrls[buttonIndex] out into url var since you use this same
value twice.

[vadimt, 2013/08/22 01:24:28]

Done.

+                 'onButtonClicked: no url for a button');
           return actionUrls.buttonUrls[buttonIndex];
         });
       }