[dotnet] [bidi] Stateful converters with hydration by nvborisenko · Pull Request #16670 · SeleniumHQ/selenium

nvborisenko · 2025-12-03T03:30:25Z

User description

Finally resolving issues with BiDi AOT trimming.

🔗 Related Issues

Related to #16095

💥 What does this PR do?

Improves stateful json converters
Keeping continuation feature as designed initially

🔧 Implementation Notes

Removed BiDi instance in DTO ctor

💡 Additional Considerations

Now assembly is fully trimmable, as we wanted.

🔄 Types of changes

Cleanup (formatting, renaming)
Bug fix (backwards compatible)
New feature (non-breaking change which adds functionality and tests!)

PR Type

Bug fix, Enhancement

Description

Removes stateful JSON converters to enable AOT trimming
Implements hydration pattern for BiDi property injection
Converts JsonSerializerContext to static singletons
Adds IBiDiHydratable interface for post-deserialization setup

Diagram Walkthrough

flowchart LR
  A["Stateful Converters<br/>with BiDi instance"] -->|Remove| B["Stateless Converters"]
  B -->|Hydrate after| C["IBiDiHydratable.Hydrate"]
  C -->|Set BiDi property| D["DTO instances"]
  E["JsonSerializerContext<br/>instance field"] -->|Convert to| F["Static singleton"]
  F -->|Use Default| G["AOT-trimmed assembly"]

File Walkthrough

Relevant files

Enhancement

20 files

IBiDiHydratable.cs `New interface for post-deserialization hydration`	+25/-0
Broker.cs `Hydrate command results and event args`	+10/-0
EventArgs.cs `Implement IBiDiHydratable for event args`	+6/-1
BrowserModule.cs `Convert context to static singleton with attributes`	+10/-7
CreateUserContextCommand.cs `Implement IBiDiHydratable for result`	+7/-1
BrowsingContextModule.cs `Convert context to static singleton with attributes`	+10/-7
CreateCommand.cs `Implement IBiDiHydratable for result`	+7/-1
GetTreeCommand.cs `Implement IBiDiHydratable for tree result`	+10/-1
AddDataCollectorCommand.cs `Implement IBiDiHydratable for result`	+7/-1
AddInterceptCommand.cs `Implement IBiDiHydratable for result`	+7/-1
NetworkModule.cs `Convert context to static singleton with attributes`	+10/-7
AddPreloadScriptCommand.cs `Implement IBiDiHydratable for result`	+7/-2
ScriptModule.cs `Convert context to static singleton with attributes`	+10/-7
EmulationModule.cs `Convert context to static singleton with attributes`	+10/-7
InputModule.cs `Convert context to static singleton with attributes`	+10/-7
LogModule.cs `Convert context to static singleton with attributes`	+10/-7
PermissionsModule.cs `Convert context to static singleton with attributes`	+10/-7
SessionModule.cs `Convert context to static singleton with attributes`	+10/-7
StorageModule.cs `Convert context to static singleton with attributes`	+10/-7
WebExtensionModule.cs `Convert context to static singleton with attributes`	+10/-7

Bug fix

20 files

BiDi.cs `Remove JsonOptions from module creation`	+13/-13
Module.cs `Remove Initialize method and JsonOptions parameter`	+1/-7
UserContext.cs `Remove BiDi constructor parameter, add hydration`	+8/-5
BrowsingContext.cs `Remove BiDi constructor parameter, add hydration`	+4/-3
Collector.cs `Remove BiDi constructor parameter, add hydration`	+8/-5
Intercept.cs `Remove BiDi constructor parameter, add hydration`	+11/-8
Handle.cs `Remove BiDi constructor parameter, add hydration`	+8/-4
InternalId.cs `Remove BiDi constructor parameter, add hydration`	+8/-4
PreloadScript.cs `Remove BiDi constructor parameter, add hydration`	+8/-5
Realm.cs `Remove BiDi constructor parameter, add hydration`	+8/-4
BrowsingContextConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
BrowserUserContextConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
CollectorConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
HandleConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
InterceptConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
InternalIdConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
PreloadScriptConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
RealmConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
WebExtensionConverter.cs `Remove stateful BiDi instance from converter`	+1/-8
Extension.cs `Remove BiDi constructor parameter, add hydration`	+8/-5

