Skip to content

[dotnet] [bidi] Align SetGeolocation polymorphic command#17338

Merged
nvborisenko merged 1 commit intoSeleniumHQ:trunkfrom
nvborisenko:bidi-setgeolocation
Apr 11, 2026
Merged

[dotnet] [bidi] Align SetGeolocation polymorphic command#17338
nvborisenko merged 1 commit intoSeleniumHQ:trunkfrom
nvborisenko:bidi-setgeolocation

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

One more polymorphic command.

💥 What does this PR do?

This pull request refactors and simplifies the geolocation override API in the BiDi Emulation module. The main change is to unify the various geolocation override methods into a single method that accepts a new, more flexible type, making the API easier to use and extend. Related test cases and type definitions have also been updated accordingly.

API Unification and Simplification:

  • Replaced multiple geolocation override methods (SetGeolocationCoordinatesOverrideAsync, SetGeolocationPositionErrorOverrideAsync) with a single method SetGeolocationOverrideAsync that accepts a new GeolocationOverride parameter, supporting both coordinate and error overrides in one place. ([[1]](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-326495cc635fdb9686d337228f031a251b2aaed55960d7096b1345e0f1c267ffL108-L123), [[2]](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-67b1e6d29a1577b79f92d4a3eece9a990768c05d60f023bd9a4c00e913e926a5L25-R25))

New Type Hierarchy for Geolocation Override:

  • Introduced an abstract record GeolocationOverride with two implementations: GeolocationCoordinatesOverride (for specifying coordinates and related parameters) and GeolocationPositionErrorOverride (for simulating a position error). ([dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.csR24-R36](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-c98b85709ff9dfdf68799a0b5e3ebb35e890deef4e56db26562a9c71d07eabcfR24-R36))

Type and Options Cleanup:

  • Removed now-unnecessary option types (SetGeolocationCoordinatesOverrideOptions, SetGeolocationPositionErrorOverrideOptions) and consolidated configuration into a single SetGeolocationOverrideOptions type. ([dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.csL40-L57](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-c98b85709ff9dfdf68799a0b5e3ebb35e890deef4e56db26562a9c71d07eabcfL40-L57))

