Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(564)

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+433 lines, -2 lines) Patch
M chrome/android/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
A chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java View 1 2 3 4 1 chunk +44 lines, -0 lines 0 comments Download
M chrome/android/java_sources.gni View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/content_settings/host_content_settings_map_factory.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +13 lines, -0 lines 0 comments Download
M chrome/browser/engagement/site_engagement_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
A chrome/browser/notifications/notification_channels_provider_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +75 lines, -0 lines 0 comments Download
A chrome/browser/notifications/notification_channels_provider_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +147 lines, -0 lines 0 comments Download
A chrome/browser/notifications/notification_channels_provider_android_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +131 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +4 lines, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 71 (35 generated)
Peter Beverloo
https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java#newcode14 chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: @CalledByNative nit: TODO w/ comment to remove once the ...
3 years, 7 months ago (2017-05-15 15:10:09 UTC) #2
raymes
A few early comments. Thanks :) https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifications/android_settings_provider.cc File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifications/android_settings_provider.cc#newcode57 chrome/browser/notifications/android_settings_provider.cc:57: if (content_type != ...
3 years, 7 months ago (2017-05-16 01:08:18 UTC) #4
awdf
Thanks for your comments both! https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java#newcode14 chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:14: @CalledByNative On 2017/05/15 15:10:08, ...
3 years, 7 months ago (2017-05-16 15:17:36 UTC) #5
Peter Beverloo
Code lg % base::Feature, but I really need Raymes to review the content settings part. ...
3 years, 7 months ago (2017-05-16 16:15:34 UTC) #6
raymes
Thanks! https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifications/android_settings_provider.cc File chrome/browser/notifications/android_settings_provider.cc (right): https://codereview.chromium.org/2886433002/diff/40001/chrome/browser/notifications/android_settings_provider.cc#newcode57 chrome/browser/notifications/android_settings_provider.cc:57: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || On 2017/05/16 15:17:35, ...
3 years, 7 months ago (2017-05-17 03:13:39 UTC) #7
awdf
going to start trying to implement GetRuleIterator next - see my question inline about how ...
3 years, 7 months ago (2017-05-17 17:14:41 UTC) #8
raymes
Getting close now. I'd refrain from adding more code here in this CL so we ...
3 years, 7 months ago (2017-05-18 00:39:37 UTC) #9
awdf
Renamed to NotificationChannelsProviderAndroid Not sure what's best for tests, I don't know how I could ...
3 years, 7 months ago (2017-05-19 17:59:58 UTC) #10
raymes
Thanks Anita. For testing I think mocking out the Java calls is a good idea ...
3 years, 7 months ago (2017-05-22 00:37:42 UTC) #11
awdf
On 2017/05/22 00:37:42, raymes wrote: > Thanks Anita. > > For testing I think mocking ...
3 years, 7 months ago (2017-05-23 14:18:57 UTC) #12
raymes
Thanks! lg with a few minor comments https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode26 chrome/browser/notifications/notification_channels_provider_android.cc:26: namespace { ...
3 years, 7 months ago (2017-05-24 00:59:51 UTC) #13
raymes
lgtm with the above comments applied
3 years, 7 months ago (2017-05-24 01:14:07 UTC) #14
Peter Beverloo
lgtm % nits https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.h File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.h#newcode45 chrome/browser/notifications/notification_channels_provider_android.h:45: std::unique_ptr<NotificationChannelsBridge> bridge); Consider making this private ...
3 years, 7 months ago (2017-05-24 10:57:43 UTC) #15
awdf
https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode26 chrome/browser/notifications/notification_channels_provider_android.cc:26: namespace { On 2017/05/24 00:59:50, raymes wrote: > nit: ...
3 years, 6 months ago (2017-06-01 15:11:22 UTC) #16
Peter Beverloo
lgtm https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/120001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc#newcode24 chrome/browser/notifications/notification_channels_provider_android_unittest.cc:24: public: On 2017/06/01 15:11:22, awdf wrote: > On ...
3 years, 6 months ago (2017-06-01 15:26:12 UTC) #19
awdf
https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc#newcode43 chrome/browser/notifications/notification_channels_provider_android_unittest.cc:43: void InitChannelsProvider(bool shouldUseChannels) { On 2017/06/01 15:26:11, Peter Beverloo ...
3 years, 6 months ago (2017-06-01 16:04:29 UTC) #20
Peter Beverloo
Note that there's a compile failure: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.android%2Fandroid_clang_dbg_recipe%2F281512%2F%2B%2Frecipes%2Fsteps%2Fcompile__with_patch_%2F0%2Fstdout https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2886433002/diff/140001/chrome/browser/notifications/notification_channels_provider_android_unittest.cc#newcode46 chrome/browser/notifications/notification_channels_provider_android_unittest.cc:46: channels_provider_ = ...
3 years, 6 months ago (2017-06-01 16:07:47 UTC) #21
awdf
On 2017/06/01 16:07:47, Peter Beverloo wrote: > Note that there's a compile failure: > > ...
3 years, 6 months ago (2017-06-01 16:10:06 UTC) #22
awdf
On 2017/05/24 01:14:07, raymes wrote: > lgtm with the above comments applied Hi Raymes, I ...
3 years, 6 months ago (2017-06-02 10:54:54 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2886433002/180001
3 years, 6 months ago (2017-06-02 10:55:32 UTC) #26
commit-bot: I haz the power
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_presubmit/builds/454280)
3 years, 6 months ago (2017-06-02 11:03:34 UTC) #28
awdf
+miguelg for OWNERS review of chrome/android/BUILD.gn
3 years, 6 months ago (2017-06-02 14:24:11 UTC) #31
Miguel Garcia
lgtm for chrome/android/BUILD.gn
3 years, 6 months ago (2017-06-02 14:32:27 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2886433002/180001
3 years, 6 months ago (2017-06-02 14:38:32 UTC) #35
commit-bot: I haz the power
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_android_rel_ng/builds/309302)
3 years, 6 months ago (2017-06-02 15:48:47 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2886433002/180001
3 years, 6 months ago (2017-06-02 16:58:13 UTC) #39
commit-bot: I haz the power
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_swarming_rel/builds/192236)
3 years, 6 months ago (2017-06-02 18:03:48 UTC) #41
raymes
https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode96 chrome/browser/notifications/notification_channels_provider_android.cc:96: const std::string origin = primary_url.spec(); Ah thanks for explaining, ...
3 years, 6 months ago (2017-06-04 23:10:51 UTC) #42
awdf
peter@ & raymes@ - PTAL at updated DCHECK in host_content_settings_map.cc and new early return statement ...
3 years, 6 months ago (2017-06-05 14:57:15 UTC) #43
Peter Beverloo
Still lgtm. Is it worth adding a test? https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/220001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode95 chrome/browser/notifications/notification_channels_provider_android.cc:95: // ...
3 years, 6 months ago (2017-06-05 15:11:31 UTC) #46
awdf
had to fix up a test too that was setting a notification CS to ASK ...
3 years, 6 months ago (2017-06-05 17:06:57 UTC) #49
picksi
Two comments from me, do something about it if you agree. I won't be upset ...
3 years, 6 months ago (2017-06-05 22:58:58 UTC) #55
raymes
lgtm https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/180001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode96 chrome/browser/notifications/notification_channels_provider_android.cc:96: const std::string origin = primary_url.spec(); On 2017/06/05 14:57:15, ...
3 years, 6 months ago (2017-06-06 01:08:14 UTC) #56
awdf
https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notifications/notification_channels_provider_android.cc File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2886433002/diff/250001/chrome/browser/notifications/notification_channels_provider_android.cc#newcode97 chrome/browser/notifications/notification_channels_provider_android.cc:97: } On 2017/06/06 01:08:14, raymes wrote: > Ahh, makes ...
3 years, 6 months ago (2017-06-08 14:46:12 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2886433002/290001
3 years, 6 months ago (2017-06-09 14:10:05 UTC) #68
commit-bot: I haz the power
3 years, 6 months ago (2017-06-09 14:14:29 UTC) #71
Message was sent while issue was closed.
Committed patchset #16 (id:290001) as
https://chromium.googlesource.com/chromium/src/+/338aa5e24e36752be190da0009bb...

Powered by Google App Engine
This is Rietveld 408576698