Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15868004:
Using notifications.getAll (Closed)
|
Created:
7 years, 6 months ago by vadimt Modified:
7 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, stromme, govind1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUsing notifications.getAll and performing 3-way merge between active notifications, notifications data in storage and new set of cards from the server.
BUG=164227
TEST=Nothing new.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203858
Patch Set 1 #
Total comments: 13
Patch Set 2 : rgustafson@'s notes #
Total comments: 4
Patch Set 3 : estade@'s comments #Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:304: notificationData.version; do you want this to be 0 or undefined otherwise? would "|| 0" help? https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:511: return; (from prior CL) -- this seems to be guaranteed to be a string in the chrome.notifications api docs. Have you seen otherwise?
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:304: notificationData.version; On 2013/05/30 23:24:33, Travis Skare wrote: > do you want this to be 0 or undefined otherwise? would "|| 0" help? 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. https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:511: return; On 2013/05/30 23:24:33, Travis Skare wrote: > (from prior CL) -- this seems to be guaranteed to be a string in the > chrome.notifications api docs. Have you seen otherwise? 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.
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:302: var version = notifications[card.notificationId] && previousVersion to make this more clear https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (!notificationData) { 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.
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:302: var version = notifications[card.notificationId] && On 2013/05/31 01:05:36, rgustafson wrote: > previousVersion to make this more clear Done. https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (!notificationData) { On 2013/05/31 01:05:36, rgustafson wrote: > 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. 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.
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:250: items.notificationsData = items.notificationsData || {}; btw, did you see the suggestion on an already submitted cl to do: storage.get({ 'activeNotifications': {}, 'recentDismissals': {} }, function(items) { ... }); for the defaults? https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (!notificationData) { On 2013/05/31 01:39:44, vadimt wrote: > On 2013/05/31 01:05:36, rgustafson wrote: > > 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. > > 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. 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. https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:303: notificationData && For the case of a notification in cards and in notifications but not in notificationsData: This is already a kind of broken situation. Right now, previousVersion = undefined and create will happen. The version of the previous card is lost, so the best we can do is guess. For the vast majority of cases, version will be 0 if it exists. If we run into a notification that is visible, but we don't have data for, should it potentially retoast or should we try our best to prevent that by treating a missing notification from getAll and from data differently? Possibly related to what Travis said.
https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:250: items.notificationsData = items.notificationsData || {}; On 2013/05/31 19:07:25, rgustafson wrote: > btw, did you see the suggestion on an already submitted cl to do: > storage.get({ > 'activeNotifications': {}, > 'recentDismissals': {} > }, function(items) { > ... > }); > for the defaults? 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. https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (!notificationData) { On 2013/05/31 19:07:25, rgustafson wrote: > On 2013/05/31 01:39:44, vadimt wrote: > > On 2013/05/31 01:05:36, rgustafson wrote: > > > 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. > > > > 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. > > 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. Thanks for the note anyways! https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:303: notificationData && On 2013/05/31 19:07:26, rgustafson wrote: > For the case of a notification in cards and in notifications but not in > notificationsData: > > This is already a kind of broken situation. Right now, previousVersion = > undefined and create will happen. The version of the previous card is lost, so > the best we can do is guess. For the vast majority of cases, version will be 0 > if it exists. > > If we run into a notification that is visible, but we don't have data for, > should it potentially retoast or should we try our best to prevent that by > treating a missing notification from getAll and from data differently? > > Possibly related to what Travis said. Since there was a disaster, and we are not sure what was the previous version, I believe it's OK to re-show the notification. Specially handling the '0' case is probably not worth it (given low probability), and then the recovery behavior for '0' and '1' would differ for no reason.
lgtm https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:250: items.notificationsData = items.notificationsData || {}; On 2013/05/31 19:24:05, vadimt wrote: > On 2013/05/31 19:07:25, rgustafson wrote: > > btw, did you see the suggestion on an already submitted cl to do: > > storage.get({ > > 'activeNotifications': {}, > > 'recentDismissals': {} > > }, function(items) { > > ... > > }); > > for the defaults? > > 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. Okay, making sure you didn't miss it cause it's a good move.
lgtm
estade@, please provide OWNER's approval
lgtm https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:510: if (typeof url != 'string') { no curlies
https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/15868004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:510: if (typeof url != 'string') { On 2013/06/03 20:20:10, Evan Stade wrote: > no curlies Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/15868004/15001
Message was sent while issue was closed.
Change committed as 203858 |