qodo-code-review · 2025-12-03T03:31:12Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Object hydration trust

Description: Hydration of IBiDiHydratable types after deserialization sets internal BiDi references
based solely on wire data, so if an attacker can influence payloads they could create
objects that gain access to the BiDi instance; ensure only trusted channels feed
deserialization and that hydration applies only to expected result types.
Broker.cs [282-286]

Referred Code

if (commandResult is IBiDiHydratable bidiHydratable)
{
    bidiHydratable.Hydrate(_bidi);
}

Ticket Compliance

🟡

🎫 #16095

🟢	Remove stateful JSON converters that depend on BiDi instance.
🟢	Use EmptyResult as valid return type for all methods instead of void.
🔴	Move BiDi access to AnyDriver/IWebDriver via AsBiDiAsync, making AnyDriver own BiDi lifecycle and dependency direction Selenium.WebDriver -> Selenium.WebDriver.BiDi.
	Expose previously hidden methods like AddIntercept as public.
	Revisit InterceptRequestAsync and related helpers to be extensions in a dedicated namespace, leveraging .NET 10 extension capabilities.
	Use normal types as event args with appropriate inheritance model.
	Remove overcomplicated syntax sugar like Result implementing IReadOnlyList, prefer extensions like .GetTreeAsync().AsContexts(), and remove strange JSON converters for these types.
	Commands should return exact types (e.g., SetCookieAsync returns exact type) instead of wrapper Result objects.
	Make command options immutable objects.
	Decide on scoped event handlers (remove or keep).
	Catch-all for any further finalizations to stabilize public API.
⚪	None

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: Critical command/result processing lacks explicit audit logging of actions, user/context,
and outcomes in the new hydration path.

Referred Code

if (commandResult is IBiDiHydratable bidiHydratable)
{
    bidiHydratable.Hydrate(_bidi);
}

command.TaskCompletionSource.SetResult((EmptyResult)commandResult);

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Hydration errors: The new IBiDiHydratable.Hydrate call is inside the try block but lacks dedicated
null/exception handling or fallback, which may swallow hydration-specific context.

Referred Code

    if (commandResult is IBiDiHydratable bidiHydratable)
    {
        bidiHydratable.Hydrate(_bidi);
    }

    command.TaskCompletionSource.SetResult((EmptyResult)commandResult);
}
catch (Exception ex)
{
    command.TaskCompletionSource.SetException(ex);

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Minimal parsing: New stateless converters accept string IDs without visible validation or sanitization,
relying on upstream trust; security relevance depends on external input sources.

Referred Code

public override BrowsingContext.BrowsingContext? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    var id = reader.GetString();

    return new BrowsingContext.BrowsingContext(id!);
}

