Android O notification permissions not counted as site settings that will be cleared |
||
Issue descriptionChrome Version: 62 OS: Android O What steps will reproduce the problem? (0) Install fresh version of Chrome on Android O (1) Grant a site notification permission eg tests.peter.sh/notification-generator/ & verify it has permission in Settings > Site settings > Notifications. (2) Go to Advanced Clear Browsing Data settings (Settings > Privacy > Clear browsing data > Advanced), note number of site settings that will be cleared. (3) Tick only 'Site settings' and Clear Data. Note the site no longer has permission in Settings > Site settings > Notifications. What is the expected result? In step 2, expect the number of settings to be cleared to be > 0 What happens instead? In step 2, it says the number of settings to be cleared is 'None'. --- I believe fixing this is just a matter of modifying SiteSettingsCounter::Count at https://cs.chromium.org/chromium/src/components/browsing_data/content/counters/site_settings_counter.cc?sq=package:chromium&l=38 to take prefs with source "notification_android" into account, not those with source "preference" (since the notification android provider already implements GetWebsiteSettingLastModified, this should JustWork). dullweber@ can you confirm?
,
Sep 6 2017
Yep just tried it and it works. I'm now wondering if that condition line is necessary at all now, given that the host content settings map is already only querying 'user modifiable' providers within GetWebsiteSettingLastModified, and the pref provider and the notification channels provider are the only user modifiable providers that are registered. Even before my change to generalise the concept of 'user modifiable providers'*, it was only querying the pref provider, so I'm not sure why that source == "preference" check was in there in the first place; do you know Christian? *https://chromium-review.googlesource.com/c/chromium/src/+/577867/7/components/content_settings/core/browser/host_content_settings_map.cc
,
Sep 6 2017
There are policy settings that should not be counted because they wont get deleted. I believe map_->GetSettingsForOneType will return these as well. They wouldn't get a time stamp but you can delete settings for "All time", which sets period_start = base::Time().
,
Sep 6 2017
Ah I see, that makes sense, and explains why the count went up to 19 when I tried removing the condition just now! Okay, so possibly a nicer / more efficient solution would be to add some new method to HCSM, map_->GetUserModifiableSettingsForOneType that only queries the user modifiable providers, and then we wouldn't need the condition, but for now I think I'll just go with the quick fix in #1 (% reviewer opinions). thanks!
,
Sep 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fdd3055f7ff31a304428c5194871eca12a14a950 commit fdd3055f7ff31a304428c5194871eca12a14a950 Author: Anita Woodruff <awdf@chromium.org> Date: Wed Sep 06 18:13:24 2017 [Android O] Include notification settings in site settings count - Previously only content settings provided by the PrefProvider were counted when calculating the number of sites that would be affected by clearing site settings. - This change means we also count settings provided by the NotificationChannelsAndroidProvider, because these settings will also be cleared. Bug: 762515 Change-Id: Ibc4265f7553a4ee6d79375fc292d5382eaa8376d Reviewed-on: https://chromium-review.googlesource.com/652807 Commit-Queue: Anita Woodruff <awdf@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Cr-Commit-Position: refs/heads/master@{#500010} [modify] https://crrev.com/fdd3055f7ff31a304428c5194871eca12a14a950/components/browsing_data/content/counters/site_settings_counter.cc
,
Sep 7 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by dullweber@chromium.org
, Sep 6 2017