Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922473003:
[Android] Implement GetRuleIterator for channels provider (Closed)
Description[Android] Implement GetRuleIterator for channels provider
- Now the Provider retrieves all the site notification channels and
uses their statuses (ENABLED/BLOCKED) to return a RuleIterator over
notification permissions.
- Note that the Java side still needs to be implemented, right now
it just returns an empty array.
Bug: 700377
Review-Url: https://codereview.chromium.org/2922473003
Cr-Commit-Position: refs/heads/master@{#478279}
Committed: https://chromium.googlesource.com/chromium/src/+/1af5bbb0e6f4c1ccc79fac777345d30e9e1c8061
Patch Set 1 #
Total comments: 16
Patch Set 2 : Replying to comments #
Total comments: 23
Patch Set 3 : Responding to more comments #Patch Set 4 : rebase and added a comment about incognito #Patch Set 5 : rebase #
Depends on Patchset: Messages
Total messages: 21 (10 generated)
Description was changed from ========== [Android] Implement GetRuleIterator for channels provider - Now the Provider retrieves all the site notification channels and uses their statuses (ENABLED/BLOCKED) to return a RuleIterator over notification permissions. - Note that the Java side still needs to be implemented, right now it just returns an empty array. Bug:700377 ========== to ========== [Android] Implement GetRuleIterator for channels provider - Now the Provider retrieves all the site notification channels and uses their statuses (ENABLED/BLOCKED) to return a RuleIterator over notification permissions. - Note that the Java side still needs to be implemented, right now it just returns an empty array. Bug:700377 ==========
awdf@chromium.org changed reviewers: + peter@chromium.org, raymes@chromium.org
Cool! https://codereview.chromium.org/2922473003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2922473003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:66: private int getStatus() { Should this (and `int status` in the member/constructor) be marked as @NotificationChannelStatus https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:72: Java_SiteChannel_getStatus(env, jchannel))}; Channel channel; channel.origin = ConvertJ.... channel.status = static_cast<... Alternatively, and maybe cleaner, you could give the Channel struct a constructor that takes both. Then lines 69 - 73 could be replaced by: channels.emplace_back( ConvertJavaStringToUTF8(Java_SiteChannel_getOrigin(env, jchannel)), static_cast<NotificationChannelStatus>( Java_SiteChannel_getStatus(env, jchannel))); That constructs a new Channel object in its actual memory location in the channels vector. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:97: std::list<content_settings::Rule> rules_; Since an instance of the Rule object will be returned, what you could do is change this class to just store the std::vector<Channel> you create above, and the index of where you are in the vector. Next() would then look something like: content_settings::Rule Next() override { DCHECK(HasNext()); return content_settings::Rule( ContentSettingsPattern::FromString(channels_[index_].origin), ContentSettingsPattern(), new base::Value(ChannelStatusToContentSetting(channels_[index_++].status))); } https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:142: DCHECK(channel.status != NotificationChannelStatus::UNAVAILABLE); nit: DCHECK_NE https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:149: new ListIterator(rules)); nit (the compiler can figure out typing): return base::MakeUnique<ListIterator>(std::move(channels)); https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.h:26: NotificationChannelStatus status; nit: give a default value https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.h:29: } // anonymous namespace Sorry that I missed this earlier - we shouldn't be using an anonymous namespace in a header file, is doesn't mean anything. If you qualify Channel as NotificationChannel we can just have them on the global scope. Alternatively you can add them as members of NotificationChannelsProviderAndroid. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:166: {.origin = kTestOrigin, .status = NotificationChannelStatus::BLOCKED}); style nit: this is C-ish struct syntax that we don't really use. More idiomatic would be: channels.emplace_back(kTestOrigin, NotificationChannelStatus::BLOCKED); (If you were to give it a constructor.)
https://codereview.chromium.org/2922473003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java (right): https://codereview.chromium.org/2922473003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java:66: private int getStatus() { On 2017/06/02 15:14:26, Peter Beverloo wrote: > Should this (and `int status` in the member/constructor) be marked as > @NotificationChannelStatus Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:72: Java_SiteChannel_getStatus(env, jchannel))}; On 2017/06/02 15:14:26, Peter Beverloo wrote: > Channel channel; > channel.origin = ConvertJ.... > channel.status = static_cast<... > > Alternatively, and maybe cleaner, you could give the Channel struct a > constructor that takes both. Then lines 69 - 73 could be replaced by: > > channels.emplace_back( > ConvertJavaStringToUTF8(Java_SiteChannel_getOrigin(env, jchannel)), > static_cast<NotificationChannelStatus>( > Java_SiteChannel_getStatus(env, jchannel))); > > That constructs a new Channel object in its actual memory location in the > channels vector. Done (with a constructor). https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:97: std::list<content_settings::Rule> rules_; On 2017/06/02 15:14:26, Peter Beverloo wrote: > Since an instance of the Rule object will be returned, what you could do is > change this class to just store the std::vector<Channel> you create above, and > the index of where you are in the vector. > > Next() would then look something like: > > content_settings::Rule Next() override { > DCHECK(HasNext()); > return content_settings::Rule( > ContentSettingsPattern::FromString(channels_[index_].origin), > ContentSettingsPattern(), > new > base::Value(ChannelStatusToContentSetting(channels_[index_++].status))); > } Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:142: DCHECK(channel.status != NotificationChannelStatus::UNAVAILABLE); On 2017/06/02 15:14:26, Peter Beverloo wrote: > nit: DCHECK_NE Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.cc:149: new ListIterator(rules)); On 2017/06/02 15:14:26, Peter Beverloo wrote: > nit (the compiler can figure out typing): > > return base::MakeUnique<ListIterator>(std::move(channels)); Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.h:26: NotificationChannelStatus status; On 2017/06/02 15:14:26, Peter Beverloo wrote: > nit: give a default value Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android.h:29: } // anonymous namespace On 2017/06/02 15:14:26, Peter Beverloo wrote: > Sorry that I missed this earlier - we shouldn't be using an anonymous namespace > in a header file, is doesn't mean anything. If you qualify Channel as > NotificationChannel we can just have them on the global scope. Alternatively you > can add them as members of NotificationChannelsProviderAndroid. Done. https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/1/chrome/browser/notification... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:166: {.origin = kTestOrigin, .status = NotificationChannelStatus::BLOCKED}); On 2017/06/02 15:14:26, Peter Beverloo wrote: > style nit: this is C-ish struct syntax that we don't really use. More idiomatic > would be: > > channels.emplace_back(kTestOrigin, NotificationChannelStatus::BLOCKED); > > (If you were to give it a constructor.) Done.
lgtm https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:85: return CONTENT_SETTING_DEFAULT; Arguably UNAVAILABLE doesn't really map to a content setting. I would suggest adding NOTREACHED here too to help aid in this not being misused. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:106: ContentSettingsPattern(), I think returning ContentSettingsPattern::Wildcard here might be more in-line with what other providers would do. An empty pattern is invalid and won't match anything. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.h:26: : origin_(origin), status_(status) {} nit: methods always come before member variables https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:137: GetRuleIteratorWhenChannelsShouldNotBeUsed_ReturnsNull) { nit: I think the _ReturnsNull in the name isn't necessary (I think peter@ may have had a comment on this previously?) https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: InitChannelsProvider(false /* should_use_channels */); nit: apparently (according to thakis@) the suggested syntax for commenting bools is: InitChannelsProvider(/*should_use_channels=*/false); https://google.github.io/styleguide/cppguide.html https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:172: EXPECT_EQ(rule.primary_pattern, nit (here and below): The expected value always comes first with EXPECT_EQ
lgtm https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:96: ~ChannelsRuleIterator() override {} micro nit: {} -> = default; https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:116: }; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || nit: please document what this means We don't enable notifications in incognito today, but there's no guarantee that we won't ever. However, what we definitely won't want is for Incognito to spill over to system UI, so this case should be considered orthogonal from the current restrictions. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: InitChannelsProvider(false /* should_use_channels */); On 2017/06/04 23:51:21, raymes wrote: > nit: apparently (according to thakis@) the suggested syntax for commenting bools > is: > > InitChannelsProvider(/*should_use_channels=*/false); > > https://google.github.io/styleguide/cppguide.html 3200 vs. 60 hits. I prefer consistency -- all our current code uses the suffix.
https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:85: return CONTENT_SETTING_DEFAULT; On 2017/06/04 23:51:21, raymes wrote: > Arguably UNAVAILABLE doesn't really map to a content setting. I would suggest > adding NOTREACHED here too to help aid in this not being misused. Done. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:106: ContentSettingsPattern(), On 2017/06/04 23:51:21, raymes wrote: > I think returning ContentSettingsPattern::Wildcard here might be more in-line > with what other providers would do. An empty pattern is invalid and won't match > anything. Done. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:116: }; On 2017/06/05 12:40:24, Peter Beverloo wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || On 2017/06/05 12:40:24, Peter Beverloo wrote: > nit: please document what this means > > We don't enable notifications in incognito today, but there's no guarantee that > we won't ever. However, what we definitely won't want is for Incognito to spill > over to system UI, so this case should be considered orthogonal from the current > restrictions. Right. But I have two questions: 1. Since this is in GetRuleIterator, where we are checking what permissions exist, if one day we did allow notifications in incognito mode, wouldn't we allow them from sites that already have permission in normal mode, in which case reading those settings here is perfectly reasonable? Or do you envisage a system where permission would have to be granted in both incognito and normal mode? Either way I agree we want to make sure channels are definitely not *created* from incognito mode, but right now there's no channel creation within this method. 2. To make sure the current rules don't get applied in incognito mode right now, should I actually be explicitly returning a global BLOCK content setting if incognito=true? According to the documentation of this interface method, "If |incognito| is true, the iterator returns only the content settings which are applicable to the incognito mode and differ from the normal mode." ..Which sounds a bit like it's saying that if no rules are returned when incognito is true, the rules returned when incognito is false should apply. - OTOH this doesn't seem like the correct place to be defining that rule and I'm assuming there's some other place where 'if (incognito) then permission denied!' is applied. So maybe I'm misinterpreting the documentation? https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.h (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.h:26: : origin_(origin), status_(status) {} On 2017/06/04 23:51:21, raymes wrote: > nit: methods always come before member variables Done. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:137: GetRuleIteratorWhenChannelsShouldNotBeUsed_ReturnsNull) { On 2017/06/04 23:51:22, raymes wrote: > nit: I think the _ReturnsNull in the name isn't necessary (I think peter@ may > have had a comment on this previously?) Done. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: InitChannelsProvider(false /* should_use_channels */); On 2017/06/05 12:40:24, Peter Beverloo wrote: > On 2017/06/04 23:51:21, raymes wrote: > > nit: apparently (according to thakis@) the suggested syntax for commenting > bools > > is: > > > > InitChannelsProvider(/*should_use_channels=*/false); > > > > https://google.github.io/styleguide/cppguide.html > > 3200 vs. 60 hits. I prefer consistency -- all our current code uses the suffix. Interesting - I agree with Peter that consistency justifies leaving it for now, but in an ideal world the style guide should dictate style, so I'll remember that syntax for future, thanks for bringing it to my attention. Aside: I note the style guide also says "Consider changing the function signature to replace a bool argument with an enum argument. This will make the argument values self-describing" - which is a great point, so if I were to change it I think actually I would go with something like SystemChannelSupport::kSupported/kNotSupported https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:172: EXPECT_EQ(rule.primary_pattern, On 2017/06/04 23:51:22, raymes wrote: > nit (here and below): The expected value always comes first with EXPECT_EQ Done.
https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || On 2017/06/05 16:29:26, awdf wrote: > On 2017/06/05 12:40:24, Peter Beverloo wrote: > > nit: please document what this means > > > > We don't enable notifications in incognito today, but there's no guarantee > that > > we won't ever. However, what we definitely won't want is for Incognito to > spill > > over to system UI, so this case should be considered orthogonal from the > current > > restrictions. > > Right. But I have two questions: > 1. Since this is in GetRuleIterator, where we are checking what permissions > exist, if one day we did allow notifications in incognito mode, wouldn't we > allow them from sites that already have permission in normal mode, in which case > reading those settings here is perfectly reasonable? Or do you envisage a system > where permission would have to be granted in both incognito and normal mode? > Either way I agree we want to make sure channels are definitely not *created* > from incognito mode, but right now there's no channel creation within this > method. To be frank: I don't know whether we'd inherit the notification permission in Incognito profiles. It automatically means that sites can create push subscriptions too, which can be surprising. Something for the very far future. In what scenario would GetRuleIterator() actually create channels? > 2. To make sure the current rules don't get applied in incognito mode right now, > should I actually be explicitly returning a global BLOCK content setting if > incognito=true? According to the documentation of this interface method, "If > |incognito| is true, the iterator returns only the content settings which are > applicable to the incognito mode and differ from the normal mode." ..Which > sounds a bit like it's saying that if no rules are returned when incognito is > true, the rules returned when incognito is false should apply. - OTOH this > doesn't seem like the correct place to be defining that rule and I'm assuming > there's some other place where 'if (incognito) then permission denied!' is > applied. So maybe I'm misinterpreting the documentation? Returning a wildcard block here would definitely surprise me. The incognito checks are in place elsewhere and are tested thoroughly. Raymes can comment on the documentation I'm sure.
https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || Re: > In what scenario would GetRuleIterator() actually create channels? Sorry, think I was getting mixed up there, channels should only ever get created via SetWebsiteSetting.
lgtm but please see the note about SetWebsiteSetting in particular https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || On 2017/06/05 17:33:36, awdf wrote: > Re: > > In what scenario would GetRuleIterator() actually create channels? > > Sorry, think I was getting mixed up there, channels should only ever get created > via SetWebsiteSetting. If incognito is true, this should just return settings that are different to settings that apply to non-incognito. So returning nullptr is the right thing to do here (I think!). The normal pref provider deals with incognito settings in a separate in-memory map. e.g. consider someone visiting a site in incognito and enabling geolocation. We want that setting to apply but we don't want it to be written. So it gets stored in-memory and only returned for that incognito session. Technically we need to be careful in SetWebsiteSetting to make sure that we don't write anything to the android channels when in incognito. Right now though, as you point out, we don't grant notifications in incognito so it's not a big deal. But if we did enable that at some point, SetWebsiteSetting would end writing the incognito setting to disk. I'll leave it up to you/peter whether you want to plumb through the incognito status in the constructor and check in SetWebsiteSetting, but it may be worth adding a note for later. https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android_unittest.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android_unittest.cc:138: InitChannelsProvider(false /* should_use_channels */); On 2017/06/05 12:40:24, Peter Beverloo wrote: > On 2017/06/04 23:51:21, raymes wrote: > > nit: apparently (according to thakis@) the suggested syntax for commenting > bools > > is: > > > > InitChannelsProvider(/*should_use_channels=*/false); > > > > https://google.github.io/styleguide/cppguide.html > > 3200 vs. 60 hits. I prefer consistency -- all our current code uses the suffix. Yeah, there's probably an argument to be had about the issue which I don't really want to be involved in :) I do think it's unfortunate to have 2 ways of doing this going forward though.
https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/notification_channels_provider_android.cc (right): https://codereview.chromium.org/2922473003/diff/20001/chrome/browser/notifica... chrome/browser/notifications/notification_channels_provider_android.cc:137: if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito || On 2017/06/06 01:39:04, raymes wrote: > On 2017/06/05 17:33:36, awdf wrote: > > Re: > > > In what scenario would GetRuleIterator() actually create channels? > > > > Sorry, think I was getting mixed up there, channels should only ever get > created > > via SetWebsiteSetting. > > If incognito is true, this should just return settings that are different to > settings that apply to non-incognito. So returning nullptr is the right thing to > do here (I think!). > > The normal pref provider deals with incognito settings in a separate in-memory > map. e.g. consider someone visiting a site in incognito and enabling > geolocation. We want that setting to apply but we don't want it to be written. > So it gets stored in-memory and only returned for that incognito session. > > Technically we need to be careful in SetWebsiteSetting to make sure that we > don't write anything to the android channels when in incognito. Right now > though, as you point out, we don't grant notifications in incognito so it's not > a big deal. But if we did enable that at some point, SetWebsiteSetting would end > writing the incognito setting to disk. I'll leave it up to you/peter whether you > want to plumb through the incognito status in the constructor and check in > SetWebsiteSetting, but it may be worth adding a note for later. Thanks. Added a TODO for now, will discuss with Peter whether it's necessary when he's back. My instinct is that having another sanity check here just before writing to system settings for incognito wouldn't hurt.
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
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2922473003/#ps80001 (title: "rebase")
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": 80001, "attempt_start_ts": 1497018425591230,
"parent_rev": "dded0412e190a6f4857d10d7f9d865c4ab700f56", "commit_rev":
"1af5bbb0e6f4c1ccc79fac777345d30e9e1c8061"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Implement GetRuleIterator for channels provider - Now the Provider retrieves all the site notification channels and uses their statuses (ENABLED/BLOCKED) to return a RuleIterator over notification permissions. - Note that the Java side still needs to be implemented, right now it just returns an empty array. Bug:700377 ========== to ========== [Android] Implement GetRuleIterator for channels provider - Now the Provider retrieves all the site notification channels and uses their statuses (ENABLED/BLOCKED) to return a RuleIterator over notification permissions. - Note that the Java side still needs to be implemented, right now it just returns an empty array. Bug: 700377 Review-Url: https://codereview.chromium.org/2922473003 Cr-Commit-Position: refs/heads/master@{#478279} Committed: https://chromium.googlesource.com/chromium/src/+/1af5bbb0e6f4c1ccc79fac777345... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/1af5bbb0e6f4c1ccc79fac777345... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