Test Updates:

  • Updated all relevant tests to use the new unified SetGeolocationOverrideAsync method and types, ensuring coverage for both coordinate and error override scenarios. ([[1]](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-ffc3db78f3491d8fdaa73663a8ab27170b6f892406b5e40a27a023ff40742fccL201-R201), [[2]](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-ffc3db78f3491d8fdaa73663a8ab27170b6f892406b5e40a27a023ff40742fccL211-R211), [[3]](https://github.com/SeleniumHQ/selenium/pull/17338/files#diff-ffc3db78f3491d8fdaa73663a8ab27170b6f892406b5e40a27a023ff40742fccL222-R222))

🔄 Types of changes

  • Cleanup (formatting, renaming)

Copilot AI review requested due to automatic review settings April 11, 2026 08:01
@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Apr 11, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Unify geolocation override API with polymorphic command pattern

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Consolidate three geolocation override methods into single polymorphic method
• Introduce abstract GeolocationOverride type with two sealed implementations
• Move coordinate parameters from options to override record properties
• Update tests to use new unified API with polymorphic types

Grey Divider

File Changes

1. dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs ✨ Enhancement +12/-14

Consolidate geolocation methods into polymorphic single method

• Replace three separate geolocation override methods with single SetGeolocationOverrideAsync
 method
• Implement pattern matching to handle GeolocationCoordinatesOverride,
 GeolocationPositionErrorOverride, and null cases
• Construct appropriate parameters based on override type using switch expression

dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs


2. dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs ✨ Enhancement +1/-3

Update interface to reflect unified geolocation API

• Remove three method overloads: SetGeolocationCoordinatesOverrideAsync (two variants) and
 SetGeolocationPositionErrorOverrideAsync
• Add single SetGeolocationOverrideAsync method accepting GeolocationOverride parameter

dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs


3. dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs ✨ Enhancement +14/-12

Add polymorphic override types and simplify options hierarchy

• Introduce abstract GeolocationOverride base record with two sealed implementations
• Move latitude, longitude, and accuracy properties from options to GeolocationCoordinatesOverride
 record
• Remove SetGeolocationCoordinatesOverrideOptions and SetGeolocationPositionErrorOverrideOptions
 types
• Consolidate into single SetGeolocationOverrideOptions sealed record

dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs


View more (1)
4. dotnet/test/webdriver/BiDi/Emulation/EmulationTests.cs 🧪 Tests +3/-3

Align tests with new unified geolocation API

• Update CanSetGeolocationCoordinatesOverride test to use new GeolocationCoordinatesOverride
 type
• Update CanSetGeolocationCoordinatesOverrideToDefault test to pass null instead of empty options
• Update CanSetGeolocationPositionErrorOverride test to use new GeolocationPositionErrorOverride
 type

dotnet/test/webdriver/BiDi/Emulation/EmulationTests.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Apr 11, 2026

Code Review by Qodo

🐞 Bugs (1)   📘 Rule violations (2)   📎 Requirement gaps (0)   🎨 UX Issues (0)
🐞\ ≡ Correctness (1)
📘\ ⚙ Maintainability (2)

Grey Divider


Action required

1. Removed SetGeolocation*OverrideAsync APIs 📘
Description
Public geolocation override methods were removed/renamed and replaced with
SetGeolocationOverrideAsync, which will break source/binary compatibility for existing consumers.
This violates the requirement to keep public interfaces backward-compatible for version-only
upgrades.
Code

dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs[L25-27]

-    Task<SetGeolocationOverrideResult> SetGeolocationCoordinatesOverrideAsync(double latitude, double longitude, SetGeolocationCoordinatesOverrideOptions? options = null, CancellationToken cancellationToken = default);
-    Task<SetGeolocationOverrideResult> SetGeolocationCoordinatesOverrideAsync(SetGeolocationOverrideOptions? options = null, CancellationToken cancellationToken = default);
-    Task<SetGeolocationOverrideResult> SetGeolocationPositionErrorOverrideAsync(SetGeolocationPositionErrorOverrideOptions? options = null, CancellationToken cancellationToken = default);
+    Task<SetGeolocationOverrideResult> SetGeolocationOverrideAsync(GeolocationOverride? geolocationOverride, SetGeolocationOverrideOptions? options = null, CancellationToken cancellationToken = default);
Evidence
PR Compliance ID 1 forbids incompatible public API/ABI changes; the interface methods
SetGeolocationCoordinatesOverrideAsync(...) and SetGeolocationPositionErrorOverrideAsync(...)
are deleted and replaced by a different signature (SetGeolocationOverrideAsync(...)), requiring
downstream code changes and breaking implementations of IEmulationModule.

AGENTS.md
dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs[25-27]
dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs[25-25]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Public geolocation override methods were removed from `IEmulationModule`/`EmulationModule`, creating a breaking change for downstream consumers.

## Issue Context
This PR introduces `SetGeolocationOverrideAsync(GeolocationOverride? ...)` but deletes the previous public overloads, which violates the compatibility requirement.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs[25-27]
- dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs[108-124]

## Suggested approach
- Reintroduce the previous methods with their original signatures.
- Mark them `[Obsolete("Use SetGeolocationOverrideAsync(...)")]`.
- Implement them as thin wrappers that construct the appropriate `GeolocationOverride` and forward to `SetGeolocationOverrideAsync(...)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Removed geolocation option types 📘
Description
Public option types for geolocation overrides were removed without a deprecation path or guidance to
the replacement. This causes immediate user breakage and violates the deprecate-before-removal
requirement.
Code

dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs[L47-57]

-public sealed record SetGeolocationCoordinatesOverrideOptions : SetGeolocationOverrideOptions
-{
-    public double? Accuracy { get; init; }
-    public double? Altitude { get; init; }
-    public double? AltitudeAccuracy { get; init; }
-    public double? Heading { get; init; }
-    public double? Speed { get; init; }
-}
-
-public sealed record SetGeolocationPositionErrorOverrideOptions : SetGeolocationOverrideOptions;
-
Evidence
PR Compliance ID 8 requires public functionality to be deprecated before removal with an
alternative; SetGeolocationCoordinatesOverrideOptions and
SetGeolocationPositionErrorOverrideOptions are deleted, with no deprecation attributes/messages
present in the diff to guide users toward SetGeolocationOverrideOptions/GeolocationOverride.

AGENTS.md
dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs[47-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Public option types were removed without deprecation and migration guidance.

## Issue Context
Consumers may reference `SetGeolocationCoordinatesOverrideOptions` / `SetGeolocationPositionErrorOverrideOptions`; removing them forces immediate code changes.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs[47-57]

## Suggested approach
- Reintroduce the removed option types.
- Mark them `[Obsolete("Use SetGeolocationOverrideOptions with GeolocationOverride (GeolocationCoordinatesOverride / GeolocationPositionErrorOverride)")]`.
- If needed, keep them as derived/alias records that map cleanly onto the new model so existing code continues to compile.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Base type rejects subclasses 🐞
Description
SetGeolocationOverrideAsync accepts the public abstract GeolocationOverride type but throws
ArgumentException for any subtype except
GeolocationCoordinatesOverride/GeolocationPositionErrorOverride, so consumer-defined subclasses
compile but always fail at runtime.
Code

dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs[R108-120]

+    public async Task<SetGeolocationOverrideResult> SetGeolocationOverrideAsync(GeolocationOverride? geolocationOverride, SetGeolocationOverrideOptions? options = null, CancellationToken cancellationToken = default)
    {
-        var coordinates = new GeolocationCoordinates(latitude, longitude, options?.Accuracy, options?.Altitude, options?.AltitudeAccuracy, options?.Heading, options?.Speed);
-        var @params = new SetGeolocationOverrideCoordinatesParameters(coordinates, options?.Contexts, options?.UserContexts);
-        return await ExecuteAsync(SetGeolocationOverrideCommand, @params, options, cancellationToken).ConfigureAwait(false);
-    }
+        SetGeolocationOverrideParameters @params = geolocationOverride switch
+        {
+            GeolocationCoordinatesOverride c => new SetGeolocationOverrideCoordinatesParameters(
+                new GeolocationCoordinates(c.Latitude, c.Longitude, c.Accuracy, c.Altitude, c.AltitudeAccuracy, c.Heading, c.Speed),
+                options?.Contexts, options?.UserContexts),
+            GeolocationPositionErrorOverride => new SetGeolocationOverridePositionErrorParameters(
+                new GeolocationPositionError(), options?.Contexts, options?.UserContexts),
+            null => new SetGeolocationOverrideCoordinatesParameters(
+                null, options?.Contexts, options?.UserContexts),
+            _ => throw new ArgumentException($"Unknown geolocation override type: {geolocationOverride.GetType()}", nameof(geolocationOverride))
+        };
Evidence
GeolocationOverride is a public abstract record with no access restrictions preventing external
inheritance, but SetGeolocationOverrideAsync only handles two known subtypes and throws for anything
else, creating a compile-time-allowed but runtime-rejected public contract.

dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs[24-36]
dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs[108-123]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`GeolocationOverride` is a `public abstract record`, so external consumers can legally create `record CustomOverride : GeolocationOverride`. However, `EmulationModule.SetGeolocationOverrideAsync` throws for any subtype other than the two built-in ones, causing a runtime exception for code that compiles.

### Issue Context
This is an API contract mismatch: the type system suggests extensibility, but the implementation rejects extensions.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs[24-36]
- dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs[108-120]

### Recommended fix
Make the hierarchy explicitly closed to external inheritance, e.g. give `GeolocationOverride` an `internal` constructor:

```csharp
public abstract record GeolocationOverride
{
   internal GeolocationOverride() { }
}
```

This preserves the public type for consumers to *use* the provided overrides while preventing consumer-defined subtypes that will always be rejected.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the .NET BiDi Emulation geolocation override API to use a single polymorphic command (SetGeolocationOverrideAsync) that accepts a new GeolocationOverride type, consolidating coordinate and error overrides into one entry point.

Changes:

  • Replaced multiple geolocation override methods with a single SetGeolocationOverrideAsync(GeolocationOverride? ...) API.
  • Added a new public GeolocationOverride hierarchy (GeolocationCoordinatesOverride, GeolocationPositionErrorOverride) and consolidated options into SetGeolocationOverrideOptions.
  • Updated BiDi emulation tests to use the unified API.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
dotnet/test/webdriver/BiDi/Emulation/EmulationTests.cs Updates tests to call the unified geolocation override API for coordinates, default reset, and error override.
dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs Introduces the new GeolocationOverride type hierarchy and removes legacy option types while consolidating options.
dotnet/src/webdriver/BiDi/Emulation/IEmulationModule.cs Updates the public interface to expose only the unified geolocation override method.
dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs Implements SetGeolocationOverrideAsync and removes the prior dedicated geolocation override methods.

@nvborisenko nvborisenko merged commit 0157a05 into SeleniumHQ:trunk Apr 11, 2026
25 checks passed
@nvborisenko nvborisenko deleted the bidi-setgeolocation branch April 11, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-dotnet .NET Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants