Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
718203004:
[PUSH] Merge notifications and push messaging prompts (Closed)
|
Created:
6 years, 1 month ago by Miguel Garcia Modified:
6 years, 1 month ago CC:
chromium-reviews, markusheintz_, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[PUSH] Merge notifications and push messaging prompts
This first step does the following
- Shortcut the prompt for push
- Instead rely on the notification permission having been allowed.
- Push#hasPermission now considers both permissions before emiting a reply.
BUG=432930
Committed: https://crrev.com/114bfd9f1543c40e8fc1f94f90eb83d2e061c40f
Cr-Commit-Position: refs/heads/master@{#304590}
Patch Set 1 #
Total comments: 49
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 16
Patch Set 6 : #Messages
Total messages: 28 (6 generated)
miguelg@chromium.org changed reviewers: + johnme@chromium.org, mvanouwerkerk@chromium.org
Initial non Owners review
On 2014/11/13 17:48:46, Miguel Garcia wrote: > Initial non Owners review I am fixing the android compile failure (which is due to LKGR) can you have a look in the meantime?
On 2014/11/13 17:48:46, Miguel Garcia wrote: > Initial non Owners review I am fixing the android compile failure (which is due to LKGR) can you have a look in the meantime?
https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:143: std::string script_result; Nit: newline after this. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:145: ASSERT_TRUE(RunScript("requestPermission();", &script_result)); Nit: you can omit the semicolon after requestPermission() - it's cleaner. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:146: ASSERT_EQ("permission status - granted", script_result); Nit: insert newline after this. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:160: std::string script_result; Nit: newline after this. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:171: std::string script_result; Nit: newline after this. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:204: std::string script_result; Nit: insert newline after this. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:206: ASSERT_TRUE(RunScript("requestPermission();", &script_result)); Insert after this: ASSERT_EQ("permission status - granted", script_result); https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:33: ->GetContentSettingAndMaybeUpdateLastUsage( I'm not sure I understand this part correctly. Why use GetContentSettingAndMaybeUpdateLastUsage for Notifications, but GetContentSetting for Push? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:44: DCHECK(notifications_content_setting == CONTENT_SETTING_ALLOW); Nit: use DCHECK_EQ, here and below. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:61: const GURL& embedder_origin, Nit: let's be consistent and call this embedding_origin throughout the file. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:71: DLOG(WARNING) << "Notification permission has not been granted."; Delete this? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:84: DLOG(WARNING) << "Push permission was explicitly blocked."; Delete this? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.h:35: Profile* push_profile_; PermissionContextBase already has this pointer. Why not create a getter for it there, so all subclasses can use it when needed? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.h:36: ContentSettingsType push_setting_type_; Why is this a member? It is always set to CONTENT_SETTINGS_TYPE_PUSH_MESSAGING anyway. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/test/data/push... File chrome/test/data/push_messaging/test.html (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/test/data/push... chrome/test/data/push_messaging/test.html:51: function requestPermission() { Please rename to requestNotificationPermission to distinguish from Push permission. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/test/data/push... chrome/test/data/push_messaging/test.html:52: Notification.requestPermission(function(granted) { s/granted/permission/
Generally looks good. A few nits and questions. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_browsertest.cc:144: InfoBarResponder accepting_responder(browser(), true); Nit: it's probably more idiomatic to call registerServiceWorker before requestPermission, since webapps should work offline whether or not push notifications is enabled. Ditto elsewhere. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:66: ->GetContentSettingAndMaybeUpdateLastUsage( I'm not too familiar with AndMaybeUpdateLastUsage - is that what we want here? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:83: if (push_content_setting == CONTENT_SETTING_BLOCK) { How would we ever get to this state, if only the notifications permission is ever surfaced in UI? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.h:22: ContentSetting GetPermissionStatus( Nit: Could you add a "// PermissionContextBase implementation:" comment? Ditto for the protected override below. https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:32: void DecidePermission(content::WebContents* web_contents, Nit: Similarly "// PushMessagingPermissionContext implementation:" comment https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:44: void NotifyPermissionSet(const PermissionRequestID& id, Nit: Ditto "// PermissionContextBase implementation:" comment?
Thanks for the fast reviews! Adding OWNERS now https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_browsertest.cc (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:143: std::string script_result; On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: newline after this. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:144: InfoBarResponder accepting_responder(browser(), true); On 2014/11/13 18:50:06, johnme wrote: > Nit: it's probably more idiomatic to call registerServiceWorker before > requestPermission, since webapps should work offline whether or not push > notifications is enabled. Ditto elsewhere. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:145: ASSERT_TRUE(RunScript("requestPermission();", &script_result)); On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: you can omit the semicolon after requestPermission() - it's cleaner. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:146: ASSERT_EQ("permission status - granted", script_result); On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: insert newline after this. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:160: std::string script_result; On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: newline after this. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:171: std::string script_result; On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: newline after this. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:204: std::string script_result; On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Nit: insert newline after this. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_browsertest.cc:206: ASSERT_TRUE(RunScript("requestPermission();", &script_result)); On 2014/11/13 18:35:51, Michael van Ouwerkerk wrote: > Insert after this: > ASSERT_EQ("permission status - granted", script_result); Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:33: ->GetContentSettingAndMaybeUpdateLastUsage( On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > I'm not sure I understand this part correctly. Why use > GetContentSettingAndMaybeUpdateLastUsage for Notifications, but > GetContentSetting for Push? I made it more clear now, I think we want to keep the same logic as in the base class so that when you Decide a permission it is marked as used but when you just get the permission status it is not. I originally considered that trying to figure out the notification permission would be a good enough reason to consider it used but this seems more clear. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:44: DCHECK(notifications_content_setting == CONTENT_SETTING_ALLOW); On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Nit: use DCHECK_EQ, here and below. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:61: const GURL& embedder_origin, On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Nit: let's be consistent and call this embedding_origin throughout the file. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:66: ->GetContentSettingAndMaybeUpdateLastUsage( On 2014/11/13 18:50:06, johnme wrote: > I'm not too familiar with AndMaybeUpdateLastUsage - is that what we want here? Yes, the base class does the same. DecidePermission is called when you are trying to obtain the permission somehow so this is the right moment to mark it as last used. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:71: DLOG(WARNING) << "Notification permission has not been granted."; On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Delete this? I actually meant to leave this one, it will be helpful if we need to debug things. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:83: if (push_content_setting == CONTENT_SETTING_BLOCK) { On 2014/11/13 18:50:06, johnme wrote: > How would we ever get to this state, if only the notifications permission is > ever surfaced in UI? I just want to be a bit defensive about it just in case. Our UX might not let us do it but Opera or ContentShell or WebView can add that option. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:84: DLOG(WARNING) << "Push permission was explicitly blocked."; On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Delete this? Same as above. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.h:22: ContentSetting GetPermissionStatus( On 2014/11/13 18:50:06, johnme wrote: > Nit: Could you add a "// PermissionContextBase implementation:" comment? Ditto > for the protected override below. Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.h:35: Profile* push_profile_; On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > PermissionContextBase already has this pointer. Why not create a getter for it > there, so all subclasses can use it when needed? I don't think subclasses should this need in general and I rather not add extra methods to the base class that are only needed by Permission classes that are in flux. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.h:36: ContentSettingsType push_setting_type_; On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Why is this a member? It is always set to CONTENT_SETTINGS_TYPE_PUSH_MESSAGING > anyway. Well, because if we ever change CONTENT_SETTINGS_TYPE_PUSH_MESSAGING to something else I only need to change it in one place. I've made it const to be sure it cannot be changed. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:32: void DecidePermission(content::WebContents* web_contents, On 2014/11/13 18:50:06, johnme wrote: > Nit: Similarly "// PushMessagingPermissionContext implementation:" comment Done. https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:44: void NotifyPermissionSet(const PermissionRequestID& id, On 2014/11/13 18:50:06, johnme wrote: > Nit: Ditto "// PermissionContextBase implementation:" comment? I've gone with // PushMessagingPermissionContext: It is not technically accurate I know but it is way more maintainable. If we ever implement the method in PushMessagingPermissionContext or introduce some other class in the hirarchy we will never remember to update this. https://codereview.chromium.org/718203004/diff/1/chrome/test/data/push_messag... File chrome/test/data/push_messaging/test.html (right): https://codereview.chromium.org/718203004/diff/1/chrome/test/data/push_messag... chrome/test/data/push_messaging/test.html:51: function requestPermission() { On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > Please rename to requestNotificationPermission to distinguish from Push > permission. Done. https://codereview.chromium.org/718203004/diff/1/chrome/test/data/push_messag... chrome/test/data/push_messaging/test.html:52: Notification.requestPermission(function(granted) { On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > s/granted/permission/ Done.
miguelg@chromium.org changed reviewers: + bauerb@chromium.org
Hi! Would you mind doing an OWNERS review of this patch? bauerb: for content_settings fgorsky: for gcm
miguelg@chromium.org changed reviewers: + fgorski@chromium.org
fgorski@chromium.org: Please review changes in
https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.cc:71: DLOG(WARNING) << "Notification permission has not been granted."; On 2014/11/14 11:34:36, Miguel Garcia wrote: > On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > > Delete this? > > I actually meant to leave this one, it will be helpful if we need to debug > things. Could you at least reduce the severity if it's only for debugging? https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://codereview.chromium.org/718203004/diff/1/chrome/browser/services/gcm/... chrome/browser/services/gcm/push_messaging_permission_context.h:36: ContentSettingsType push_setting_type_; On 2014/11/14 11:34:36, Miguel Garcia wrote: > On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > > Why is this a member? It is always set to CONTENT_SETTINGS_TYPE_PUSH_MESSAGING > > anyway. > > Well, because if we ever change CONTENT_SETTINGS_TYPE_PUSH_MESSAGING to > something else I only need to change it in one place. I've made it const to be > sure it cannot be changed. Couldn't you make it a constant declared in the .cc file? https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context.cc:36: if (notifications_content_setting == CONTENT_SETTING_BLOCK || Nit: add braces for multiline conditions. https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context.cc:43: DCHECK_EQ(notifications_content_setting, CONTENT_SETTING_ALLOW); Put the expected value first for nicer error messages. https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context.cc:48: /** Use //-style comments, not Javadoc :) https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:15: static std::string EMBEDDER = "https://example.org"; This is going to create a static constructor, which is disallowed. You're going to have to define the constant as Plain Old Data, i.e. const char[]. One advantage of that is that const variables automatically get internal linkage, so the anonymous namespace isn't necessary anymore. Also, constants should be kConstantStyle; ALL_CAPS implies a preprocessor macro or an enum. https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:29: bool WasPersisted() { return was_persisted_; } Make this unix_hacker_style? https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:75: base::MessageLoop message_loop_; Use a TestBrowserThreadBundle? It's one less line, and saves you from having to worry about whether you need additional threads. https://codereview.chromium.org/718203004/diff/20001/chrome/browser/services/... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:79: virtual void SetUp() { I think this method overrides testing::Test::SetUp(), so it should get override.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Thanks for the quick review Bernhard! https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:71: DLOG(WARNING) << "Notification permission has not been granted."; Ok, note that there is a presubmit check to avoid (D)LOG(INFO) but it seems reasonable to bypass in this case. On 2014/11/14 12:31:33, Bernhard Bauer wrote: > On 2014/11/14 11:34:36, Miguel Garcia wrote: > > On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > > > Delete this? > > > > I actually meant to leave this one, it will be helpful if we need to debug > > things. > > Could you at least reduce the severity if it's only for debugging? https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.h:36: ContentSettingsType push_setting_type_; On 2014/11/14 12:31:33, Bernhard Bauer wrote: > On 2014/11/14 11:34:36, Miguel Garcia wrote: > > On 2014/11/13 18:35:52, Michael van Ouwerkerk wrote: > > > Why is this a member? It is always set to > CONTENT_SETTINGS_TYPE_PUSH_MESSAGING > > > anyway. > > > > Well, because if we ever change CONTENT_SETTINGS_TYPE_PUSH_MESSAGING to > > something else I only need to change it in one place. I've made it const to be > > sure it cannot be changed. > > Couldn't you make it a constant declared in the .cc file? Done. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context.cc:36: if (notifications_content_setting == CONTENT_SETTING_BLOCK || On 2014/11/14 12:31:33, Bernhard Bauer wrote: > Nit: add braces for multiline conditions. Done. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context.cc:43: DCHECK_EQ(notifications_content_setting, CONTENT_SETTING_ALLOW); On 2014/11/14 12:31:33, Bernhard Bauer wrote: > Put the expected value first for nicer error messages. Done. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context.cc:48: /** On 2014/11/14 12:31:33, Bernhard Bauer wrote: > Use //-style comments, not Javadoc :) Done. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:15: static std::string EMBEDDER = "https://example.org"; All done, thanks for the long explanation! On 2014/11/14 12:31:33, Bernhard Bauer wrote: > This is going to create a static constructor, which is disallowed. You're going > to have to define the constant as Plain Old Data, i.e. const char[]. > > One advantage of that is that const variables automatically get internal > linkage, so the anonymous namespace isn't necessary anymore. > > Also, constants should be kConstantStyle; ALL_CAPS implies a preprocessor macro > or an enum. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:29: bool WasPersisted() { return was_persisted_; } On 2014/11/14 12:31:34, Bernhard Bauer wrote: > Make this unix_hacker_style? Done. https://chromiumcodereview.appspot.com/718203004/diff/20001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:79: virtual void SetUp() { On 2014/11/14 12:31:34, Bernhard Bauer wrote: > I think this method overrides testing::Test::SetUp(), so it should get override. Done.
https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... chrome/browser/services/gcm/push_messaging_permission_context.cc:71: DLOG(WARNING) << "Notification permission has not been granted."; On 2014/11/17 11:43:22, Miguel Garcia wrote: > Ok, note that there is a presubmit check to avoid (D)LOG(INFO) but it seems > reasonable to bypass in this case. Yeah, I think technically you're supposed to use log level DEBUG for debugging information. Or you could use (D)VLOG, which has the advantage that the logging threshold can be controlled more granularily. https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:15: const GURL kEmbedderUrl(kEmbedder); This will still create a static constructor. Sorry, I should have mentioned my comment applied to both objects.
Moved LOG(INFO) to VLOG(1) which as you say removes the warning. On 2014/11/17 11:53:37, Bernhard Bauer wrote: > https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... > File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): > > https://chromiumcodereview.appspot.com/718203004/diff/1/chrome/browser/servic... > chrome/browser/services/gcm/push_messaging_permission_context.cc:71: > DLOG(WARNING) << "Notification permission has not been granted."; > On 2014/11/17 11:43:22, Miguel Garcia wrote: > > Ok, note that there is a presubmit check to avoid (D)LOG(INFO) but it seems > > reasonable to bypass in this case. > > Yeah, I think technically you're supposed to use log level DEBUG for debugging > information. Or you could use (D)VLOG, which has the advantage that the logging > threshold can be controlled more granularily. > > https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... > File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc > (right): > > https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... > chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:15: > const GURL kEmbedderUrl(kEmbedder); > This will still create a static constructor. Sorry, I should have mentioned my > comment applied to both objects.
https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/80001/chrome/browser/se... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:15: const GURL kEmbedderUrl(kEmbedder); On 2014/11/17 11:53:37, Bernhard Bauer wrote: > This will still create a static constructor. Sorry, I should have mentioned my > comment applied to both objects. Of course, sorry about that.
https://chromiumcodereview.appspot.com/718203004/diff/100001/chrome/browser/s... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/100001/chrome/browser/s... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:21: static const GURL kEmbedderUrl; Remove this now that you use GURL(kEmbedder)?
https://chromiumcodereview.appspot.com/718203004/diff/100001/chrome/browser/s... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://chromiumcodereview.appspot.com/718203004/diff/100001/chrome/browser/s... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:21: static const GURL kEmbedderUrl; On 2014/11/17 13:46:01, Bernhard Bauer wrote: > Remove this now that you use GURL(kEmbedder)? Done.
LGTM!
lgtm with nit https://chromiumcodereview.appspot.com/718203004/diff/120001/chrome/browser/s... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://chromiumcodereview.appspot.com/718203004/diff/120001/chrome/browser/s... chrome/browser/services/gcm/push_messaging_permission_context.h:37: Profile* push_profile_; Can you not call this |profile_| ?
LGTM, provided the answer to persist thingy does not mean "ask me again later..." https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:12: const ContentSettingsType kPushSettingType = Can you put these into an anonymous namespace? https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:54: // - You need to not have push permission explicitly blocked nit: dot at the end. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:73: false /* persist */, false /* granted */); Could you please remind me what the semantics of persist is here? If it does not lead to behavior of "ask me again later" I am ok with the code, but I am curious what this thing means, and why you are setting it to true only when successful. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:67: host_content_settings_map->SetContentSetting(pattern, pattern, setting, nit: move all of the parameters to the new line https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:69: } nit: new line https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:74: void SetUp() override { SetUp is typically public https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:123: SetContentSetting(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW); I think you are already testing for that in 114-116
Thanks for the review! https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_permission_context.cc (right): https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:12: const ContentSettingsType kPushSettingType = On 2014/11/17 18:24:06, fgorski wrote: > Can you put these into an anonymous namespace? Agreed online that it is not required (and I got the opposite comment from another reviewer). https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:54: // - You need to not have push permission explicitly blocked On 2014/11/17 18:24:06, fgorski wrote: > nit: dot at the end. Done. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.cc:73: false /* persist */, false /* granted */); On 2014/11/17 18:24:06, fgorski wrote: > Could you please remind me what the semantics of persist is here? > > If it does not lead to behavior of "ask me again later" I am ok with the code, > but I am curious what this thing means, and why you are setting it to true only > when successful. We are denying the permission here just because the permission for notifications has not been granted, so persisting that we don't want to grant push would be bad, if the user eventually grants notifications we want to make sure they can request push automatically. Note that in general there will no prompts for push after this change, we will only prompt for notifications which will follow the usual pattern. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_permission_context.h (right): https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context.h:37: Profile* push_profile_; On 2014/11/17 14:50:09, Michael van Ouwerkerk wrote: > Can you not call this |profile_| ? Done. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... File chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc (right): https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:67: host_content_settings_map->SetContentSetting(pattern, pattern, setting, On 2014/11/17 18:24:06, fgorski wrote: > nit: move all of the parameters to the new line this is what clank cl format has decided... https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:69: } On 2014/11/17 18:24:06, fgorski wrote: > nit: new line Done. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:74: void SetUp() override { On 2014/11/17 18:24:07, fgorski wrote: > SetUp is typically public Ok, I made it public but would like to understand why? We will not be calling the method from this class, it will be called on the superclass directly.. https://codereview.chromium.org/718203004/diff/120001/chrome/browser/services... chrome/browser/services/gcm/push_messaging_permission_context_unittest.cc:123: SetContentSetting(CONTENT_SETTINGS_TYPE_NOTIFICATIONS, CONTENT_SETTING_ALLOW); On 2014/11/17 18:24:06, fgorski wrote: > I think you are already testing for that in 114-116 Indeed, removed this duplicated check.
The CQ bit was checked by miguelg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/718203004/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/114bfd9f1543c40e8fc1f94f90eb83d2e061c40f Cr-Commit-Position: refs/heads/master@{#304590} |