public override void Write(Utf8JsonWriter writer, BrowsingContext.BrowsingContext value, JsonSerializerOptions options)
{

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-03T03:32:38Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Hydrate event arguments after deserialization~~ Suggestion Impact: The commit adds hydration-like assignment for event arguments by setting eventArgs.BiDi = _bidi after deserialization, addressing the suggested need to hydrate event arguments. It also removes the previous hydration call for command results. code diff: if (_eventTypesMap.TryGetValue(method, out var eventInfo)) { var eventArgs = (EventArgs)JsonSerializer.Deserialize(ref paramsReader, eventInfo)!; + + eventArgs.BiDi = _bidi; Add hydration logic for deserialized event arguments in the `ProcessReceivedMessage` method, similar to how command results are handled, to prevent runtime errors. dotnet/src/webdriver/BiDi/Broker.cs [282-285] +if (commandResult is IBiDiHydratable bidiHydratable) +{ + bidiHydratable.Hydrate(_bidi); +} - `[Suggestion processed]` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where event arguments are not hydrated, which would lead to `NullReferenceException` at runtime, defeating a major goal of the PR.	High
General	Allow derived classes to extend hydration Make the `Hydrate` method in the `EventArgs` base class `virtual` and `protected` to allow derived classes to override it and hydrate their own nested properties. dotnet/src/webdriver/BiDi/EventArgs.cs [24-33] public abstract record EventArgs : IBiDiHydratable { [JsonIgnore] public BiDi BiDi { get; internal set; } - void IBiDiHydratable.Hydrate(BiDi bidi) + protected virtual void Hydrate(BiDi bidi) { BiDi = bidi; } + + void IBiDiHydratable.Hydrate(BiDi bidi) + { + Hydrate(bidi); + } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion provides a robust and extensible pattern for hydrating nested objects within event arguments, which is a necessary improvement to the hydration mechanism introduced in the PR.	Medium
Learned best practice	✅ ~~Eliminate dead JSON options~~ Suggestion Impact: The commit removed GetJsonOptions and replaced its usage by introducing a single _jsonOptions field initialized in the constructor and passing it to Module.Create, thus eliminating the dead method and centralizing JSON options. code diff: + private readonly JsonSerializerOptions _jsonOptions; + private BiDi(string url) { var uri = new Uri(url); Broker = new Broker(this, uri); + + _jsonOptions = new JsonSerializerOptions + { + PropertyNameCaseInsensitive = true, + PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, + + // BiDi returns special numbers such as "NaN" as strings + // Additionally, -0 is returned as a string "-0" + NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals \| JsonNumberHandling.AllowReadingFromString, + Converters = + { + new BrowsingContextConverter(this), + //new BrowserUserContextConverter(), + //new CollectorConverter(), + //new InterceptConverter(), + //new HandleConverter(), + //new InternalIdConverter(), + //new PreloadScriptConverter(), + //new RealmConverter(), + new DateTimeOffsetConverter(), + //new WebExtensionConverter(), + } + }; } + + private Broker Broker { get; } internal Session.SessionModule SessionModule => AsModule<Session.SessionModule>(); @@ -85,35 +113,6 @@ public T AsModule<T>() where T : Module, new() { - return (T)_modules.GetOrAdd(typeof(T), _ => Module.Create<T>(this, Broker)); - } - - private Broker Broker { get; } - - private JsonSerializerOptions GetJsonOptions() - { - return new JsonSerializerOptions - { - //PropertyNameCaseInsensitive = true, - //PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - //DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - - // BiDi returns special numbers such as "NaN" as strings - // Additionally, -0 is returned as a string "-0" - NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals \| JsonNumberHandling.AllowReadingFromString, - Converters = - { - //new BrowsingContextConverter(), - //new BrowserUserContextConverter(), - //new CollectorConverter(), - //new InterceptConverter(), - //new HandleConverter(), - //new InternalIdConverter(), - //new PreloadScriptConverter(), - //new RealmConverter(), - new DateTimeOffsetConverter(), - //new WebExtensionConverter(), - } - }; + return (T)_modules.GetOrAdd(typeof(T), _ => Module.Create<T>(this, Broker, _jsonOptions)); } Remove the now-unused JsonSerializerOptions method and inline config; prefer the new source-generated contexts to avoid dead code and duplication. dotnet/src/webdriver/BiDi/BiDi.cs [95-117] -return new JsonSerializerOptions -{ - //PropertyNameCaseInsensitive = true, - //PropertyNamingPolicy = JsonNamingPolicy.CamelCase, - //DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, +// Remove GetJsonOptions() entirely and references to it. +// Modules now use source-generated contexts (e.g., BrowsingContextJsonSerializerContext.Default) - // BiDi returns special numbers such as "NaN" as strings - // Additionally, -0 is returned as a string "-0" - NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals \| JsonNumberHandling.AllowReadingFromString, - Converters = - { - //new BrowsingContextConverter(), - //new BrowserUserContextConverter(), - //new CollectorConverter(), - //new InterceptConverter(), - //new HandleConverter(), - //new InternalIdConverter(), - //new PreloadScriptConverter(), - //new RealmConverter(), - new DateTimeOffsetConverter(), - //new WebExtensionConverter(), - } -}; - `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Pattern 4: Centralize shared configuration logic to avoid duplication and ensure consistency (e.g., JSON options/converters).	Low
Update

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

dotnet/src/webdriver/BiDi/Broker.cs

qodo-code-review · 2025-12-03T04:08:12Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Deserialization hydration risk

Description: The hydration hook allows any deserialized type implementing IBiDiHydratable to receive a
BiDi instance, which could be abused if untrusted payloads were ever deserialized; ensure
inputs come only from trusted BiDi endpoints and IBiDiHydratable is confined to internal
types.
Broker.cs [279-288]

Referred Code

    var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo)
        ?? throw new JsonException("Remote end returned null command result in the 'result' property.");

    if (commandResult is IBiDiHydratable bidiHydratable)
    {
        bidiHydratable.Hydrate(_bidi);
    }

    command.TaskCompletionSource.SetResult((EmptyResult)commandResult);
}

Event hydration surface

Description: Event args deserialization hydrates any IBiDiHydratable instance from incoming events; if
event payloads were spoofable this could lead to unintended object graph mutation, so
validate source authenticity and restrict IBiDiHydratable to SDK types.
Broker.cs [310-316]

Referred Code

var eventArgs = (EventArgs)JsonSerializer.Deserialize(ref paramsReader, eventInfo)!;

if (eventArgs is IBiDiHydratable bidiHydratable)
{
    bidiHydratable.Hydrate(_bidi);
}

Ticket Compliance

🟡

🎫 #16095

🟢	Remove stateful JSON converters; avoid converters depending on BiDi instance.
🟢	Use EmptyResult as a valid return type instead of void across methods.
🔴	Move BiDi extension method AsBiDiAsync() to IWebDriver/AnyDriver as the owner of BiDi, ensuring lifecycle ownership and dependency direction Selenium.WebDriver -> Selenium.WebDriver.BiDi.
	Revisit InterceptRequestAsync and related APIs; move helpers/extensions to a dedicated namespace using .NET 10 extension capabilities.
	Use normal types as event args (inheritance topic).
	Remove IReadOnlyList sugar from Result types and associated custom JSON converters; provide extensions like .GetTreeAsync().AsContexts().
	Commands should return the exact type (Result) instead of wrapping/other objects; review all commands accordingly.
	Make command options immutable after construction.
	Decide on scoped event handlers: remove or keep.
	Address any additional big design changes as needed to finalize the public API.
⚪	Expose previously hidden internal methods like AddIntercept() as public.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Event Hydration Unlogged: New hydration steps for results and events add critical post-deserialization actions
without any accompanying audit logging, which may obscure tracing of these critical BiDi
interactions.

Referred Code

        if (commandResult is IBiDiHydratable bidiHydratable)
        {
            bidiHydratable.Hydrate(_bidi);
        }

        command.TaskCompletionSource.SetResult((EmptyResult)commandResult);
    }
    catch (Exception ex)
    {
        command.TaskCompletionSource.SetException(ex);
    }
    finally
    {
        _pendingCommands.TryRemove(id.Value, out _);
    }
}
else
{
    throw new BiDiException($"The remote end responded with 'success' message type, but no pending command with id {id} was found.");
}



 ... (clipped 14 lines)

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null Result Handling: While null results throw JsonException, the new hydration step does not guard against
partial/invalid object graphs (e.g., missing IDs) before setting BiDi, which could lead to
late failures without explicit handling.

Referred Code

    var commandResult = JsonSerializer.Deserialize(ref resultReader, command.JsonResultTypeInfo)
        ?? throw new JsonException("Remote end returned null command result in the 'result' property.");

    if (commandResult is IBiDiHydratable bidiHydratable)
    {
        bidiHydratable.Hydrate(_bidi);
    }

    command.TaskCompletionSource.SetResult((EmptyResult)commandResult);
}

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Minimal Validation: New stateless converters and constructors accept string IDs without validating format or
emptiness, which could allow invalid external input to propagate if upstream validation is
absent.

