Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22912021:
Handling errors from chrome APIs (Closed)
|
Created:
7 years, 4 months ago by vadimt Modified:
7 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandling errors from chrome APIs.
Some Chrome API calls can fail and return 'undefined' result. Making our code not send crashes and behave reasonably in such cases.
Also, sending crashes if server sends malformed cards.
+ some smaller cleanups.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219157
Patch Set 1 #
Total comments: 24
Patch Set 2 : Comments #
Messages
Total messages: 14 (0 generated)
lgtm https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:588: var notificationData = items.notificationsData[notificationId]; no action required but consider var nd = items && items.notificationsData && items.notificationsData[nId];
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:260: items.notificationsData = items.notificationsData || {}; Is a check here necessary too? https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:521: items.pendingDismissals = items.pendingDismissals || []; Should we check items here? https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:662: items.pendingDismissals = items.pendingDismissals || []; Check? https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:883: var geolocationEnabled = !!prefValue.value; Is any Chrome API subject to returning undefined? We'll want to check here too. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/cards.js (left): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/cards.js:43: try { We don't need to catch exceptions anymore?
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:588: var notificationData = items.notificationsData[notificationId]; On 2013/08/21 23:12:32, Travis Skare wrote: > no action required but consider > var nd = items && items.notificationsData && items.notificationsData[nId]; +1, will return below anyways. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:602: if (!url) This is verified already in the selector. if statement not needed. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:890: !items || !!items.userRespondedToToast; Could this get the user in a weird state if there was a sporadic failure? https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:978: verify(actionUrls.buttonUrls[buttonIndex], pull actionUrls.buttonUrls[buttonIndex] out into url var since you use this same value twice. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:508: items = {}; why not items.currentDelayStorageKey like everything else?
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:508: items = {}; On 2013/08/22 00:46:14, rgustafson wrote: > why not items.currentDelayStorageKey like everything else? I just answered that for myself right after asking it. :)
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:260: items.notificationsData = items.notificationsData || {}; On 2013/08/22 00:01:56, Robert Liao wrote: > Is a check here necessary too? Yes, but this would cause conflicts with other CLs. This will be taken care of later. Good note though. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:521: items.pendingDismissals = items.pendingDismissals || []; On 2013/08/22 00:01:56, Robert Liao wrote: > Should we check items here? Conflicts, see above. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:588: var notificationData = items.notificationsData[notificationId]; On 2013/08/22 00:46:14, rgustafson wrote: > On 2013/08/21 23:12:32, Travis Skare wrote: > > no action required but consider > > var nd = items && items.notificationsData && items.notificationsData[nId]; > > +1, will return below anyways. Done. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:602: if (!url) On 2013/08/22 00:46:14, rgustafson wrote: > This is verified already in the selector. if statement not needed. 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. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:662: items.pendingDismissals = items.pendingDismissals || []; On 2013/08/22 00:01:56, Robert Liao wrote: > Check? Conflicts. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:883: var geolocationEnabled = !!prefValue.value; On 2013/08/22 00:01:56, Robert Liao wrote: > Is any Chrome API subject to returning undefined? We'll want to check here too. No. Only those that are explicitly implemented to be able to fail (something like SendResponse(false)). Can this one fail? https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:890: !items || !!items.userRespondedToToast; On 2013/08/22 00:46:14, rgustafson wrote: > Could this get the user in a weird state if there was a sporadic failure? 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. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:978: verify(actionUrls.buttonUrls[buttonIndex], On 2013/08/22 00:46:14, rgustafson wrote: > pull actionUrls.buttonUrls[buttonIndex] out into url var since you use this same > value twice. Done. https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/cards.js (left): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/cards.js:43: try { On 2013/08/22 00:01:56, Robert Liao wrote: > We don't need to catch exceptions anymore? It's better if our instrumentation catches this and send crash report. This will throw an exception if a server sends a malformed card.
lgtm https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:883: var geolocationEnabled = !!prefValue.value; As written, it cannot fail. On 2013/08/22 01:24:28, vadimt wrote: > On 2013/08/22 00:01:56, Robert Liao wrote: > > Is any Chrome API subject to returning undefined? We'll want to check here > too. > > No. Only those that are explicitly implemented to be able to fail (something > like SendResponse(false)). Can this one fail?
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:890: !items || !!items.userRespondedToToast; On 2013/08/22 01:24:28, vadimt wrote: > On 2013/08/22 00:46:14, rgustafson wrote: > > Could this get the user in a weird state if there was a sporadic failure? > > 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. 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.
https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/22912021/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:890: !items || !!items.userRespondedToToast; On 2013/08/22 19:07:32, rgustafson wrote: > On 2013/08/22 01:24:28, vadimt wrote: > > On 2013/08/22 00:46:14, rgustafson wrote: > > > Could this get the user in a weird state if there was a sporadic failure? > > > > 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. > > 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. 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)
lgtm
arv@, please provide OWNER's approval
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/22912021/10001
Message was sent while issue was closed.
Change committed as 219157 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
