Skip to content

fix(cddl): fix CDDL syntax for emulation.SetGeolocationOverrideParameters #914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

fix(cddl): fix CDDL syntax for emulation.SetGeolocationOverrideParameters #914

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
587cd8c
fix(cddl): fix CDDL syntax for emulation.SetGeolocationOverrideParame…
christian-bromann May 6, 2025
bcfbc4b
fix group syntax
christian-bromann May 6, 2025
3d8a8f8
align with extensibility idiom
christian-bromann May 6, 2025
8d2eefc
PR feedback
christian-bromann May 8, 2025
72060f6
add type parameter
christian-bromann May 8, 2025
82b7005
PR feedback
christian-bromann May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5624,13 +5624,20 @@ The <dfn export for=commands>emulation.setGeolocationOverride</dfn> command modi
params: emulation.SetGeolocationOverrideParameters
)

emulation.SetGeolocationOverrideParameters = {
(
(coordinates: emulation.GeolocationCoordinates / null) //
(error: emulation.GeolocationPositionError)
),
emulation.SetGeolocationOverrideParameters = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICS this can stay as a map, or do I miss something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think this should stay a map

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is possible to insert a group into a map, e.g.:

foobar = {
  someGroup,
  key: value
}

It is not possible to include group choices, because the map expects a single group, not a group choice expression*.

RFC 8610 (Section 3.5) says:

When a group is used as a member of a map, the entries in the group are included at that location in the map.

But that refers to a group, not a group choice. Group choices (//) are only valid inside a group.

Copy link
Contributor

@OrKoN OrKoN May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(emulation.SetGeolocationPosition // emulation.GeolocationPositionError) defines a new group (due to ()) so it is valid to put it into a map. We have other places where this works. The current PR version does provide any type for the params key.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OrKoN it is not allowed to have a group choice (//) between two maps ({ ... }). This aligns with other places in the spec, e.g.

  • (browser.ClientWindowNamedState // browser.ClientWindowRectState) (ref) where both browser.ClientWindowNamedState and browser.ClientWindowRectState are groups (( ... ))
  • (network.ContinueWithAuthCredentials // network.ContinueWithAuthNoCredentials) (ref) where also both network.ContinueWithAuthCredentials and network.ContinueWithAuthNoCredentials are groups

The CDDL specification indirectly defines this in RFC 8610 §3.9 — Group-to-Group Composition:

The operator // is used to express a choice between groups. The right-hand side of the operator must be a group.

This means:

  • A // B is only valid if both A and B are groups.
  • A map ({ ... }) is a type, not a group, and therefore cannot be used directly in //.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am not following: both SetGeolocationPosition and GeolocationPositionError are groups, not maps. SetGeolocationOverrideParameters has to remain a map:

emulation.SetGeolocationOverrideParameters = {
      ? contexts: [+browsingContext.BrowsingContext],
      ? userContexts: [+browser.UserContext],
     (emulation.SetGeolocationPosition // emulation.GeolocationPositionError)
}

? contexts: [+browsingContext.BrowsingContext],
? userContexts: [+browser.UserContext],
(emulation.SetGeolocationOverrideSuccessParameters // emulation.SetGeolocationOverrideErrorParameters)
)

emulation.SetGeolocationOverrideSuccessParameters = {
type: "success",
coordinates: emulation.GeolocationCoordinates / null
}

emulation.SetGeolocationOverrideErrorParameters = {
type: "error",
error: emulation.GeolocationPositionError
}

emulation.GeolocationCoordinates = {
Expand Down