[Geolocation] Parameterize GeolocationBrowserTest [chromium/src : main]
0 views
Skip to first unread message
Antonio Sartori (Gerrit)
Jul 9, 2026, 7:02:03 AM (yesterday) Jul 9
to Tom Van Goethem, Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Tom Van Goethem
Antonio Sartori added 1 comment![]( "Open in Gerrit")
File chrome/browser/geolocation/geolocation_browsertest.cc
Line 152, Patchset 2 (Latest):
public testing::WithParamInterface<bool> {Antonio Sartori . unresolved
You can use `WithFeatureOverride` instead, cf. https://source.chromium.org/chromium/chromium/src/+/main:base/test/with_feature_override.h;l=18;drc=0c0af43ed4dc6e5adda6fbd4d8cf84596520900a
Related details
Attention is currently required from:
- Alvin Ji
- Tom Van Goethem
Submit Requirements:
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. |
Tom Van Goethem (Gerrit)
Jul 9, 2026, 9:44:02 AM (yesterday) Jul 9
to Antonio Sartori, Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Antonio Sartori
Tom Van Goethem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Van Goethem
| Commit-Queue | +1 |
1 comment
File chrome/browser/geolocation/geolocation_browsertest.cc
You can use `WithFeatureOverride` instead, cf. https://source.chromium.org/chromium/chromium/src/+/main:base/test/with_feature_override.h;l=18;drc=0c0af43ed4dc6e5adda6fbd4d8cf84596520900a
Attention is currently required from:
- Alvin Ji
- Antonio Sartori
Submit Requirements:
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)
Jul 9, 2026, 9:51:03 AM (yesterday) Jul 9
to Tom Van Goethem, Alvin Ji, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Alvin Ji and Tom Van Goethem
Antonio Sartori added 2 comments![]( "Open in Gerrit")
File chrome/browser/geolocation/geolocation_browsertest.cc
Line 373, Patchset 3 (Latest):
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission) &&Antonio Sartori . unresolved
you can also use `IsParamFeatureEnabled()`, as you prefer
Line 444, Patchset 3 (Latest):
PermissionSetting GeolocationBrowserTest::ExpectedAllowPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kAllowed,
PermissionOption::kAllowed};
}
return CONTENT_SETTING_ALLOW;
}
PermissionSetting GeolocationBrowserTest::ExpectedBlockPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kDenied,
PermissionOption::kDenied};
}
return CONTENT_SETTING_BLOCK;
}
PermissionSetting GeolocationBrowserTest::ExpectedAskPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kAsk, PermissionOption::kAsk};
}
return CONTENT_SETTING_ASK;
}Antonio Sartori . unresolved
you should be able to simplify this by using `PermissionSettingsRegistry::GetInstance()->Get(content_settings::GeolocationContentSettingsType())->ToPermissionSetting(content_setting)`
Related details
Attention is currently required from:
- Alvin Ji
- Tom Van Goethem
Submit Requirements:
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. |
Alvin Ji (Gerrit)
Jul 9, 2026, 5:21:44 PM (18 hours ago) Jul 9
to Tom Van Goethem, Antonio Sartori, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Tom Van Goethem
Alvin Ji voted and added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Tom Van Goethem
Submit Requirements:
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. |
Tom Van Goethem (Gerrit)
Jul 9, 2026, 6:36:30 PM (16 hours ago) Jul 9
to Alvin Ji, Antonio Sartori, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Antonio Sartori
Tom Van Goethem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tom Van Goethem
| Commit-Queue | +1 |
2 comments
File chrome/browser/geolocation/geolocation_browsertest.cc
Line 373, Patchset 3:
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission) &&Antonio Sartori . resolved
you can also use `IsParamFeatureEnabled()`, as you prefer
Tom Van Goethem
Done
PermissionSetting GeolocationBrowserTest::ExpectedAllowPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kAllowed,
PermissionOption::kAllowed};
}
return CONTENT_SETTING_ALLOW;
}
PermissionSetting GeolocationBrowserTest::ExpectedBlockPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kDenied,
PermissionOption::kDenied};
}
return CONTENT_SETTING_BLOCK;
}
PermissionSetting GeolocationBrowserTest::ExpectedAskPermissionSetting() {
if (base::FeatureList::IsEnabled(
content_settings::features::kApproximateGeolocationPermission)) {
return GeolocationSetting{PermissionOption::kAsk, PermissionOption::kAsk};
}
return CONTENT_SETTING_ASK;
}Antonio Sartori . resolved
you should be able to simplify this by using `PermissionSettingsRegistry::GetInstance()->Get(content_settings::GeolocationContentSettingsType())->ToPermissionSetting(content_setting)`
Attention is currently required from:
- Antonio Sartori
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Van Goethem (Gerrit)
5:44 AM (5 hours ago) 5:44 AM
to Alvin Ji, Antonio Sartori, Chromium LUCI CQ, chromium...@chromium.org, mattreyno...@chromium.org
Attention needed from Antonio Sartori
Tom Van Goethem voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Antonio Sartori
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
5:47 AM (5 hours ago) 5:47 AM
to Tom Van Goethem, Alvin Ji, Antonio Sartori, chromium...@chromium.org, mattreyno...@chromium.org
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/geolocation/geolocation_browsertest.cc
Insertions: 30, Deletions: 40.
@@ -27,6 +27,8 @@
#include "chrome/test/base/ui_test_utils.h"
#include "components/content_settings/browser/page_specific_content_settings.h"
#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/browser/permission_settings_info.h"
+#include "components/content_settings/core/browser/permission_settings_registry.h"
#include "components/content_settings/core/common/content_settings.h"
#include "components/content_settings/core/common/content_settings_utils.h"
#include "components/content_settings/core/common/features.h"
@@ -216,9 +218,7 @@
PermissionSetting GetGeolocationContentSetting(const GURL& url);
void SetGeolocationContentSetting(const GURL& url, PermissionSetting setting);
- PermissionSetting ExpectedAllowPermissionSetting();
- PermissionSetting ExpectedBlockPermissionSetting();
- PermissionSetting ExpectedAskPermissionSetting();
+ PermissionSetting ExpectedPermissionSetting(ContentSetting setting);
// Calls watchPosition in JavaScript and accepts or denies the resulting
// permission request. Returns |true| if the expected behavior happened.
@@ -370,8 +370,7 @@
auto* manager =
permissions::PermissionRequestManager::FromWebContents(web_contents());
manager->set_auto_response_for_test(request_response);
- if (base::FeatureList::IsEnabled(
- content_settings::features::kApproximateGeolocationPermission) &&
+ if (IsParamFeatureEnabled() &&
(request_response == permissions::PermissionRequestManager::ACCEPT_ALL ||
request_response ==
permissions::PermissionRequestManager::ACCEPT_ONCE)) {
@@ -442,30 +441,12 @@
url, url, content_settings::GeolocationContentSettingsType(), setting);
}
-PermissionSetting GeolocationBrowserTest::ExpectedAllowPermissionSetting() {
- if (base::FeatureList::IsEnabled(
- content_settings::features::kApproximateGeolocationPermission)) {
- return GeolocationSetting{PermissionOption::kAllowed,
- PermissionOption::kAllowed};
- }
- return CONTENT_SETTING_ALLOW;
-}
-
-PermissionSetting GeolocationBrowserTest::ExpectedBlockPermissionSetting() {
- if (base::FeatureList::IsEnabled(
- content_settings::features::kApproximateGeolocationPermission)) {
- return GeolocationSetting{PermissionOption::kDenied,
- PermissionOption::kDenied};
- }
- return CONTENT_SETTING_BLOCK;
-}
-
-PermissionSetting GeolocationBrowserTest::ExpectedAskPermissionSetting() {
- if (base::FeatureList::IsEnabled(
- content_settings::features::kApproximateGeolocationPermission)) {
- return GeolocationSetting{PermissionOption::kAsk, PermissionOption::kAsk};
- }
- return CONTENT_SETTING_ASK;
+PermissionSetting GeolocationBrowserTest::ExpectedPermissionSetting(
+ ContentSetting setting) {
+ return content_settings::PermissionSettingsRegistry::GetInstance()
+ ->Get(content_settings::GeolocationContentSettingsType())
+ ->delegate()
+ .ToPermissionSetting(setting);
}
// Tests ----------------------------------------------------------------------
@@ -474,7 +455,7 @@
ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
ASSERT_TRUE(WatchPositionAndGrantPermission());
- EXPECT_EQ(ExpectedAllowPermissionSetting(),
+ EXPECT_EQ(ExpectedPermissionSetting(CONTENT_SETTING_ALLOW),
GetGeolocationContentSetting(GetTestURL()));
// Ensure a second request doesn't create a prompt in this tab.
@@ -492,7 +473,7 @@
EXPECT_TRUE(WatchPositionAndDenyPermission());
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
- EXPECT_EQ(ExpectedBlockPermissionSetting(),
+ EXPECT_EQ(ExpectedPermissionSetting(CONTENT_SETTING_BLOCK),
GetGeolocationContentSetting(GetTestURL()));
// Ensure a second request doesn't create a prompt in this tab.
@@ -511,7 +492,8 @@
IN_PROC_BROWSER_TEST_P(GeolocationBrowserTest, NoPromptForDeniedOrigin) {
ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
- SetGeolocationContentSetting(GetTestURL(), ExpectedBlockPermissionSetting());
+ SetGeolocationContentSetting(
+ GetTestURL(), ExpectedPermissionSetting(CONTENT_SETTING_BLOCK));
// Check that the request wasn't shown but we get an error for this origin.
WatchPositionAndObservePermissionRequest(false);
@@ -525,7 +507,8 @@
IN_PROC_BROWSER_TEST_P(GeolocationBrowserTest, NoPromptForAllowedOrigin) {
ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
- SetGeolocationContentSetting(GetTestURL(), ExpectedAllowPermissionSetting());
+ SetGeolocationContentSetting(
+ GetTestURL(), ExpectedPermissionSetting(CONTENT_SETTING_ALLOW));
// The request is not shown, there is no error, and the position gets to the
// script.
WatchPositionAndObservePermissionRequest(false);
@@ -737,7 +720,8 @@
ASSERT_TRUE(WatchPositionAndGrantPermission());
ExpectPosition(fake_latitude(), fake_longitude());
- SetGeolocationContentSetting(GetTestURL(), ExpectedBlockPermissionSetting());
+ SetGeolocationContentSetting(
+ GetTestURL(), ExpectedPermissionSetting(CONTENT_SETTING_BLOCK));
EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
@@ -749,7 +733,8 @@
ASSERT_TRUE(WatchPositionAndGrantPermission());
ExpectPosition(fake_latitude(), fake_longitude());
- SetGeolocationContentSetting(GetTestURL(), ExpectedAskPermissionSetting());
+ SetGeolocationContentSetting(GetTestURL(),
+ ExpectedPermissionSetting(CONTENT_SETTING_ASK));
EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
@@ -762,12 +747,14 @@
ASSERT_TRUE(WatchPositionAndGrantPermission());
ExpectPosition(fake_latitude(), fake_longitude());
- SetGeolocationContentSetting(GetTestURL(), ExpectedBlockPermissionSetting());
+ SetGeolocationContentSetting(
+ GetTestURL(), ExpectedPermissionSetting(CONTENT_SETTING_BLOCK));
EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
- SetGeolocationContentSetting(GetTestURL(), ExpectedAllowPermissionSetting());
+ SetGeolocationContentSetting(
+ GetTestURL(), ExpectedPermissionSetting(CONTENT_SETTING_ALLOW));
ASSERT_TRUE(WatchPositionAndGrantPermission());
ExpectPosition(fake_latitude(), fake_longitude());
}
@@ -783,7 +770,8 @@
ExpectPosition(fake_latitude(), fake_longitude());
// Toggle to deny on a.test
- SetGeolocationContentSetting(a_test_gurl, ExpectedBlockPermissionSetting());
+ SetGeolocationContentSetting(
+ a_test_gurl, ExpectedPermissionSetting(CONTENT_SETTING_BLOCK));
EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
@@ -799,7 +787,8 @@
GURL b_test_gurl = GetTestURLForHostname("b.test");
// Set up geolocation as allowed on b.test
- SetGeolocationContentSetting(b_test_gurl, ExpectedAllowPermissionSetting());
+ SetGeolocationContentSetting(
+ b_test_gurl, ExpectedPermissionSetting(CONTENT_SETTING_ALLOW));
// Open a.test and allow geolocation.
ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT, a_test_gurl));
@@ -807,7 +796,8 @@
ExpectPosition(fake_latitude(), fake_longitude());
// Toggle grant to block.
- SetGeolocationContentSetting(a_test_gurl, ExpectedBlockPermissionSetting());
+ SetGeolocationContentSetting(
+ a_test_gurl, ExpectedPermissionSetting(CONTENT_SETTING_BLOCK));
EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
```
Change information
Commit message:
[Geolocation] Parameterize GeolocationBrowserTest
This CL parameterizes GeolocationBrowserTest over the
kApproximateGeolocationPermission feature flag.
To support both configurations, test assertions and content setting
helpers are updated to natively read and write PermissionSetting
variants instead of lossily translating to legacy ContentSettings.
Additionally, tests that auto-accept permission requests now inject
GeolocationPromptOptions to satisfy the resolver's requirements when
the feature flag is enabled.
Bug: 509923623
Change-Id: I4678a12d574a5b9191994048d8a170d446bb8ba7
Reviewed-by: Alvin Ji <alv...@chromium.org>
Commit-Queue: Tom Van Goethem <t...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1660145}
Files:
- M chrome/browser/geolocation/geolocation_browsertest.cc
Change size: M
Delta: 1 file changed, 91 insertions(+), 57 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Alvin Ji
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages