feat: add accuracyMode to emulation.setGeolocationOverride#1105
feat: add accuracyMode to emulation.setGeolocationOverride#1105alvinjiooo wants to merge 1 commit into
Conversation
|
Hi @OrKoN, |
whimboo
added
enhancement
| (coordinates: emulation.GeolocationCoordinates / null) // | ||
| (error: emulation.GeolocationPositionError) | ||
| ), | ||
| ? accuracyMode: "precise" / "approximate", |
There was a problem hiding this comment.
Does that also apply to errors? If not we should add this field to GeolocationCoordinates.
| ? accuracyMode: "precise" / "approximate", | |
| ? accuracyMode: "approximate", "precise", |
There was a problem hiding this comment.
Regarding the placement of accuracyMode: While we could move it into GeolocationCoordinates, it is intended to describe the operating mode of the browser's geolocation service (based on the website's request and the user's permission state) rather than a property of the coordinates themselves. In the underlying Geolocation spec PR #195, accuracyMode is defined on the GeolocationPosition interface at the same level as coords (GeolocationCoordinates).
And yes, accuracyMode should be paired only with coordinates, I wonder would it work if I move it to the group with coordinates like:
emulation.SetGeolocationOverrideParameters = {
(
(
coordinates: emulation.GeolocationCoordinates / null,
? accuracyMode: "precise" / "approximate"
) //
(error: emulation.GeolocationPositionError)
),
? contexts: [+browsingContext.BrowsingContext],
? userContexts: [+browser.UserContext],
}
Will this break existing tooling and tests which already using setGeolocationOverride?
There was a problem hiding this comment.
I actually update the PR further for emulated position data to either emulation.GeolocationCoordinates or GeolocationPositionError. I think it probably will be better to pass emulated position data as a map with {coordinates:..., accuracyMode?: ... } or {error: {code: ...}}, so the Geolocation side can decide what to created at their side. I also have Geolocation PR#195 (at step 5.5 of Acquire a position algorithm) updated to hook this up.
PTAL at latest version and thanks!
There was a problem hiding this comment.
Will this break existing tooling and tests which already using setGeolocationOverride?
This should work fine given that this is a new optional parameter. Older versions of browsers will just ignore it as long as the handling of the parameter is not supported.
There was a problem hiding this comment.
Hm, I don't see the proposed changes from your second last comment yet. accuracyMode should be part of emulation.GeolocationCoordinates given that if coordinates is null we won't need this parameter's value.
There was a problem hiding this comment.
Hi @whimboo,
Sorry for the confusion, my original plan was to add accuracyMode outside of GeolocationCoordinates, but after discussing with w3c/geolocation#195 reviewer we think it seems also feasible to add new accuracyMode into GeolocationCoordinates. So I updated the CDDL and remote end steps for the new proposal (add new accuracyMode into GeolocationCoordinates). Thanks!
There was a problem hiding this comment.
Hi @whimboo,
Do you get a chance to review the latest PR?
Thanks!
alvinjiooo
force-pushed
the
geolocation-api
branch
2 times, most recently
from
a0cc095 to
f0340e4
Compare
alvinjiooo
force-pushed
the
geolocation-api
branch
from
f0340e4 to
4c532cd
Compare
| A <dfn>geolocation override</dfn> is a [=struct=] with: | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">latitude</dfn> which is a float; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">longitude</dfn> which is a float; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">accuracy</dfn> which is a float; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">altitude</dfn> which is a float or null; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">altitudeAccuracy</dfn> which is a float or null; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">heading</dfn> which is a float or null; | ||
| * [=struct/item=] named <dfn attribute for="geolocation override">speed</dfn> which is a float or null. | ||
| A <dfn>geolocation override</dfn> is a [=/map=]. |
There was a problem hiding this comment.
Why did you change all these lines? It's for the internal data structure. You need to get the accuracy mode added. Maybe this was caused by a merge conflict? The same applies to the changes below.
There was a problem hiding this comment.
Hi @whimboo,
Thanks for the review! The reason for changing the geolocation override from a struct to a map was intend to fix an existing type inconsistency in the spec regarding how we handle the error parameter.
In the original code, geolocation override was defined as a strict struct containing only coordinate floats (latitude, longitude, etc.). However, in the algorithm steps for setGeolocationOverride, when an "error" is provided, the spec does this:
Let emulated position data be a map matching GeolocationPositionError production...
This created an inconsistency where the geolocation override configuration (which is typed as a geolocation override) was being forced to store a map representing a GeolocationPositionError, instead of the expected struct of coordinates. I believe changing internal definition of geolocation override to be a map, containing either GeolocationCoordinates or GeolocationPositionError, should resolve this conflict. So Geolocation side can check what does geolocation override contain and decide what to create. Please let me know what do you think about this.
Thanks!
…factor setGeolocationOverride
alvinjiooo
force-pushed
the
geolocation-api
branch
from
4c532cd to
b9a46f8
Compare
2 participants
This pull request adds an optional
accuracyModeparameter to theemulation.setGeolocationOverridecommand.This aligns WebDriver BiDi with the upcoming changes to the W3C Geolocation API, which introduces support for approximate geolocation via the
accuracyModeattribute on theGeolocationPositioninterface.Preview | Diff