Introduce accuracyMode for GeolocationPosition [chromium/src : main]
Tom Van Goethem (Gerrit)
Tom Van Goethem added 1 comment![]( "Open in Gerrit")
Related details
- Alvin Ji
- 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 1 comment![]( "Open in Gerrit")
[RuntimeEnabled=ApproximateGeolocationPermission] readonly attribute AccuracyMode accuracyMode;This needs to be guarded behind a different feature flag (ApproximateGeolocationPermissionAPIChanges or something similar), which can only be enabled if ApproximateGeolocationPermission is enabled (or which enables the ApproximateGeolocationPermission transitively).
Related details
- Alvin Ji
- Tom Van Goethem
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)
Tom Van Goethem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Van Goethem
| Commit-Queue | +1 |
1 comment
[RuntimeEnabled=ApproximateGeolocationPermission] readonly attribute AccuracyMode accuracyMode;This needs to be guarded behind a different feature flag (ApproximateGeolocationPermissionAPIChanges or something similar), which can only be enabled if ApproximateGeolocationPermission is enabled (or which enables the ApproximateGeolocationPermission transitively).
Added a ApproximateGeolocationWebVisibleAPI feature flag.
Related details
- Alvin Ji
- 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 4 comments![]( "Open in Gerrit")
@Features.EnableFeatures({PermissionsAndroidFeatureList.APPROXIMATE_GEOLOCATION_PERMISSION})nit: you don't need braces {} if you only have one feature
ContentSettingsType.GEOLOCATION,shouldn't you set GEOLOCATION_WITH_OPTIONS?
mPermissionRule.runJavaScriptCodeInCurrentTabWithGesture(runJavaScriptCodeInCurrentTab should return the value from javascript. My understanding is that it should even work with promises. Can you try to use that instead of relying on the window title (!) being updated?
runTest(/* precise= */ true);can we inline the test content here instead? You can use a parameter for precise, as in https://source.chromium.org/chromium/chromium/src/+/main:base/test/android/junit/src/org/chromium/base/test/params/ExampleParameterizedTest.java;l=85;drc=6d903dddebf88f24cf9f9ba504777ea4b81f429b
Related details
- Alvin Ji
- Tom Van Goethem
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)
Alvin Ji added 1 comment![]( "Open in Gerrit")
const expectedPosition = {timestamp, coords: mockCoords, accuracyMode: 'precise'};Currently the geolocation test in wpt_internal is using `third_party/blink/web_tests/wpt_internal/geolocation-api/resources/geolocation-mock.js` to intercept mojo message from blink. Hence the `setGeolocationPosition` and `makeGeoposition` need to be updated for having `accuracyMode` set too.
Related details
- Tom Van Goethem
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)
Tom Van Goethem added 5 comments![]( "Open in Gerrit")
@Features.EnableFeatures({PermissionsAndroidFeatureList.APPROXIMATE_GEOLOCATION_PERMISSION})nit: you don't need braces {} if you only have one feature
Done
shouldn't you set GEOLOCATION_WITH_OPTIONS?
Done
mPermissionRule.runJavaScriptCodeInCurrentTabWithGesture(runJavaScriptCodeInCurrentTab should return the value from javascript. My understanding is that it should even work with promises. Can you try to use that instead of relying on the window title (!) being updated?
I used the same mechanism as [related tests](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/javatests/src/org/chromium/chrome/browser/permissions/RuntimePermissionTestUtils.java;l=166-199) here. However, I agree that relying on window title is not ideal, and was already working on a better approach in a follow-up CL. That CL will also combine both the API and `<geolocation>` permissions into a single test. Is it fine if I address this comment there?
can we inline the test content here instead? You can use a parameter for precise, as in https://source.chromium.org/chromium/chromium/src/+/main:base/test/android/junit/src/org/chromium/base/test/params/ExampleParameterizedTest.java;l=85;drc=6d903dddebf88f24cf9f9ba504777ea4b81f429b
Done
const expectedPosition = {timestamp, coords: mockCoords, accuracyMode: 'precise'};Currently the geolocation test in wpt_internal is using `third_party/blink/web_tests/wpt_internal/geolocation-api/resources/geolocation-mock.js` to intercept mojo message from blink. Hence the `setGeolocationPosition` and `makeGeoposition` need to be updated for having `accuracyMode` set too.
Updated, and added test that checks both precise and approximate accuracy modes.
Related details
- Alvin Ji
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Antonio Sartori
| Code-Review | +1 |
3 comments
mPermissionRule.runJavaScriptCodeInCurrentTabWithGesture(Tom Van GoethemrunJavaScriptCodeInCurrentTab should return the value from javascript. My understanding is that it should even work with promises. Can you try to use that instead of relying on the window title (!) being updated?
I used the same mechanism as [related tests](https://source.chromium.org/chromium/chromium/src/+/main:chrome/android/javatests/src/org/chromium/chrome/browser/permissions/RuntimePermissionTestUtils.java;l=166-199) here. However, I agree that relying on window title is not ideal, and was already working on a better approach in a follow-up CL. That CL will also combine both the API and `<geolocation>` permissions into a single test. Is it fine if I address this comment there?
I guess it is :)
promise_test(async () => {
mock.setGeolocationPermission(false, true);
mock.setGeolocationPosition(mockCoords.latitude,
mockCoords.longitude,
mockCoords.accuracy);
const position = await new Promise((resolve, reject) => {
navigator.geolocation.getCurrentPosition(
resolve,
(e) => reject(new Error(`Error callback invoked unexpectedly: ${e.message}`)));
});
assert_object_equals(position.coords.toJSON(), mockCoords);
const timestamp = position.timestamp;
const expectedPosition = {timestamp, coords: mockCoords};
if (internals.runtimeFlags.approximateGeolocationWebVisibleAPIEnabled) {
expectedPosition.accuracyMode = 'approximate';
}
assert_object_equals(position.toJSON(), expectedPosition);
}, "Tests toJSON() on GeolocationPosition and GeolocationCoordinates with approximate permission.");(nit) There seems to be a lot of duplicated logic here. Maybe you could just have parametrized tests? E.g.
```js
[ true, false ].forEach(isPrecise => {
promise_test(async () => {
// ...
});
});
```
(feel free to improve e.g. with named params.)
Related details
- Alvin Ji
- Tom Van Goethem
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)
Alvin Ji voted and added 3 comments![]( "Open in Gerrit")
Votes added by Alvin Ji
| Code-Review | +1 |
3 comments
LGTM with comments.
EqualIgnoringASCIICase(params.new_value, kAccuracyModePrecise));Should we also remove `kAccuracyModePrecise` if now we don't use it?.
setTimeout(() => {this.createGeolocation(receiver, user_gesture)}, 50);This `setTimeout` call is not reachable in base version of the code. I think we should remove it here.
Related details
- Tom Van Goethem
Tom Van Goethem (Gerrit)
Tom Van Goethem added 3 comments![]( "Open in Gerrit")
EqualIgnoringASCIICase(params.new_value, kAccuracyModePrecise));Should we also remove `kAccuracyModePrecise` if now we don't use it?.
It is removed (see line 22). Unless I'm missing something?
promise_test(async () => {
mock.setGeolocationPermission(false, true);
mock.setGeolocationPosition(mockCoords.latitude,
mockCoords.longitude,
mockCoords.accuracy);
const position = await new Promise((resolve, reject) => {
navigator.geolocation.getCurrentPosition(
resolve,
(e) => reject(new Error(`Error callback invoked unexpectedly: ${e.message}`)));
});
assert_object_equals(position.coords.toJSON(), mockCoords);
const timestamp = position.timestamp;
const expectedPosition = {timestamp, coords: mockCoords};
if (internals.runtimeFlags.approximateGeolocationWebVisibleAPIEnabled) {
expectedPosition.accuracyMode = 'approximate';
}
assert_object_equals(position.toJSON(), expectedPosition);
}, "Tests toJSON() on GeolocationPosition and GeolocationCoordinates with approximate permission.");(nit) There seems to be a lot of duplicated logic here. Maybe you could just have parametrized tests? E.g.
```js
[ true, false ].forEach(isPrecise => {
promise_test(async () => {
// ...
});
});
```(feel free to improve e.g. with named params.)
Done.
setTimeout(() => {this.createGeolocation(receiver, user_gesture)}, 50);This `setTimeout` call is not reachable in base version of the code. I think we should remove it here.
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)
Tom Van Goethem voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Colin Blundell
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
1 comment
Related details
- Dave Tapuska
- Tom Van Goethem
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Tom Van Goethem
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)
Tom Van Goethem voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Introduce accuracyMode for GeolocationPosition
This CL introduces the accuracyMode attribute when the ApproximateGeolocationPermission feature is enabled.
- M chrome/android/javatests/BUILD.gn
- A chrome/android/javatests/src/org/chromium/chrome/browser/permissions/ApproximateGeolocationTest.java
- M content/test/data/android/geolocation.html
- M services/device/public/java/src/org/chromium/device/geolocation/MockLocationProvider.java
- M testing/variations/fieldtrial_testing_config.json
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/core/geolocation/geolocation.cc
- M third_party/blink/renderer/core/geolocation/geolocation_position.idl
- M third_party/blink/renderer/core/geolocation/geoposition.cc
- M third_party/blink/renderer/core/geolocation/geoposition.h
- M third_party/blink/renderer/core/html/html_geolocation_element.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/web_tests/wpt_internal/geolocation-api/json.https.html
- M third_party/blink/web_tests/wpt_internal/geolocation-api/resources/geolocation-mock.js
Code-Review: +1 by Colin Blundell, +1 by Antonio Sartori, +1 by Dave Tapuska, +1 by Alvin Ji
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |