[ApproximateLocation] Add approximate location setting into site settings page. [chromium/src : main]
Yifan Luo (Gerrit)
Yifan Luo added 1 comment![]( "Open in Gerrit")
Related details
- Antonio Sartori
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 12 comments![]( "Open in Gerrit")
const FeatureEntry::FeatureParam kApproximateGeolocationPermissionArm1Params[] =This is for the 6 variants of the prompt, which won't exist on desktop. Can we just have an on-off chrome://flag on desktop?
<select id="geolocationPermission" class="md-select"
hidden$="[[!isApproximateGeolocationPermission()]]"
aria-label$="[[label]]"
aria-describedby="permissionSecondary"
on-change="onGeolocationPermissionSelectionChange_"
disabled$="[[!isPermissionUserControlled_(site.source, category,
site.setting, systemPermissionWarningKey_)]]">
<option id="default" value="ask">
$i18n{siteSettingsActionAskDefault}
</option>
<option id="approximate"
value="approximate">
$i18n{siteSettingsActionAllowApproximate}
</option>
<option id="precise" value="precise">
$i18n{siteSettingsActionAllowPrecise}
</option>
<option id="block" value="block">
$i18n{siteSettingsActionBlock}
</option>
</select>I don't have a strong opinion here, but this kind of duplicates the permission select below. What we seem to do in general is to have the <select id="permission"> below and control what it shows and what it doesn't show with the various funcionts `showAllowedSetting_` and `showAskSetting_` etc.
Maybe we should do the same? Add an option for approximate below and reuse allow for precise while changing its label?
if (content_type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS) {Can you create a bug and add a TODO here that we should take care of this?
CHECK(content_settings::ContentSettingFromString(Can't we just rename this `PermissionSettingFromString`, have it take a `PermissionSetting` and implement the logic inside the function depending on whether it gets a ContentSetting or a GeolocaitonSetting using `std::visit` (like here https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/geolocation_header_service.cc;l=193;drc=c8b8e953163e6d035fc4e1bc4b32ec94c458dba8)
if ((content_type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&
value == "block") ||
(content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&
setting != PermissionSetting{CONTENT_SETTING_BLOCK})) {The way to do this easily is e.g.
`PermissionSettingsRegistry::GetInstance()->Get(content_type)->delegate().IsBlocked(setting)`
which works for all permissions.
if (content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&just a note that this shouldn't be necessary anymore after https://crrev.com/c/7796856 lands
if (content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS) {can you just check here if this is NOTIFICATIONS and remove that check from MaybeLogSafeBrowsing...?
Otherwise pass a PermissionSetting to MaybeLog... and remove the check here.
bool mute = (setting == PermissionSetting{CONTENT_SETTING_BLOCK}) ||let's just define `ContentSetting content_setting = std::get<ContentSetting>(setting)` here.
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
base_types->push_back(ContentSettingsType::GEOLOCATION_WITH_OPTIONS);
} else {
base_types->push_back(ContentSettingsType::GEOLOCATION);
}`base_types->push_back(content_settings::GeolocationContentSettingsType())`
permissions::PermissionUtil::PermissionSettingToPermissionStatus(setting),we shouldn't have this function because there is no proper way of converting a PermissionSetting into a PermissionStatus. The information of the granularity granted is lost.
To compare, here is the analogous function for android: https://source.chromium.org/chromium/chromium/src/+/main:components/browser_ui/site_settings/android/website_preference_bridge.cc;l=270;drc=a2de7835eb6515901e1165988ff1b1a918581785
Seems much easier. I have to understand why this is so complicated here and how we can get around it.
ContentSettingsTypes.GEOLOCATION_WITH_OPTIONS) {let's add a version of this test for GEOLOCATION_WITH_OPTIONS
Related details
- Yifan Luo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yifan Luo (Gerrit)
Yifan Luo added 8 comments![]( "Open in Gerrit")
const FeatureEntry::FeatureParam kApproximateGeolocationPermissionArm1Params[] =This is for the 6 variants of the prompt, which won't exist on desktop. Can we just have an on-off chrome://flag on desktop?
Split it into a saperate cl: https://chromium-review.git.corp.google.com/c/chromium/src/+/7806824
if (content_type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS) {Can you create a bug and add a TODO here that we should take care of this?
Done
Can't we just rename this `PermissionSettingFromString`, have it take a `PermissionSetting` and implement the logic inside the function depending on whether it gets a ContentSetting or a GeolocaitonSetting using `std::visit` (like here https://source.chromium.org/chromium/chromium/src/+/main:components/omnibox/browser/geolocation_header_service.cc;l=193;drc=c8b8e953163e6d035fc4e1bc4b32ec94c458dba8)
Done
if ((content_type == ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&
value == "block") ||
(content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&
setting != PermissionSetting{CONTENT_SETTING_BLOCK})) {The way to do this easily is e.g.
`PermissionSettingsRegistry::GetInstance()->Get(content_type)->delegate().IsBlocked(setting)`
which works for all permissions.
Done
if (content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&just a note that this shouldn't be necessary anymore after https://crrev.com/c/7796856 lands
It looks like content_settings::ContentSettingToValue() can only take ContentSetting, if we don't check it ahead of time, it might cost error. Is there an existing function that we can convert PermissionSetting to value instead?
if (content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS) {can you just check here if this is NOTIFICATIONS and remove that check from MaybeLogSafeBrowsing...?
Otherwise pass a PermissionSetting to MaybeLog... and remove the check here.
Done
bool mute = (setting == PermissionSetting{CONTENT_SETTING_BLOCK}) ||let's just define `ContentSetting content_setting = std::get<ContentSetting>(setting)` here.
Done
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
base_types->push_back(ContentSettingsType::GEOLOCATION_WITH_OPTIONS);
} else {
base_types->push_back(ContentSettingsType::GEOLOCATION);
}Yifan Luo`base_types->push_back(content_settings::GeolocationContentSettingsType())`
Done
Related details
- Antonio Sartori
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
if (content_type != ContentSettingsType::GEOLOCATION_WITH_OPTIONS &&Yifan Luojust a note that this shouldn't be necessary anymore after https://crrev.com/c/7796856 lands
It looks like content_settings::ContentSettingToValue() can only take ContentSetting, if we don't check it ahead of time, it might cost error. Is there an existing function that we can convert PermissionSetting to value instead?
- Yifan Luo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |