Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
842233003:
Push API: Require user visible notification, else show auto notification (Closed)
|
Created:
5 years, 11 months ago by johnme Modified:
5 years, 11 months ago Reviewers:
Peter Beverloo CC:
chromium-reviews, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@invalid Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPush API: Require user visible notification, else show auto notification
BUG=437277
Committed: https://crrev.com/48ccc7e7853f4a0c7f73fb81d97135f381d2d795
Cr-Commit-Position: refs/heads/master@{#310862}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address review comments #Patch Set 3 : Tweak comment #
Total comments: 8
Patch Set 4 : Review comments 2 #Patch Set 5 : Comment nit #
Total comments: 4
Patch Set 6 : Review nits #Patch Set 7 : Add TODO #Patch Set 8 : String nits #
Total comments: 4
Patch Set 9 : Fix includes and typo #
Messages
Total messages: 14 (2 generated)
johnme@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (left): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:155: DCHECK(application_id.IsValid()); Why are you removing this DCHECK? You're using the value all throughout. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:57: int CountNotifications(Profile* profile, const GURL& origin) { CountNotificationsForOrigin. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:234: // Showing then immediately closing a notification is not sufficient. This is a comment that should live in the header file. All these conditions don't really matter either, what you care about is that, at the time of this call, N>1. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:236: return; // The requirement is met by a pre-existing notification. Really? That would allow me to do a cycle like: [push 1] show a notification (N=1) [push 2] close the notification (N=0) [push 3] show a notification (N=1) ... Which would give me, for users that ignore notifications, 50% free background push messages. I'm not sure that we should care about the notification count *before* event execution at all. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:241: // show a generic notification. See https://github.com/w3c/push-api/pull/87 I would very much prefer not to refer to pull requests in a code comment. Referring to a bug, which then refers to the pull request would be better. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:252: notification_data.icon = GURL("about:blank"); GURL() https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: content::ChildProcessHost::kInvalidUniqueID /* render_process_id */); This is a bit awkward, but OK. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.h (right): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.h:86: void RequireUserVisibleUX(const PushMessagingApplicationId& application_id, Even though this is private, it would be great if we could add some documentation to the method because it's not clear what it does. Something along the lines of: // Developers are required to display a Web Notification in response to an // incoming push message in order to clarify to the user that something has // happened in the background. When they forget to do so, display a default // notification on their behalf.
Addressed review comments https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (left): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:155: DCHECK(application_id.IsValid()); On 2015/01/09 17:58:20, Peter Beverloo wrote: > Why are you removing this DCHECK? You're using the value all throughout. After discussing with Michael, we concluded that malformed input shouldn't crash the browser even in debug. The existing if condition two lines below covers this case correctly already. It's somewhat orthogonal; happy to split it into its own change if you'd rather. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:57: int CountNotifications(Profile* profile, const GURL& origin) { On 2015/01/09 17:58:21, Peter Beverloo wrote: > CountNotificationsForOrigin. Deleted. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:234: // Showing then immediately closing a notification is not sufficient. On 2015/01/09 17:58:20, Peter Beverloo wrote: > This is a comment that should live in the header file. Done. On 2015/01/09 17:58:20, Peter Beverloo wrote: > All these conditions > don't really matter either, what you care about is that, at the time of this > call, N>1. Disagree, but done - we shall tweak the heuristic later anyway. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:236: return; // The requirement is met by a pre-existing notification. On 2015/01/09 17:58:21, Peter Beverloo wrote: > Really? That would allow me to do a cycle like: > > [push 1] show a notification (N=1) > [push 2] close the notification (N=0) > [push 3] show a notification (N=1) > ... > > Which would give me, for users that ignore notifications, 50% free background > push messages. I'm not sure that we should care about the notification count > *before* event execution at all. We want to support e.g. dismissing a notification on other devices when the user reads it on one device. I guess it might be a problem if people send pairs of push messages, where the first shows a notification and then the second one immediately hides it again. We could fix that by requiring that the pre-existing notification has been showing for some minimum duration in order to qualify. But sure, I've removed this for now for simplicity, and we'll tweak this heuristic before shipping (I added a TODO). https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:241: // show a generic notification. See https://github.com/w3c/push-api/pull/87 On 2015/01/09 17:58:21, Peter Beverloo wrote: > I would very much prefer not to refer to pull requests in a code comment. > Referring to a bug, which then refers to the pull request would be better. Done. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:252: notification_data.icon = GURL("about:blank"); On 2015/01/09 17:58:20, Peter Beverloo wrote: > GURL() Done. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:259: content::ChildProcessHost::kInvalidUniqueID /* render_process_id */); On 2015/01/09 17:58:20, Peter Beverloo wrote: > This is a bit awkward, but OK. Acknowledged. https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.h (right): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.h:86: void RequireUserVisibleUX(const PushMessagingApplicationId& application_id, On 2015/01/09 17:58:21, Peter Beverloo wrote: > Even though this is private, it would be great if we could add some > documentation to the method because it's not clear what it does. Something along > the lines of: > > // Developers are required to display a Web Notification in response to an > // incoming push message in order to clarify to the user that something has > // happened in the background. When they forget to do so, display a default > // notification on their behalf. Done.
https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (left): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:155: DCHECK(application_id.IsValid()); On 2015/01/09 18:28:47, johnme wrote: > On 2015/01/09 17:58:20, Peter Beverloo wrote: > > Why are you removing this DCHECK? You're using the value all throughout. > > After discussing with Michael, we concluded that malformed input shouldn't crash > the browser even in debug. The existing if condition two lines below covers this > case correctly already. > > It's somewhat orthogonal; happy to split it into its own change if you'd rather. No, that's OK. Thanks for the info! https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_service_impl.cc:236: return; // The requirement is met by a pre-existing notification. On 2015/01/09 18:28:47, johnme wrote: > On 2015/01/09 17:58:21, Peter Beverloo wrote: > > Really? That would allow me to do a cycle like: > > > > [push 1] show a notification (N=1) > > [push 2] close the notification (N=0) > > [push 3] show a notification (N=1) > > ... > > > > Which would give me, for users that ignore notifications, 50% free background > > push messages. I'm not sure that we should care about the notification count > > *before* event execution at all. > > We want to support e.g. dismissing a notification on other devices when the user > reads it on one device. > > I guess it might be a problem if people send pairs of push messages, where the > first shows a notification and then the second one immediately hides it again. > We could fix that by requiring that the pre-existing notification has been > showing for some minimum duration in order to qualify. > > But sure, I've removed this for now for simplicity, and we'll tweak this > heuristic before shipping (I added a TODO). Smells like a cat-and-mouse game. We should consider such heuristics very carefully. https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15024: + <message name="IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_TITLE" desc="Title of the generic notification shown automatically when a site has received a push message, but failed to show/update/close a notification. Its purpose is both to inform users that new content may be available somewhere on that site, and so the user is aware that the website did something in the background."> "Title of an auto-generated notification informing the user that the site may have received an update in the background, in case the site did not satisfy the visible notification requirement itself." As the description, perhaps? It's a bit shorter, and translators don't need *all* the context. https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15025: + Website updated Didn't we want to include the website's host in the title? Relevant reading material: https://code.google.com/p/chromium/issues/detail?id=402191#c10 https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15028: + A site updated in the background. If we either display the origin in the title, or display the origin as a context message on the toast, "A site" immediately gets a meaning. This feels very much like the kind of nagging notification a user won't be able to do anything at all with. https://codereview.chromium.org/842233003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:241: notification_data.icon = GURL(); // TODO(johnme): Better icon? nit: two spaces before the comment.
Addressed review comments https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15024: + <message name="IDS_PUSH_MESSAGING_GENERIC_NOTIFICATION_TITLE" desc="Title of the generic notification shown automatically when a site has received a push message, but failed to show/update/close a notification. Its purpose is both to inform users that new content may be available somewhere on that site, and so the user is aware that the website did something in the background."> On 2015/01/09 18:46:25, Peter Beverloo wrote: > "Title of an auto-generated notification informing the user that the site may > have received an update in the background, in case the site did not satisfy the > visible notification requirement itself." > > As the description, perhaps? It's a bit shorter, and translators don't need > *all* the context. Done. https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15025: + Website updated On 2015/01/09 18:46:25, Peter Beverloo wrote: > Didn't we want to include the website's host in the title? > > Relevant reading material: > https://code.google.com/p/chromium/issues/detail?id=402191#c10 Done. https://codereview.chromium.org/842233003/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:15028: + A site updated in the background. On 2015/01/09 18:46:25, Peter Beverloo wrote: > If we either display the origin in the title, or display the origin as a context > message on the toast, "A site" immediately gets a meaning. This feels very much > like the kind of nagging notification a user won't be able to do anything at all > with. Done. https://codereview.chromium.org/842233003/diff/40001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/40001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:241: notification_data.icon = GURL(); // TODO(johnme): Better icon? On 2015/01/09 18:46:25, Peter Beverloo wrote: > nit: two spaces before the comment. Done.
Code lg minus two textual nits. Did you consider writing a test? https://codereview.chromium.org/842233003/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/842233003/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:15027: + </ph> updated "push.google.com updated" +has been? https://codereview.chromium.org/842233003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:234: // https://codereview.chromium.org/532523002/ once it lands. https://crbug.com/402698 gets resolved?
Addressed review comments > Did you consider writing a test? I'm about halfway through writing one (there's already a TODO) - will upload a follow-up patch on Monday, which I'd be happy to get merged. But it'd be nice to get this in for m41 so any developers trying out the experimental API have their expectations set (more) correctly about showing notifications. https://codereview.chromium.org/842233003/diff/80001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/842233003/diff/80001/chrome/app/generated_res... chrome/app/generated_resources.grd:15027: + </ph> updated On 2015/01/09 19:33:27, Peter Beverloo wrote: > "push.google.com updated" > > +has been? Done. https://codereview.chromium.org/842233003/diff/80001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/80001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_service_impl.cc:234: // https://codereview.chromium.org/532523002/ once it lands. On 2015/01/09 19:33:27, Peter Beverloo wrote: > https://crbug.com/402698 gets resolved? Done.
lgtm. Please be sure to CC me on the patch adding the test. https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:15: #include "chrome/browser/notifications/notification_ui_manager.h" This include isn't used. https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:28: #include "components/content_settings/core/common/content_settings_types.h" nit: these two includes don't seem to be used?
https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_service_impl.cc (right): https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:15: #include "chrome/browser/notifications/notification_ui_manager.h" On 2015/01/09 19:48:05, Peter Beverloo wrote: > This include isn't used. It's used by the following line: int notification_count = g_browser_process->notification_ui_manager()-> GetAllIdsByProfileAndSourceOrigin(profile_, application_id.origin).size(); https://codereview.chromium.org/842233003/diff/140001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_service_impl.cc:28: #include "components/content_settings/core/common/content_settings_types.h" On 2015/01/09 19:48:05, Peter Beverloo wrote: > nit: these two includes don't seem to be used? Done.
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/842233003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/48ccc7e7853f4a0c7f73fb81d97135f381d2d795 Cr-Commit-Position: refs/heads/master@{#310862} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
