Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2886433002:
[Android] Adding content settings provider for notification channels (Closed)
|
Created:
3 years, 7 months ago by awdf Modified:
3 years, 6 months ago CC:
chromium-reviews, msramek+watch_chromium.org, awdf+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, raymes+watch_chromium.org, agrieve+watch_chromium.org, markusheintz_, picksi1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Adding content settings provider for notification channels
- Right now it just attempts to create channels on grant, and delete
them on resetting the permission.
- Java side is not yet implemented so this is actually a no-op.
BUG=700377
Review-Url: https://codereview.chromium.org/2886433002
Cr-Commit-Position: refs/heads/master@{#478267}
Committed: https://chromium.googlesource.com/chromium/src/+/338aa5e24e36752be190da0009bb23e8f913566c
Patch Set 1 #Patch Set 2 : rename files #Patch Set 3 : update filenames elsewhere #
Total comments: 42
Patch Set 4 : Responding to comments #
Total comments: 27
Patch Set 5 : Responding to more comments #
Total comments: 5
Patch Set 6 : Rename to NotificationChannelsProviderAndroid #Patch Set 7 : Added unit tests (refactored out a class for jni calls) #
Total comments: 27
Patch Set 8 : responding to comments #
Total comments: 5
Patch Set 9 : Nits from patch set 8 #Patch Set 10 : adding some overrides to make the compile bot happy #
Total comments: 3
Patch Set 11 : Fix up test failures & real-life crash by making dcheck allow the global wildcard #Patch Set 12 : Use Origin.Serialize instead of GURL.spec() #
Total comments: 4
Patch Set 13 : Improve comment and fixup site engagement service test #Patch Set 14 : Improve comment and fixup site engagement service test #
Total comments: 11
Patch Set 15 : Comments from #14 and add override to test destructor to satisfy clang compile #Patch Set 16 : Conditionally include new header file #Dependent Patchsets: Messages
Total messages: 71 (35 generated)
peter@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: @CalledByNative nit: TODO w/ comment to remove once the the SDK versions in the Android BuildInfo class have been updated? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:41: AndroidSettingsProvider::~AndroidSettingsProvider() {} micro nit: {} -> = default; https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:59: return false; micro nit: {} (multi-line statement) https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) This warrants a comment https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:75: return result; When would creating a channel fail? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:90: // TODO(crbug.com/700377) delete all channels nit: if |content_type| == NOTIFICATIONS https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.h (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:17: namespace content_settings { nit: only code that's part of the content_settings component should be in this namespace. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:20: // channels on Android O+. It would be good to document the priority order of this provider. (I.e. it takes precedence over normal content settings, but makes place for supervised users and policy settings.) https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:21: class AndroidSettingsProvider : public ObservableProvider { I'd suggest calling this NotificationContentSettingsProviderAndroid. It's super long, I realise, but: (a) We shouldn't generalize this yet. It'd need to move to a new place anyway if it ends up being generalized. (b) "Settings" is much more overloaded than "ContentSettings" https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:40: void ShutdownOnUIThread() override; micro nit: I wouldn't put the empty lines between these methods, because the comment on line 26 applies to all of them. https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:11: #include "base/android/jni_android.h" None of the changes in this file are used? https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings.h:80: SETTING_SOURCE_SYSTEM, dito
raymes@chromium.org changed reviewers: + raymes@chromium.org
A few early comments. Thanks :) https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:57: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || Should we just DCHECK that it's only notifications for now? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) Would this be better as DCHECK too? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:76: } What if there was already a channel created but the channel has the opposite setting to what is being set? I feel as though maybe we should DCHECK in that case? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:79: case CONTENT_SETTING_ASK: This should never be passed in so I think we can remove this and fall through to the default in that case https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:84: return false; I'd suggest DCHECKing here too https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:90: // TODO(crbug.com/700377) delete all channels On 2017/05/15 15:10:08, Peter Beverloo wrote: > nit: if |content_type| == NOTIFICATIONS DCHECK here too?
Thanks for your comments both! https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: @CalledByNative On 2017/05/15 15:10:08, Peter Beverloo wrote: > nit: TODO w/ comment to remove once the the SDK versions in the Android > BuildInfo class have been updated? Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:41: AndroidSettingsProvider::~AndroidSettingsProvider() {} On 2017/05/15 15:10:08, Peter Beverloo wrote: > micro nit: {} -> = default; Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:57: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || On 2017/05/16 01:08:18, raymes wrote: > Should we just DCHECK that it's only notifications for now? But won't this get all types of content settings updates? Looking at https://cs.chromium.org/chromium/src/components/content_settings/core/browser... , it seems all providers may receive all content settings updates that go through setWebsiteSetting (until one of them returns true), but maybe I'm missing something? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:59: return false; On 2017/05/15 15:10:08, Peter Beverloo wrote: > micro nit: {} (multi-line statement) Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) On 2017/05/15 15:10:08, Peter Beverloo wrote: > This warrants a comment Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) On 2017/05/16 01:08:18, raymes wrote: > Would this be better as DCHECK too? hmmm good question. I agree it might be useful to document and enforce our assumption that notification permissions cannot be set dynamically on wildcard primary urls through SetWebsiteSetting, but I wonder if this is the best place to do that. How do you feel about adding a DCHECK to HostContentSettingsMap::SetWebsiteSettingCustomScope to ensure this? Feels like it's safer to expect people writing new callers to respect DCHECKs set there, but they are likely not gonna test their code on Android O and notice a DCHECK if we just add it here. However, adding a DCHECK on content type + url validity to HostContentSettingsMap::SetWebsiteSettingCustomScope feels like it might be unnecessarily specific, given we can just gracefully ignore that case here. While it's an assumption that might produce surprising behaviour if broken, it's not an assumption we rely on for this code to function correctly. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:75: return result; On 2017/05/15 15:10:08, Peter Beverloo wrote: > When would creating a channel fail? Well right now it does because I haven't implemented it yet :D Right now there's also the chance that reflection could fail with an exception. Happy to take this out in future though, once implemented & the reflection is gone (when we target O). https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:76: } On 2017/05/16 01:08:18, raymes wrote: > What if there was already a channel created but the channel has the opposite > setting to what is being set? I feel as though maybe we should DCHECK in that > case? Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:79: case CONTENT_SETTING_ASK: On 2017/05/16 01:08:18, raymes wrote: > This should never be passed in so I think we can remove this and fall through to > the default in that case Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:84: return false; On 2017/05/16 01:08:17, raymes wrote: > I'd suggest DCHECKing here too again I wonder about dchecking this earlier on, if this is an assumption we really care about. In an ideal world I would love it if these assumptions could be enforced at compile time, not runtime, eg by using a specialized content setting type for notifications that only has three possible values, rather than attempting to generalize everything. however i suppose maybe dchecking here is better than nothing in terms of adding a sanity check for our assumptions. Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:90: // TODO(crbug.com/700377) delete all channels On 2017/05/15 15:10:08, Peter Beverloo wrote: > nit: if |content_type| == NOTIFICATIONS Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:90: // TODO(crbug.com/700377) delete all channels On 2017/05/16 01:08:17, raymes wrote: > On 2017/05/15 15:10:08, Peter Beverloo wrote: > > nit: if |content_type| == NOTIFICATIONS > > DCHECK here too? again, I don't think I can? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.h (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:17: namespace content_settings { On 2017/05/15 15:10:09, Peter Beverloo wrote: > nit: only code that's part of the content_settings component should be in this > namespace. Done (I note every other content settings observable provider declares itself within the content_settings namespace..) https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:20: // channels on Android O+. On 2017/05/15 15:10:09, Peter Beverloo wrote: > It would be good to document the priority order of this provider. (I.e. it takes > precedence over normal content settings, but makes place for supervised users > and policy settings.) Done. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:21: class AndroidSettingsProvider : public ObservableProvider { On 2017/05/15 15:10:09, Peter Beverloo wrote: > I'd suggest calling this NotificationContentSettingsProviderAndroid. It's super > long, I realise, but: > > (a) We shouldn't generalize this yet. It'd need to move to a new place anyway > if it ends up being generalized. > (b) "Settings" is much more overloaded than "ContentSettings" Done. ..Although, what about 'NotificationChannelsProviderAndroid' - given we are restricting its scope anyway, that would be perhaps more descriptive? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:40: void ShutdownOnUIThread() override; On 2017/05/15 15:10:09, Peter Beverloo wrote: > micro nit: I wouldn't put the empty lines between these methods, because the > comment on line 26 applies to all of them. interesting, even if they're multiline? That would make this inconsistent with other ProviderInterface implementations.. https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:11: #include "base/android/jni_android.h" On 2017/05/15 15:10:09, Peter Beverloo wrote: > None of the changes in this file are used? Indeed, about the jni stuff.. I do need to add an entry to the map though, to satisfy this nonobvious static assert: https://cs.chromium.org/chromium/src/components/content_settings/core/browser... - but I forgot to update it.. Done. https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings.h:80: SETTING_SOURCE_SYSTEM, On 2017/05/15 15:10:09, Peter Beverloo wrote: > dito actually i do need to pick a settings source for the map, see above. Have decided to go with SETTING_SOURCE_USER now though, instead of adding a new one, since channel settings are ultimately decided by the user. (Does that seem right @raymes?)
Code lg % base::Feature, but I really need Raymes to review the content settings part. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.h (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:21: class AndroidSettingsProvider : public ObservableProvider { On 2017/05/16 15:17:36, awdf wrote: > On 2017/05/15 15:10:09, Peter Beverloo wrote: > > I'd suggest calling this NotificationContentSettingsProviderAndroid. It's > super > > long, I realise, but: > > > > (a) We shouldn't generalize this yet. It'd need to move to a new place > anyway > > if it ends up being generalized. > > (b) "Settings" is much more overloaded than "ContentSettings" > > Done. ..Although, what about 'NotificationChannelsProviderAndroid' - given we > are restricting its scope anyway, that would be perhaps more descriptive? Up to you. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.h:40: void ShutdownOnUIThread() override; On 2017/05/16 15:17:36, awdf wrote: > On 2017/05/15 15:10:09, Peter Beverloo wrote: > > micro nit: I wouldn't put the empty lines between these methods, because the > > comment on line 26 applies to all of them. > > interesting, even if they're multiline? That would make this inconsistent with > other ProviderInterface implementations.. Yeah, think of it as a "block" of implementations of the interface indicated by the comment. (Which should have s/ProviderInterface/ObservableProvider/ btw.) https://codereview.chromium.org/2886433002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: // TODO: Remove this and check BuildInfo.sdk_int() instead, once SdkVersion enum includes O. TODO(awdf) https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_factory.cc:113: #if defined(OS_ANDROID) Should we also guard the implementation behind a base::Feature so that we can enable (or disable!) it in one go? You can add a value to //chrome/common/chrome_features.h https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_factory.cc:114: auto android_system_provider = micro nit: rename https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:86: channel_status == NotificationChannelStatus::ENABLED)); These frighten me a bit. Until the mitigations are in place to avoid us from hitting these (e.g. the infobar changes) it'd be possible for Android O users to hit this, right? https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:92: return false; Is returning FALSE here the right thing to do? We did delete the channel.
Thanks! https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:57: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || On 2017/05/16 15:17:35, awdf wrote: > On 2017/05/16 01:08:18, raymes wrote: > > Should we just DCHECK that it's only notifications for now? > > But won't this get all types of content settings updates? > > Looking at > https://cs.chromium.org/chromium/src/components/content_settings/core/browser... > , it seems all providers may receive all content settings updates that go > through setWebsiteSetting (until one of them returns true), but maybe I'm > missing something? Ah good point, you're right. Sorry for the misguidance. https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) On 2017/05/16 15:17:35, awdf wrote: > On 2017/05/16 01:08:18, raymes wrote: > > Would this be better as DCHECK too? > > hmmm good question. > > I agree it might be useful to document and enforce our assumption that > notification permissions cannot be set dynamically on wildcard primary urls > through SetWebsiteSetting, but I wonder if this is the best place to do that. > > How do you feel about adding a DCHECK to > HostContentSettingsMap::SetWebsiteSettingCustomScope to ensure this? Feels like > it's safer to expect people writing new callers to respect DCHECKs set there, > but they are likely not gonna test their code on Android O and notice a DCHECK > if we just add it here. > > However, adding a DCHECK on content type + url validity to > HostContentSettingsMap::SetWebsiteSettingCustomScope feels like it might be > unnecessarily specific, given we can just gracefully ignore that case here. > While it's an assumption that might produce surprising behaviour if broken, it's > not an assumption we rely on for this code to function correctly. I guess my thought is that if there are callers passing in non-origins, we should try to find out about them because they are bugs that should be fixed. If this is a DCHECK, we have a much better chance of finding out about them, because we'll get a crash in tests, etc. I would be ok adding the specific DCHECK to HostContentSettingsMap::SetWebsiteSettingCustomScope but we would only be able to do it on Android, as other platforms can set non-origin patterns. That's why having it here seemed ok too. If we are worried about trying to write invalid URLs to Android and messing up the settings then we could use a CHECK. https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... File components/content_settings/core/common/content_settings.h (right): https://codereview.chromium.org/2886433002/diff/40001/components/content_sett... components/content_settings/core/common/content_settings.h:80: SETTING_SOURCE_SYSTEM, On 2017/05/16 15:17:36, awdf wrote: > On 2017/05/15 15:10:09, Peter Beverloo wrote: > > dito > > actually i do need to pick a settings source for the map, see above. Have > decided to go with SETTING_SOURCE_USER now though, instead of adding a new one, > since channel settings are ultimately decided by the user. > > (Does that seem right @raymes?) Yep, perfect. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_factory.cc:113: #if defined(OS_ANDROID) On 2017/05/16 16:15:34, Peter Beverloo wrote: > Should we also guard the implementation behind a base::Feature so that we can > enable (or disable!) it in one go? You can add a value to > //chrome/common/chrome_features.h +1 good idea :) https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:5: #include "chrome/browser/notifications/notification_content_settings_provider_android.h" Renaming is really a pain so I hate to suggest this but if we wanted to be more consistent with other provider names, I would suggest content_settings_notifications_provider_android.h/cc. I think all our other providers start with content_settings... https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:86: channel_status == NotificationChannelStatus::ENABLED)); On 2017/05/16 16:15:34, Peter Beverloo wrote: > These frighten me a bit. Until the mitigations are in place to avoid us from > hitting these (e.g. the infobar changes) it'd be possible for Android O users to > hit this, right? Won't these help to find the callsites that need to be changed though? My motivation for suggesting this was just to ensure that we don't have any UI that is expecting to update content settings, but not actually succeeding. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:92: return false; On 2017/05/16 16:15:34, Peter Beverloo wrote: > Is returning FALSE here the right thing to do? We did delete the channel. Good catch, we probably should return true. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:96: return false; I think it's safer to return true here (and in all the above return statements, except the first one). The reason being that I think we should try to keep the user prefs for notifications clean, even if we fail to write a pref here. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.h:19: // channels on Android O+. This provider takes precedence over normal content nit: normal content settings->pref provided content settings (or something to that effect) https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:58: {"notification_content_settings_android", nit: notification_android https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:60: NOTIFICATION_CONTENT_SETTINGS_ANDROID_PROVIDER, nit: it's probably ok to leave out CONTENT_SETTINGS here since they're all CONTENT_SETTINGS providers
going to start trying to implement GetRuleIterator next - see my question inline about how exactly to do that? https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifica... chrome/browser/notifications/android_settings_provider.cc:61: if (!primary_url.is_valid()) On 2017/05/17 03:13:38, raymes wrote: > On 2017/05/16 15:17:35, awdf wrote: > > On 2017/05/16 01:08:18, raymes wrote: > > > Would this be better as DCHECK too? > > > > hmmm good question. > > > > I agree it might be useful to document and enforce our assumption that > > notification permissions cannot be set dynamically on wildcard primary urls > > through SetWebsiteSetting, but I wonder if this is the best place to do that. > > > > How do you feel about adding a DCHECK to > > HostContentSettingsMap::SetWebsiteSettingCustomScope to ensure this? Feels > like > > it's safer to expect people writing new callers to respect DCHECKs set there, > > but they are likely not gonna test their code on Android O and notice a DCHECK > > if we just add it here. > > > > However, adding a DCHECK on content type + url validity to > > HostContentSettingsMap::SetWebsiteSettingCustomScope feels like it might be > > unnecessarily specific, given we can just gracefully ignore that case here. > > While it's an assumption that might produce surprising behaviour if broken, > it's > > not an assumption we rely on for this code to function correctly. > > I guess my thought is that if there are callers passing in non-origins, we > should try to find out about them because they are bugs that should be fixed. If > this is a DCHECK, we have a much better chance of finding out about them, > because we'll get a crash in tests, etc. > > I would be ok adding the specific DCHECK to > HostContentSettingsMap::SetWebsiteSettingCustomScope but we would only be able > to do it on Android, as other platforms can set non-origin patterns. That's why > having it here seemed ok too. If we are worried about trying to write invalid > URLs to Android and messing up the settings then we could use a CHECK. Okay I have added the DCHECK here and in HCSM on Android for additional safety. https://codereview.chromium.org/2886433002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: // TODO: Remove this and check BuildInfo.sdk_int() instead, once SdkVersion enum includes O. On 2017/05/16 16:15:34, Peter Beverloo wrote: > TODO(awdf) Done. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... File chrome/browser/content_settings/host_content_settings_map_factory.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_factory.cc:113: #if defined(OS_ANDROID) On 2017/05/16 16:15:34, Peter Beverloo wrote: > Should we also guard the implementation behind a base::Feature so that we can > enable (or disable!) it in one go? You can add a value to > //chrome/common/chrome_features.h Done. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/content_... chrome/browser/content_settings/host_content_settings_map_factory.cc:114: auto android_system_provider = On 2017/05/16 16:15:34, Peter Beverloo wrote: > micro nit: rename Done. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:5: #include "chrome/browser/notifications/notification_content_settings_provider_android.h" On 2017/05/17 03:13:38, raymes wrote: > Renaming is really a pain so I hate to suggest this but if we wanted to be more > consistent with other provider names, I would suggest > content_settings_notifications_provider_android.h/cc. I think all our other > providers start with content_settings... I was considering changing it to notification_channels_provider.h/cc actually, to be a bit more descriptive, WDYT? (If not, I'm happy to change it to content_settings_notifications_provider_android instead) https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:44: NotificationContentSettingsProviderAndroid::GetRuleIterator( @raymes I'm considering how to implement this and I think I can get away with using something like this ListIterator implementation defined in one of the tests: https://cs.chromium.org/chromium/src/components/content_settings/core/browser... - since none of the channel rules should overlap with each other, we don't need a RuleIteratorImpl (which takes precedence into account), right? So I plan to just grab a list/vector of existing channels from java, transform them into Rules & stick them in a ListIterator, sound ok? https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:86: channel_status == NotificationChannelStatus::ENABLED)); On 2017/05/16 16:15:34, Peter Beverloo wrote: > These frighten me a bit. Until the mitigations are in place to avoid us from > hitting these (e.g. the infobar changes) it'd be possible for Android O users to > hit this, right? Right now it wouldn't crash because the 'else' is never hit since the java always returns UNAVAILABLE.. but if the java side to create/check channels gets implemented before we adapt the site settings UI, (which seems likely now I'm developing behind a flag) you're right that this would crash if someone blocks/enables a site from site settings with that flag enabled. However if that flag is disabled by default is this actually a problem? https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:92: return false; On 2017/05/16 16:15:34, Peter Beverloo wrote: > Is returning FALSE here the right thing to do? We did delete the channel. I think you're right and we should be returning true here actually to indicate we handled it. Done. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:96: return false; On 2017/05/17 03:13:38, raymes wrote: > I think it's safer to return true here (and in all the above return statements, > except the first one). The reason being that I think we should try to keep the > user prefs for notifications clean, even if we fail to write a pref here. shouldn't really make a difference since this is after a NOTREACHED, and I've added another DCHECK to HCSM that there are no other possible content setting values passed in for notifications on android, but changed to true here anyway for consistency. https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.h:19: // channels on Android O+. This provider takes precedence over normal content On 2017/05/17 03:13:39, raymes wrote: > nit: normal content settings->pref provided content settings > > (or something to that effect) Done. https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:58: {"notification_content_settings_android", On 2017/05/17 03:13:39, raymes wrote: > nit: notification_android Done. https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/2886433002/diff/60001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.h:60: NOTIFICATION_CONTENT_SETTINGS_ANDROID_PROVIDER, On 2017/05/17 03:13:39, raymes wrote: > nit: it's probably ok to leave out CONTENT_SETTINGS here since they're all > CONTENT_SETTINGS providers Done.
Getting close now. I'd refrain from adding more code here in this CL so we can get this landed quicker. What were you thinking for tests for this class? I think unittests would be good but it might not make sense to add them until the Java side is filled in. Thanks! https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:5: #include "chrome/browser/notifications/notification_content_settings_provider_android.h" On 2017/05/17 17:14:41, awdf wrote: > On 2017/05/17 03:13:38, raymes wrote: > > Renaming is really a pain so I hate to suggest this but if we wanted to be > more > > consistent with other provider names, I would suggest > > content_settings_notifications_provider_android.h/cc. I think all our other > > providers start with content_settings... > > I was considering changing it to notification_channels_provider.h/cc actually, > to be a bit more descriptive, WDYT? (If not, I'm happy to change it to > content_settings_notifications_provider_android instead) So a few quick notes: 1) Keeping the _android suffix seems good, since this is android only but I'm no expert with android code/conventions. 2) All our other providers filenames have a content_settings_ prefixes. However the name of the classes don't have ContentSettings prefixes which is kind of inconsistent with the naming style. So I don't feel strongly whether we keep the content_settings_ prefixes on the filenames. 3) I don't feel strongly about keeping "channels" in the name or not, I'll leave it up to you folks. 4) Do we tend to use Notifications (plural) when naming things? I'll also leave that to you folks. So any of the below are fine with me (the last one is probably might slight preference): notifications_channels_provider_android.h/cc -> NotificationsChannelsProviderAndroid notifications_provider_android.h/cc -> NotificationsProviderAndroid content_settings_notifications_provider_android.h/cc -> NotificationsProviderAndroid https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:44: NotificationContentSettingsProviderAndroid::GetRuleIterator( On 2017/05/17 17:14:41, awdf wrote: > @raymes I'm considering how to implement this and I think I can get away with > using something like this ListIterator implementation defined in one of the > tests: > https://cs.chromium.org/chromium/src/components/content_settings/core/browser... > - since none of the channel rules should overlap with each other, we don't need > a RuleIteratorImpl (which takes precedence into account), right? > > So I plan to just grab a list/vector of existing channels from java, transform > them into Rules & stick them in a ListIterator, sound ok? Overall that sounds good. Let's land this CL as it is before adding more to it though, since we're getting close :) Also, I think we may need to sort the list before returning it. ContentSettingsPatterns have a particular precedence order even when they are disjoint (which is basically a form of sorting by host/port/etc). Using std::sort on a vector of ContentSettingsPatterns should do the trick? Also don't worry too much about factoring out that ListIterator from the test to share code. I think it's fine to add a new subclass. https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:76: return result; nit: probably we should return true here regardless. I don't think there is a reason to fall through to the pref provider even if this fails https://codereview.chromium.org/2886433002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:400: CONTENT_SETTING_DEFAULT); nit: let's leave this DCHECK of here for now. The only thing this is checking that isn't already checked is that client code won't try to store ASK (we already check validity of other settings). But that applies to many content settings and we should probably address that separately. The DCHECK above seems ok for now.
Renamed to NotificationChannelsProviderAndroid Not sure what's best for tests, I don't know how I could mock out the java side since it's basically a bunch of static calls.. Once the Java side is implemented maybe I could do some kind of end-to-end test like we do for NotificationPlatformBridge.java? https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/60001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:44: NotificationContentSettingsProviderAndroid::GetRuleIterator( On 2017/05/18 00:39:37, raymes wrote: > On 2017/05/17 17:14:41, awdf wrote: > > @raymes I'm considering how to implement this and I think I can get away with > > using something like this ListIterator implementation defined in one of the > > tests: > > > https://cs.chromium.org/chromium/src/components/content_settings/core/browser... > > - since none of the channel rules should overlap with each other, we don't > need > > a RuleIteratorImpl (which takes precedence into account), right? > > > > So I plan to just grab a list/vector of existing channels from java, transform > > them into Rules & stick them in a ListIterator, sound ok? > > Overall that sounds good. Let's land this CL as it is before adding more to it > though, since we're getting close :) > > Also, I think we may need to sort the list before returning it. > ContentSettingsPatterns have a particular precedence order even when they are > disjoint (which is basically a form of sorting by host/port/etc). Using > std::sort on a vector of ContentSettingsPatterns should do the trick? > > Also don't worry too much about factoring out that ListIterator from the test to > share code. I think it's fine to add a new subclass. OK. Happy to land this one with just the creating-channels part and leave the rule iterator to a further cl. https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:76: return result; On 2017/05/18 00:39:37, raymes wrote: > nit: probably we should return true here regardless. I don't think there is a > reason to fall through to the pref provider even if this fails Turns out we can't create channels as blocked, at least for now (I'll forward you some context). So we may need to store initially-blocked channels in user prefs, until the user enables them in site settings, at which point we can create a channel and return true. I'd like to do this by making the new code resilient to channel-initialization failing and have it fallback to user prefs, rather than assuming channel creation will always work/fail in different scenarios, and then having to adapt if/when it becomes possible to create blocked channels. I appreciate that having multiple providers handle the same origin complicates things, but with the precedence ordering of the content settings providers, I don't see why this shouldn't work (but maybe I'm missing something). https://codereview.chromium.org/2886433002/diff/80001/components/content_sett... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/80001/components/content_sett... components/content_settings/core/browser/host_content_settings_map.cc:400: CONTENT_SETTING_DEFAULT); On 2017/05/18 00:39:37, raymes wrote: > nit: let's leave this DCHECK of here for now. The only thing this is checking > that isn't already checked is that client code won't try to store ASK (we > already check validity of other settings). But that applies to many content > settings and we should probably address that separately. > > The DCHECK above seems ok for now. Acknowledged.
Thanks Anita. For testing I think mocking out the Java calls is a good idea (introduce a helper class that just makes the Java calls and mock it in tests). This would allow us to test the provider in a similar way to other providers. But please chat with Peter to get his thoughts too as I'm not very familiar with testing with Android. https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... File chrome/browser/notifications/notification_content_settings_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... chrome/browser/notifications/notification_content_settings_provider_android.cc:76: return result; On 2017/05/19 17:59:57, awdf wrote: > On 2017/05/18 00:39:37, raymes wrote: > > nit: probably we should return true here regardless. I don't think there is a > > reason to fall through to the pref provider even if this fails > > Turns out we can't create channels as blocked, at least for now (I'll forward > you some context). > > So we may need to store initially-blocked channels in user prefs, until the user > enables them in site settings, at which point we can create a channel and return > true. I'd like to do this by making the new code resilient to > channel-initialization failing and have it fallback to user prefs, rather than > assuming channel creation will always work/fail in different scenarios, and then > having to adapt if/when it becomes possible to create blocked channels. > > I appreciate that having multiple providers handle the same origin complicates > things, but with the precedence ordering of the content settings providers, I > don't see why this shouldn't work (but maybe I'm missing something). That's really unfortunate :( Personally I think we will be compromising too much if we had to store some settings in content settings and some in android settings. We're dealing with a feature that is already adding complexity to the system and this would require more complexity - both in the code and in the user experience. In any case, let's land this CL and continue that discussion separately (if you can CC me in on the relevant threads, that would be great :). For now I would prefer to return true here to ensure that we don't touch the pref provider on Android O. Coordinating settings from 2 providers may have other complications. As we work out the high level direction we can dig into how we want to move forward in the code.
On 2017/05/22 00:37:42, raymes wrote: > Thanks Anita. > > For testing I think mocking out the Java calls is a good idea (introduce a > helper class that just makes the Java calls and mock it in tests). This would > allow us to test the provider in a similar way to other providers. But please > chat with Peter to get his thoughts too as I'm not very familiar with testing > with Android. Done. Introduced an inner helper class and mocked it out in tests. > > https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... > File > chrome/browser/notifications/notification_content_settings_provider_android.cc > (right): > > https://codereview.chromium.org/2886433002/diff/80001/chrome/browser/notifica... > chrome/browser/notifications/notification_content_settings_provider_android.cc:76: > return result; > On 2017/05/19 17:59:57, awdf wrote: > > On 2017/05/18 00:39:37, raymes wrote: > > > nit: probably we should return true here regardless. I don't think there is > a > > > reason to fall through to the pref provider even if this fails > > > > Turns out we can't create channels as blocked, at least for now (I'll forward > > you some context). > > > > So we may need to store initially-blocked channels in user prefs, until the > user > > enables them in site settings, at which point we can create a channel and > return > > true. I'd like to do this by making the new code resilient to > > channel-initialization failing and have it fallback to user prefs, rather than > > assuming channel creation will always work/fail in different scenarios, and > then > > having to adapt if/when it becomes possible to create blocked channels. > > > > I appreciate that having multiple providers handle the same origin complicates > > things, but with the precedence ordering of the content settings providers, I > > don't see why this shouldn't work (but maybe I'm missing something). > > That's really unfortunate :( Personally I think we will be compromising too much > if we had to store some settings in content settings and some in android > settings. We're dealing with a feature that is already adding complexity to the > system and this would require more complexity - both in the code and in the user > experience. > > In any case, let's land this CL and continue that discussion separately (if you > can CC me in on the relevant threads, that would be great :). For now I would > prefer to return true here to ensure that we don't touch the pref provider on > Android O. Coordinating settings from 2 providers may have other complications. > As we work out the high level direction we can dig into how we want to move > forward in the code. sgtm. Will copy you in the latest thread so you're kept up to date.
Thanks! lg with a few minor comments https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:26: namespace { nit: newline after this https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:93: const std::string origin = primary_url.spec(); One thing I didn't consider earlier: Some valid non-wildcard patterns are actually not origins. For example file:///path/x patterns are valid. I don't think these work on Android notifications though. It may be better to do this: std::string origin = primary_pattern.ToString(); DCHECK(!url::Origin(GURL(origin)).is_unique()); That will give us firmer guarantees and we can loosen the restrictions later if need (though I don't suspect we will need to for android). Also I don't think we really need to use the output of .spec() as the stored representation. It's going to have to be put back into a ContentSettingsPattern anyway at the end of the day so it doesn't seem necessary to take the GURL parsed version. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:18: namespace { nit: newline after this https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: I don't think we use (c) anymore https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:50: EXPECT_EQ(true, result); nit (here and below): EXPECT_TRUE(result) https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: } nit: could we have a test which checks that the provider returns false if ShouldUseChannelSettings returns false?
lgtm with the above comments applied
lgtm % nits https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:45: std::unique_ptr<NotificationChannelsBridge> bridge); Consider making this private with a friends declaration for NotificationChannelsProviderAndroidTest. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:53: nit: no blank lines https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:19: NotificationChannelsProviderAndroidTest() {} TEST_F(X, Y) means that you're creating a test with a given fixture, i.e. a class that (indirectly) extends testing::Test with additional functionality. You're not adding anything here. In those cases you can just use TEST(X, Y) w/o declaring the class. However, each test here starts with the same block of code: auto* mock_bridge = new MockNotificationChannelsBridge(); EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true)); NotificationChannelsProviderAndroid channels_provider( base::WrapUnique(mock_bridge)); // ... channels_provider.ShutdownOnUIThread(); It would be great if |channels_provider| can be a protected member of the fixture. This reduces duplication, and a single point of allocation makes the friend declaration you need in NCPA easier. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:24: public: nit: define dtor https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:33: SetWebsiteSettingAllowedWhenChannelUnavailable_CreatesEnabledChannelAndReturnsTrue) { Such descriptive names are super rare in Chromium C++ code. Can we just call this "CreatesEnabledChannelWhenUnavailable"? Especially the "AndReturnsTrue" is not super interesting. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:40: EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/")) nit: consider using a constant for the origin.
https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:26: namespace { On 2017/05/24 00:59:50, raymes wrote: > nit: newline after this Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:93: const std::string origin = primary_url.spec(); On 2017/05/24 00:59:51, raymes wrote: > One thing I didn't consider earlier: > Some valid non-wildcard patterns are actually not origins. For example > file:///path/x patterns are valid. I don't think these work on Android > notifications though. > > It may be better to do this: > std::string origin = primary_pattern.ToString(); > DCHECK(!url::Origin(GURL(origin)).is_unique()); > Done. > That will give us firmer guarantees and we can loosen the restrictions later if > need (though I don't suspect we will need to for android). Also I don't think we > really need to use the output of .spec() as the stored representation. It's > going to have to be put back into a ContentSettingsPattern anyway at the end of > the day so it doesn't seem necessary to take the GURL parsed version. I used spec() in case SetWebsiteSetting gets called twice for conceptually the same origin but different 'pattern.ToString()'s, so that we don't end up with two channels for the same origin. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:18: namespace { On 2017/05/24 00:59:51, raymes wrote: > nit: newline after this Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:45: std::unique_ptr<NotificationChannelsBridge> bridge); On 2017/05/24 10:57:42, Peter Beverloo wrote: > Consider making this private with a friends declaration for > NotificationChannelsProviderAndroidTest. Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.h:53: On 2017/05/24 10:57:42, Peter Beverloo wrote: > nit: no blank lines Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/24 00:59:51, raymes wrote: > nit: I don't think we use (c) anymore > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:19: NotificationChannelsProviderAndroidTest() {} On 2017/05/24 10:57:43, Peter Beverloo wrote: > TEST_F(X, Y) means that you're creating a test with a given fixture, i.e. a > class that (indirectly) extends testing::Test with additional functionality. > You're not adding anything here. In those cases you can just use TEST(X, Y) w/o > declaring the class. > > However, each test here starts with the same block of code: > > auto* mock_bridge = new MockNotificationChannelsBridge(); > EXPECT_CALL(*mock_bridge, > ShouldUseChannelSettings()).WillOnce(Return(true)); > NotificationChannelsProviderAndroid channels_provider( > base::WrapUnique(mock_bridge)); > // ... > channels_provider.ShutdownOnUIThread(); > > It would be great if |channels_provider| can be a protected member of the > fixture. This reduces duplication, and a single point of allocation makes the > friend declaration you need in NCPA easier. Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:24: public: On 2017/05/24 10:57:43, Peter Beverloo wrote: > nit: define dtor Done. Do I also need to define a ctor? https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:33: SetWebsiteSettingAllowedWhenChannelUnavailable_CreatesEnabledChannelAndReturnsTrue) { On 2017/05/24 10:57:42, Peter Beverloo wrote: > Such descriptive names are super rare in Chromium C++ code. Can we just call > this "CreatesEnabledChannelWhenUnavailable"? Especially the "AndReturnsTrue" is > not super interesting. I'd argue that 'CreatesEnabledChannelWhenUnavailable' is not at all clear on its own (when What is unavailable?). I think when tests fail the test name should be useful not cryptic. I agree wholeheartedly that the rest of chromium sucks at this. No reason not to make our own test failure messages more useful though? When something fails I wanna know what happened before checking the logs! However, I have compromised by removing 'AndReturnsTrue' in some places in the interests of keeping line length <80 chars. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:40: EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/")) On 2017/05/24 10:57:43, Peter Beverloo wrote: > nit: consider using a constant for the origin. Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:40: EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/")) On 2017/05/24 10:57:43, Peter Beverloo wrote: > nit: consider using a constant for the origin. Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:50: EXPECT_EQ(true, result); On 2017/05/24 00:59:51, raymes wrote: > nit (here and below): EXPECT_TRUE(result) Done. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: } On 2017/05/24 00:59:51, raymes wrote: > nit: could we have a test which checks that the provider returns false if > ShouldUseChannelSettings returns false? Done.
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:24: public: On 2017/06/01 15:11:22, awdf wrote: > On 2017/05/24 10:57:43, Peter Beverloo wrote: > > nit: define dtor > > Done. Do I also need to define a ctor? No, that's fine. It's just virtual destructors that we need to make sure we override. https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:33: SetWebsiteSettingAllowedWhenChannelUnavailable_CreatesEnabledChannelAndReturnsTrue) { On 2017/06/01 15:11:22, awdf wrote: > On 2017/05/24 10:57:42, Peter Beverloo wrote: > > Such descriptive names are super rare in Chromium C++ code. Can we just call > > this "CreatesEnabledChannelWhenUnavailable"? Especially the "AndReturnsTrue" > is > > not super interesting. > > I'd argue that 'CreatesEnabledChannelWhenUnavailable' is not at all clear on its > own (when What is unavailable?). > > I think when tests fail the test name should be useful not cryptic. > > I agree wholeheartedly that the rest of chromium sucks at this. No reason not to > make our own test failure messages more useful though? When something fails I > wanna know what happened before checking the logs! > > However, I have compromised by removing 'AndReturnsTrue' in some places in the > interests of keeping line length <80 chars. Okay! https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:43: void InitChannelsProvider(bool shouldUseChannels) { should_use_channels, lines 45, 54, 65 etc too https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:46: channels_provider_ = might be nice to add a comment about why we can't use base::MakeUnique here
https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:43: void InitChannelsProvider(bool shouldUseChannels) { On 2017/06/01 15:26:11, Peter Beverloo wrote: > should_use_channels, lines 45, 54, 65 etc too Done. https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:46: channels_provider_ = On 2017/06/01 15:26:11, Peter Beverloo wrote: > might be nice to add a comment about why we can't use base::MakeUnique here Done. (This is because the constructor is private right?)
Note that there's a compile failure: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.andro... https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:46: channels_provider_ = On 2017/06/01 16:04:29, awdf wrote: > On 2017/06/01 15:26:11, Peter Beverloo wrote: > > might be nice to add a comment about why we can't use base::MakeUnique here > > Done. (This is because the constructor is private right?) Yes.
On 2017/06/01 16:07:47, Peter Beverloo wrote: > Note that there's a compile failure: > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.andro... > Fixed.
On 2017/05/24 01:14:07, raymes wrote: > lgtm with the above comments applied Hi Raymes, I applied all your comments, except for the one about not using spec() as the stored representation, see my reply to your comment. Going to submit this now in the interests of getting some code landed, but let me know if you're not happy with anything and I will fix it in a followup. (Note this is all behind a flag). Thanks for the thorough review! Another one for GetRuleIterator will be headed your way soon.. Cheers Anita
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, peter@chromium.org Link to the patchset: https://codereview.chromium.org/2886433002/#ps180001 (title: "adding some overrides to make the compile bot happy")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== [Android] Adding content settings provider for notification channels - Right now it just attempts to create channels on grant, and delete them on resetting the permission. - Java side is not yet implemented so this is actually a no-op. BUG=700377 ========== to ========== [Android] Adding content settings provider for notification channels - Right now it just attempts to create channels on grant, and delete them on resetting the permission. - Java side is not yet implemented so this is actually a no-op. BUG=700377 ==========
awdf@chromium.org changed reviewers: + miguelg@google.com
+miguelg for OWNERS review of chrome/android/BUILD.gn
miguelg@chromium.org changed reviewers: + miguelg@chromium.org
lgtm for chrome/android/BUILD.gn
The CQ bit was checked by awdf@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by awdf@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:96: const std::string origin = primary_url.spec(); Ah thanks for explaining, that makes sense. Since we expect this to be an origin, using origin.Serialize() may be better than spec() but it's not major issue e.g. url::Origin origin(GURL(primary_pattern.ToString())); DCHECK(!origin.unique()); const std::string origin_string = origin.Serialize();
peter@ & raymes@ - PTAL at updated DCHECK in host_content_settings_map.cc and new early return statement in notification_channels_provider_android.cc if primary_pattern matches all hosts. The updated DCHECK stops tests failing and means we don't crash when the global notifications setting is toggled in Site Settings. I also changed it to use Origin.Serialize while I was at it. https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:96: const std::string origin = primary_url.spec(); On 2017/06/04 23:10:50, raymes wrote: > Ah thanks for explaining, that makes sense. > > Since we expect this to be an origin, using origin.Serialize() may be better > than spec() but it's not major issue > > e.g. > url::Origin origin(GURL(primary_pattern.ToString())); > DCHECK(!origin.unique()); > const std::string origin_string = origin.Serialize(); Done. Since we will probably use this for the user-visible channel name as well as the channel id I agree .Serialize is preferable, since it drops the '/' suffix. Curious though whether there are other reasons it's better?
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Still lgtm. Is it worth adding a test? https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:95: // This provider only handles settings for specific origins. nit: I'd place the comment above the conditional and remove the brackets. (The conditional validates the statement made on the line following, which reads a bit awkward.) https://codereview.chromium.org/2886433002/diff/220001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/220001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:405: #endif This is getting really complicated to read. What about the following? #if defined(OS_ANDROID) && DCHECK_IS_ON() // The notification permission on Android only allows valid origins, or the // wildcard origin for the default setting, with a restricted set of settings. if (content_type == CONTENT_SETTINGS_TYPE_NOTIFICATIONS && !primary_pattern.MatchesAllHosts()) { // The |primary_pattern| must be a valid GURL as it may be presented to the // user: Android Notification Channels may be used on Android O onward. DCHECK(GURL(primary_pattern.ToString()).is_valid()); ContentSetting setting = content_settings::ValueToContentSetting(value.get()); // Disallow notification content setting updates for specific origins that // are not ALLOW/BLOCK/DEFAULT on Android. DCHECK(setting == CONTENT_SETTING_ALLOW || setting == CONTENT_SETTING_BLOCK || setting == CONTENT_SETTING_DEFAULT); } #endif
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
had to fix up a test too that was setting a notification CS to ASK - changed it to DEFAULT. https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:95: // This provider only handles settings for specific origins. On 2017/06/05 15:11:31, Peter Beverloo wrote: > nit: I'd place the comment above the conditional and remove the brackets. (The > conditional validates the statement made on the line following, which reads a > bit awkward.) Done. https://codereview.chromium.org/2886433002/diff/220001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/220001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:405: #endif On 2017/06/05 15:11:31, Peter Beverloo wrote: > This is getting really complicated to read. What about the following? > > #if defined(OS_ANDROID) && DCHECK_IS_ON() > // The notification permission on Android only allows valid origins, or the > // wildcard origin for the default setting, with a restricted set of settings. > if (content_type == CONTENT_SETTINGS_TYPE_NOTIFICATIONS && > !primary_pattern.MatchesAllHosts()) { > // The |primary_pattern| must be a valid GURL as it may be presented to the > // user: Android Notification Channels may be used on Android O onward. > DCHECK(GURL(primary_pattern.ToString()).is_valid()); > > ContentSetting setting = > content_settings::ValueToContentSetting(value.get()); > > // Disallow notification content setting updates for specific origins that > // are not ALLOW/BLOCK/DEFAULT on Android. > DCHECK(setting == CONTENT_SETTING_ALLOW || > setting == CONTENT_SETTING_BLOCK || > setting == CONTENT_SETTING_DEFAULT); > } > #endif Ooh, I didn't know about DCHECK_IS_ON. thanks! Done.
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
picksi@google.com changed reviewers: + picksi@google.com
Two comments from me, do something about it if you agree. I won't be upset if you don't! https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:105: case CONTENT_SETTING_BLOCK: { Have these two cases fall through and then later in the code having three different checks for which 'case' is operating feels a bit nasty. Is it possible to refactor out a single function that wraps the code that would be duplicated in both cases? Something like SetContentSetting(bridge, true); in the fist case and SetContentSetting(bridge, false); in the second. https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:111: DCHECK(!(setting == CONTENT_SETTING_ALLOW && This DCHECK does my head in trying to understand what the intent is! It looks like its looking ensuring that the state has changed? Is it possible to make this more understandable? Possibly just a single DCHECK that has a single comparison in it? e.g. DCHECK(oldStatus != newStatus) [or whatever the intention is!]
lgtm https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:96: const std::string origin = primary_url.spec(); On 2017/06/05 14:57:15, awdf wrote: > On 2017/06/04 23:10:50, raymes wrote: > > Ah thanks for explaining, that makes sense. > > > > Since we expect this to be an origin, using origin.Serialize() may be better > > than spec() but it's not major issue > > > > e.g. > > url::Origin origin(GURL(primary_pattern.ToString())); > > DCHECK(!origin.unique()); > > const std::string origin_string = origin.Serialize(); > > Done. Since we will probably use this for the user-visible channel name as well > as the channel id I agree .Serialize is preferable, since it drops the '/' > suffix. Curious though whether there are other reasons it's better? Nothing major, just slightly simpler. https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:97: } Ahh, makes sense. I would probably change this to be the same check as in other providers: if (primary_pattern == ContentSettingsPattern::Wildcard() && secondary_pattern == ContentSettingsPattern::Wildcard() && resource_identifier.empty()) { return false; } https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:98: GURL primary_url = GURL(primary_pattern.ToString()); nit: this is now unused https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:111: DCHECK(!(setting == CONTENT_SETTING_ALLOW && On 2017/06/05 22:58:58, picksi wrote: > This DCHECK does my head in trying to understand what the intent is! It looks > like its looking ensuring that the state has changed? Is it possible to make > this more understandable? > > Possibly just a single DCHECK that has a single comparison in it? e.g. > DCHECK(oldStatus != newStatus) [or whatever the intention is!] It's actually the opposite, the check is trying to ensure the old status is equal to the status we're trying to set it to. The reason is, that unless we're creating the channel for the first time, we can't actually update its value, so we shouldn't try. https://codereview.chromium.org/2886433002/diff/250001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/250001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:406: #endif This is getting complicated and I'm not quite happy about the cost of adding it to HostContentSettingsMap in terms of code readability. How about we just remove this whole block for now. It would still be nice to have these checks but they're not all specific to this CL and we can consider how to add them later.
https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:97: } On 2017/06/06 01:08:14, raymes wrote: > Ahh, makes sense. I would probably change this to be the same check as in other > providers: > if (primary_pattern == ContentSettingsPattern::Wildcard() && > secondary_pattern == ContentSettingsPattern::Wildcard() && > resource_identifier.empty()) { > return false; > } Done. https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:98: GURL primary_url = GURL(primary_pattern.ToString()); On 2017/06/06 01:08:13, raymes wrote: > nit: this is now unused Oops, thanks! Done. https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:105: case CONTENT_SETTING_BLOCK: { On 2017/06/05 22:58:58, picksi wrote: > Have these two cases fall through and then later in the code having three > different checks for which 'case' is operating feels a bit nasty. Is it possible > to refactor out a single function that wraps the code that would be duplicated > in both cases? Something like SetContentSetting(bridge, true); in the fist case > and SetContentSetting(bridge, false); in the second. Great suggestion, agree this is clearer and neater now. https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notific... chrome/browser/notifications/notification_channels_provider_android.cc:111: DCHECK(!(setting == CONTENT_SETTING_ALLOW && On 2017/06/06 01:08:13, raymes wrote: > On 2017/06/05 22:58:58, picksi wrote: > > This DCHECK does my head in trying to understand what the intent is! It looks > > like its looking ensuring that the state has changed? Is it possible to make > > this more understandable? > > > > Possibly just a single DCHECK that has a single comparison in it? e.g. > > DCHECK(oldStatus != newStatus) [or whatever the intention is!] > > It's actually the opposite, the check is trying to ensure the old status is > equal to the status we're trying to set it to. The reason is, that unless we're > creating the channel for the first time, we can't actually update its value, so > we shouldn't try. Done. Although I'm slightly concerned that this could be racey - what if the channel status changes in between calling SetWebsiteSetting and getting to this point? Unlikely, but seems wrong to DCHECK something that can change outside of chrome's control at any time. Given we can't guarantee SetWebsiteSetting isn't called twice for the same origin, I'm tempted to remove the DCHECKs entirely and just make it ignore subsequent SetWebsiteSetting updates.. have added a TODO about that for now. https://codereview.chromium.org/2886433002/diff/250001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/2886433002/diff/250001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:406: #endif On 2017/06/06 01:08:14, raymes wrote: > This is getting complicated and I'm not quite happy about the cost of adding it > to HostContentSettingsMap in terms of code readability. How about we just remove > this whole block for now. It would still be nice to have these checks but > they're not all specific to this CL and we can consider how to add them later. I agree there's got to be a nicer way to ensure our assumptions here. And there must be other content settings with their own assumptions - would be awesome if we could come up with a way to ensure all these assumptions are enforced at compile time while keeping things simple. Do you have any thoughts about that longer term? Have removed this stuff for now but filed crbug.com/731126 so we don't forget to put it back in some form. These checks already helped me fix a bug in my code (they caused a test to fail which motivated me to check prod code for the same thing and add the wildcard special case), so I think they are important.
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by awdf@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by awdf@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from miguelg@chromium.org, peter@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2886433002/#ps290001 (title: "Conditionally include new header file")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 290001, "attempt_start_ts": 1497017395696170,
"parent_rev": "b706bcfe02f9b5310e3ca095deb40479ce92eec4", "commit_rev":
"338aa5e24e36752be190da0009bb23e8f913566c"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Adding content settings provider for notification channels - Right now it just attempts to create channels on grant, and delete them on resetting the permission. - Java side is not yet implemented so this is actually a no-op. BUG=700377 ========== to ========== [Android] Adding content settings provider for notification channels - Right now it just attempts to create channels on grant, and delete them on resetting the permission. - Java side is not yet implemented so this is actually a no-op. BUG=700377 Review-Url: https://codereview.chromium.org/2886433002 Cr-Commit-Position: refs/heads/master@{#478267} Committed: https://chromium.googlesource.com/chromium/src/+/338aa5e24e36752be190da0009bb... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as https://chromium.googlesource.com/chromium/src/+/338aa5e24e36752be190da0009bb... |