Referred Code

public override BrowsingContext.BrowsingContext? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
    var id = reader.GetString();

    return new BrowsingContext.BrowsingContext(id!);
}

public override void Write(Utf8JsonWriter writer, BrowsingContext.BrowsingContext value, JsonSerializerOptions options)

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-03T04:09:45Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ ~~Add runtime checks for hydration~~ Suggestion Impact: The commit introduced private nullable backing fields and updated the BiDi property in both UserContext and BrowsingContext to throw InvalidOperationException when not hydrated, matching the suggested pattern. code diff: + private BiDi? _bidi; + internal UserContext(string id) { Id = id; @@ -35,7 +37,11 @@ internal string Id { get; } [JsonIgnore] - public BiDi BiDi { get; internal set; } + public BiDi BiDi + { + get => _bidi ?? throw new InvalidOperationException($"{nameof(BiDi)} instance has not been hydrated."); + internal set => _bidi = value; + } public Task RemoveAsync() { # File: dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs @@ -41,8 +41,14 @@ internal string Id { get; } - [JsonIgnore] - public BiDi BiDi { get; internal set; } + private BiDi? _bidi; + + [JsonIgnore] + public BiDi BiDi + { + get => _bidi ?? throw new InvalidOperationException("BiDi instance has not been hydrated."); + internal set => _bidi = value; + } To prevent `NullReferenceException`s, add a runtime check to the `BiDi` property getter on objects using the new hydration pattern. This check should throw an `InvalidOperationException` if the property is accessed before it has been hydrated. Examples: dotnet/src/webdriver/BiDi/Browser/UserContext.cs [38] public BiDi BiDi { get; internal set; } dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [45] public BiDi BiDi { get; internal set; } Solution Walkthrough: Before: // In dotnet/src/webdriver/BiDi/Browser/UserContext.cs public sealed class UserContext : IAsyncDisposable { // ... constructor ... [JsonIgnore] public BiDi BiDi { get; internal set; } public Task RemoveAsync() { // This will throw NullReferenceException if BiDi is not hydrated return BiDi.Browser.RemoveUserContextAsync(this); } // ... } After: // In dotnet/src/webdriver/BiDi/Browser/UserContext.cs public sealed class UserContext : IAsyncDisposable { private BiDi? _bidi; // ... constructor ... [JsonIgnore] public BiDi BiDi { get => _bidi ?? throw new InvalidOperationException("BiDi instance has not been hydrated."); internal set => _bidi = value; } public Task RemoveAsync() { return BiDi.Browser.RemoveUserContextAsync(this); } // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant design weakness in the new hydration pattern, preventing potential `NullReferenceException`s and improving API robustness with explicit, fail-fast checks.	Medium
General	Improve interface accessibility for testability Change the `Hydrate` method in the `IBiDiHydratable` interface from `internal` to `public` to allow implementation and mocking outside the assembly. dotnet/src/webdriver/BiDi/IBiDiHydratable.cs [22-25] public interface IBiDiHydratable { - internal void Hydrate(BiDi bidi); + void Hydrate(BiDi bidi); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that an `internal` method in a `public` interface limits its utility for consumers, particularly for mocking and testing, and proposes making it public to improve testability.	Low
More

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

dotnet/src/webdriver/BiDi/BiDi.cs

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

nvborisenko · 2025-12-04T16:51:51Z

I successfully implemented 2 variants:

In general the first approach is new MyJsonSerializerContext(somEOptions) - as soon as we do it we are getting +2MB into assembly size with AOT trimming.
Be fully json generated. In this case assembly is slim, fully trimmed unnecessary unused types. But in this case hydration process will look like:

Task<Result> DoSomethingAsync()
{
  var result = await Broker.ExecuteCommandAsync(....);
  result.Hydrate(); // walk through all nested object properties and set `BiDi` instance
  return result;
}

The 2nd variant is technically ideal, but it requires a lot of boilerplate code. It also introduces new rule for us.

nvborisenko · 2025-12-06T15:45:01Z

Nicely done!

await bidi.StatusAsync();

Resulting trimmed assembly is 8.22 MB (vs 15 MB in trunk).

RenderMichael · 2025-12-06T16:28:09Z

dotnet/src/webdriver/BiDi/BiDi.cs

-                new WebExtensionConverter(this),
-            }
-        };
+        return (T)_modules.GetOrAdd(typeof(T), _ => Module.Create<T>(this, Broker, _jsonOptions));


Does this still support adding a module after BiDi is created?

Yes, each module creates new copy of provided instance.

var logOptions = new JsonSerializerOptions(jsonSerializerOptions) { Converters = { new BrowsingContextConverter(BiDi), new RealmConverter(BiDi), new InternalIdConverter(BiDi), new HandleConverter(BiDi), } }; _jsonContext = new LogJsonSerializerContext(logOptions);

We can simplify it, and always put new instance into module Initialize(...). I would be better for external modules. Will do.

nvborisenko added 6 commits December 3, 2025 04:49

Remove stateless converters from json options

7b398a1

Generated options

b0f5e1e

Hidrate the BiDi property example

79f723d

One more hidrate example

cafbba5

Unified hidration for top level types

d7956b4

Fix name

aaddb1e

nvborisenko requested a review from RenderMichael December 3, 2025 03:30

selenium-ci added the C-dotnet .NET Bindings label Dec 3, 2025

qodo-code-review bot added the Review effort 4/5 label Dec 3, 2025

RenderMichael reviewed Dec 3, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs Outdated Show resolved Hide resolved

RenderMichael reviewed Dec 3, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Broker.cs Outdated Show resolved Hide resolved

nvborisenko marked this pull request as draft December 3, 2025 03:45

nvborisenko added 5 commits December 3, 2025 06:57

Hydrate event args

25389bd

Hydrate AddInterceptResult

61ad320

Hydrate AddDataCollectorResult

2d20a43

Hydrate CreateResult

3f482c0

Hydrate AddPreloadScriptResult

00d499e

nvborisenko marked this pull request as ready for review December 3, 2025 04:07

qodo-code-review bot added the Review effort 3/5 label Dec 3, 2025

nvborisenko commented Dec 3, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs Outdated Show resolved Hide resolved

nvborisenko commented Dec 3, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/BiDi.cs Outdated Show resolved Hide resolved

nvborisenko commented Dec 3, 2025

View reviewed changes

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs Outdated Show resolved Hide resolved

nvborisenko added 4 commits December 3, 2025 13:51

Hydrate InstallResult

7204604

Throw if not hydrated

a465945

Fix property name for exception

4e487df

Don't hydrate primitives except BrowsingContext

1501af1

High level interception is not hydratable

d1a2fcb

nvborisenko added 17 commits December 6, 2025 17:16

Use shared json options

372fac0

Remove generated context options

3aca801

Remove hydration

125f52e

Returned back some primitives

713ad83

Some oothers

5861966

All primitive types are now hydratable

542f49f

Allow public ctor for hydratable primitives

f3ed0a4

Add note for converters

9ab3b93

Moved WebExtensionConverter

cb902a5

Moved CollectorConverter and InterceptConverter

d214de3

Moved PreloadScriptConverter

3bd611a

Moved RealmConverter

b4354d5

Update LogModule.cs

66aa3ff

Moved InternalIdConverter

8235211

Moved HandleConverter

f3e65eb

Moved BrowserUserContextConverter

1ddd37d

All converters as module requires

015d863

nvborisenko changed the title ~~[dotnet] [bidi] Stateless converters with hydration~~ [dotnet] [bidi] Stateful converters with hydration Dec 6, 2025

nvborisenko merged commit fb859ad into SeleniumHQ:trunk Dec 6, 2025
10 checks passed

nvborisenko deleted the bidi-stateless-converters branch December 6, 2025 15:57

RenderMichael reviewed Dec 6, 2025

View reviewed changes

nvborisenko mentioned this pull request Dec 6, 2025

[🚀 Feature]: [dotnet] [bidi] Finalize public API to prevent further breaking changes #16095

Open

12 tasks

Uh oh!

Conversation

nvborisenko commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-code-review bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

Uh oh!

Uh oh!

qodo-code-review bot commented Dec 3, 2025

PR Compliance Guide 🔍

Uh oh!

qodo-code-review bot commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nvborisenko commented Dec 4, 2025

Uh oh!

nvborisenko commented Dec 6, 2025

Uh oh!

Uh oh!

RenderMichael Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

nvborisenko Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

nvborisenko Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nvborisenko commented Dec 3, 2025 •

edited

Loading

qodo-code-review bot commented Dec 3, 2025 •

edited

Loading

qodo-code-review bot commented Dec 3, 2025 •

edited

Loading

qodo-code-review bot commented Dec 3, 2025 •

edited

Loading