Handling errors from chrome APIs

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.
  * The service performs periodic updating of Google Now cards.
  * Each updating of the cards includes 4 steps:
  * 1. Obtaining the location of the machine;
  * 2. Processing requests for cards dismissals that are not yet sent to the
  *    server;
  * 3. Making a server request based on that location;
  * 4. Showing the received cards as notifications.
  */
 
 // TODO(vadimt): Use background permission to show notifications even when all
 // browser windows are closed.
 // TODO(vadimt): Decide what to do in incognito mode.
 // 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
  * code the server will send.
  */
 var HTTP_OK = 200;
 
 var HTTP_BAD_REQUEST = 400;
 var HTTP_UNAUTHORIZED = 401;
 var HTTP_FORBIDDEN = 403;
@@ skipping 213 matching lines @@
   if (typeof parsedResponse.next_poll_seconds != 'number') {
     callback();
     return;
   }
 
   tasks.debugSetStepName('parseAndShowNotificationCards-storage-get');
   instrumented.storage.local.get(['notificationsData', 'recentDismissals'],
       function(items) {
         console.log('parseAndShowNotificationCards-get ' +
             JSON.stringify(items));
         items.notificationsData = items.notificationsData || {};

[robliao, 2013/08/22 00:01:56]

Is a check here necessary too?

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

Yes, but this would cause conflicts with other CLs. This will be taken care of
later.
Good note though.

         items.recentDismissals = items.recentDismissals || {};
 
         tasks.debugSetStepName(
             'parseAndShowNotificationCards-notifications-getAll');
         instrumented.notifications.getAll(function(notifications) {
           console.log('parseAndShowNotificationCards-getAll ' +
               JSON.stringify(notifications));
           // TODO(vadimt): Figure out what to do when notifications are
           // disabled for our extension.
           notifications = notifications || {};
@@ skipping 67 matching lines @@
  * For example, this occurs when the geolocation preference is unchecked in the
  * content settings.
  */
 function removeAllCards() {
   console.log('removeAllCards');
 
   // TODO(robliao): Once Google Now clears its own checkbox in the
   // 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() {});
     }
     chrome.storage.local.set({notificationsData: {}});
   });
 }
 
 /**
  * Requests notification cards from the server.
  * @param {Location} position Location of this computer.
@@ skipping 39 matching lines @@
   });
 }
 
 /**
  * Starts getting location for a cards update.
  */
 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;
     var minTimeInMilliseconds =
         parseInt(params && params.minTimeInMilliseconds, 10) ||
         180000;  // 3 minutes.
 
     chrome.location.watchLocation(LOCATION_WATCH_NAME, {
       minDistanceInMeters: minDistanceInMeters,
       minTimeInMilliseconds: minTimeInMilliseconds
@@ skipping 92 matching lines @@
  * Tries to send dismiss requests for all pending dismissals.
  * @param {function(boolean)} callbackBoolean Completion callback with 'success'
  *     parameter. Success means that no pending dismissals are left.
  */
 function processPendingDismissals(callbackBoolean) {
   tasks.debugSetStepName('processPendingDismissals-storage-get');
   instrumented.storage.local.get(['pendingDismissals', 'recentDismissals'],
       function(items) {
         console.log('processPendingDismissals-storage-get ' +
                     JSON.stringify(items));
         items.pendingDismissals = items.pendingDismissals || [];

[robliao, 2013/08/22 00:01:56]

Should we check items here?

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

Conflicts, see above.

         items.recentDismissals = items.recentDismissals || {};
 
         var dismissalsChanged = false;
 
         function onFinish(success) {
           if (dismissalsChanged) {
             chrome.storage.local.set({
               pendingDismissals: items.pendingDismissals,
               recentDismissals: items.recentDismissals
             });
@@ skipping 43 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) {
   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.

 
     if (!notificationData) {
       // 'notificationsData' in storage may not match the actual list of
       // notifications.
       return;
     }
 
     var actionUrls = notificationData.actionUrls;
     if (typeof actionUrls != 'object') {
       return;
     }
 
     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) {
       if (tab)
         chrome.windows.update(tab.windowId, {focused: true});
       else
         chrome.windows.create({url: url, focused: true});
     });
   });
 }
@@ skipping 39 matching lines @@
   tasks.add(DISMISS_CARD_TASK_NAME, function(callback) {
     dismissalAttempts.start();
 
     // Deleting the notification in case it was re-added while this task was
     // scheduled, waiting for execution.
     cardSet.clear(notificationId);
 
     tasks.debugSetStepName('onNotificationClosed-storage-get');
     instrumented.storage.local.get(['pendingDismissals', 'notificationsData'],
         function(items) {
           items.pendingDismissals = items.pendingDismissals || [];

[robliao, 2013/08/22 00:01:56]

Check?

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

Conflicts.

           items.notificationsData = items.notificationsData || {};
 
           var notificationData = items.notificationsData[notificationId];
 
           var dismissal = {
             notificationId: notificationId,
             time: Date.now(),
             parameters: notificationData && notificationData.dismissalParameters
           };
           items.pendingDismissals.push(dismissal);
@@ skipping 200 matching lines @@
           'GoogleNow',
           function(response) {
             var enableBackground =
                 (!response || (response.enableBackground != 'false'));
             tasks.debugSetStepName(
                 'onStateChange-get-googleGeolocationAccessEnabledPref');
             instrumented.
                 preferencesPrivate.
                 googleGeolocationAccessEnabled.
                 get({}, function(prefValue) {
                   var geolocationEnabled = !!prefValue.value;

[robliao, 2013/08/22 00:01:56]

Is any Chrome API subject to returning undefined? We'll want to check here too.

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

No. Only those that are explicitly implemented to be able to fail (something
like SendResponse(false)). Can this one fail?

[robliao, 2013/08/22 18:01:13]

As written, it cannot fail.
On 2013/08/22 01:24:28, vadimt wrote:

                   tasks.debugSetStepName(
                     'onStateChange-get-userRespondedToToast');
                   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,
                             userRespondedToToast,
                             enableBackground,
                             callback);
                       });
                 });
           });
     });
@@ skipping 67 matching lines @@
     });
 
 instrumented.notifications.onButtonClicked.addListener(
     function(notificationId, buttonIndex) {
       if (notificationId == WELCOME_TOAST_NOTIFICATION_ID) {
         onToastNotificationClicked(buttonIndex);
       } else {
         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];
         });
       }
     });
 
 instrumented.notifications.onClosed.addListener(onNotificationClosed);
 
 instrumented.location.onLocationUpdate.addListener(function(position) {
   recordEvent(GoogleNowEvent.LOCATION_UPDATE);
   updateNotificationsCards(position);
 });
 
 instrumented.omnibox.onInputEntered.addListener(function(text) {
   localStorage['server_url'] = NOTIFICATION_CARDS_URL = text;
   initialize();
 });