Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 2886433002: [Android] Adding content settings provider for notification channels

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week, 1 day ago by awdf
Modified:
4 hours, 45 minutes ago
Reviewers:
Peter Beverloo, raymes
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_
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

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: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+428 lines, -1 line) Patch
M chrome/android/BUILD.gn View 1 2 3 4 5 6 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 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 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 2 chunks +10 lines, -0 lines 0 comments Download
A chrome/browser/notifications/notification_channels_provider_android.h View 1 2 3 4 5 6 1 chunk +72 lines, -0 lines 1 comment Download
A chrome/browser/notifications/notification_channels_provider_android.cc View 1 2 3 4 5 6 1 chunk +129 lines, -0 lines 2 comments Download
A chrome/browser/notifications/notification_channels_provider_android_unittest.cc View 1 2 3 4 5 6 1 chunk +138 lines, -0 lines 3 comments Download
M chrome/common/chrome_features.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/common/chrome_features.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 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 3 chunks +17 lines, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 14 (2 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 ...
1 week, 1 day 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 != ...
1 week, 1 day 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, ...
1 week 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. ...
1 week 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, ...
1 week ago (2017-05-17 03:13:39 UTC) #7
awdf
going to start trying to implement GetRuleIterator next - see my question inline about how ...
6 days, 12 hours 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 ...
6 days, 5 hours 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 ...
4 days, 11 hours 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 ...
2 days, 5 hours 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 ...
15 hours, 40 minutes 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 { ...
4 hours, 59 minutes ago (2017-05-24 00:59:51 UTC) #13
raymes
4 hours, 45 minutes ago (2017-05-24 01:14:07 UTC) #14
lgtm with the above comments applied
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06