[dotnet] [bidi] Statically declared events#17331
Draft
nvborisenko wants to merge 10 commits intoSeleniumHQ:trunkfrom
Draft
[dotnet] [bidi] Statically declared events#17331nvborisenko wants to merge 10 commits intoSeleniumHQ:trunkfrom
nvborisenko wants to merge 10 commits intoSeleniumHQ:trunkfrom
Conversation
Contributor
Review Summary by Qodo[dotnet] [bidi] Statically declare commands and events with descriptors
WalkthroughsDescription• Refactor BiDi commands and events to use static descriptors • Replace dynamic command/event creation with pre-defined static fields • Simplify module implementations by removing factory methods • Update broker to work with command/event descriptors instead of instances File Changes1. dotnet/src/webdriver/BiDi/Command.cs
|
Contributor
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+24
to
+29
| public readonly record struct Command<TParameters, TResult>( | ||
| string Method, | ||
| JsonTypeInfo<TParameters> ParamsTypeInfo, | ||
| JsonTypeInfo<TResult> ResultTypeInfo) | ||
| where TParameters : Parameters | ||
| where TResult : EmptyResult | ||
| { | ||
| [JsonPropertyOrder(2)] | ||
| public TParameters Params { get; } = @params; | ||
| } | ||
| where TResult : EmptyResult; |
Contributor
There was a problem hiding this comment.
1. command base type removed 📘 Rule violation ⚙ Maintainability
The public Command inheritance model was removed and replaced with a readonly record struct descriptor, which breaks consumers that previously derived from Command/`Command<TParameters, TResult>`. This violates the requirement to preserve user-facing API/ABI compatibility.
Agent Prompt
## Issue description
Public `Command` inheritance types were removed, which is a breaking API change for consumers that built custom commands/modules.
## Issue Context
The code now uses a descriptor-style `Command<TParameters, TResult>` record struct, but existing consumers may rely on deriving from `Command`/`Command<TParameters, TResult>`.
## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Command.cs[24-36]
- dotnet/src/webdriver/BiDi/Module.cs[22-44]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
31
| protected Task<TResult> ExecuteAsync<TParameters, TResult>(Command<TParameters, TResult> descriptor, TParameters @params, CommandOptions? options, CancellationToken cancellationToken) | ||
| where TParameters : Parameters | ||
| where TResult : EmptyResult | ||
| { | ||
| return Broker.ExecuteCommandAsync(command, options, jsonCommandTypeInfo, jsonResultTypeInfo, cancellationToken); | ||
| return Broker.ExecuteAsync(descriptor, @params, options, cancellationToken); | ||
| } |
Contributor
There was a problem hiding this comment.
2. executecommandasync removed 📘 Rule violation ⚙ Maintainability
The protected Module.ExecuteCommandAsync(...) API was removed/renamed to ExecuteAsync(...), breaking consumers implementing custom modules derived from Module. This violates the compatibility requirement for user-facing APIs.
Agent Prompt
## Issue description
`Module.ExecuteCommandAsync(...)` was removed, breaking external modules that derived from `Module` and called the protected helper.
## Issue Context
The new descriptor-based execution can still be supported while preserving the old method as an adapter (potentially `[Obsolete]`) for existing consumers.
## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Module.cs[22-44]
- dotnet/src/webdriver/BiDi/Broker.cs[117-165]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| namespace OpenQA.Selenium.BiDi.Browser; | ||
|
|
||
| public sealed class BrowserModule : Module, IBrowserModule | ||
| internal sealed class BrowserModule : Module, IBrowserModule |
Contributor
There was a problem hiding this comment.
3. Public modules made internal 📘 Rule violation ⚙ Maintainability
Multiple previously public *Module concrete types were changed to internal, breaking consumers who referenced these types directly (even if interfaces still exist). This is a user-facing API/ABI break without an indicated compatibility plan.
Agent Prompt
## Issue description
Concrete `*Module` classes were changed from `public` to `internal`, which is a breaking change for any consumers referencing those types directly.
## Issue Context
Even if `BiDi` exposes public interfaces, removing public concrete types breaks source and binary compatibility for advanced usage patterns.
## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Network/NetworkModule.cs[25-26]
- dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Input/InputModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Script/ScriptModule.cs[26-27]
- dotnet/src/webdriver/BiDi/Storage/StorageModule.cs[25-26]
- dotnet/src/webdriver/BiDi/WebExtension/WebExtensionModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Log/LogModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Permissions/PermissionsModule.cs[25-26]
- dotnet/src/webdriver/BiDi/Speculation/SpeculationModule.cs[25-26]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the .NET BiDi refactor to avoid per-call allocations by switching from per-command/per-event classes to statically declared command/event descriptors, and updates the broker/module plumbing accordingly.
Changes:
- Replace command object allocation/serialization with static
Command<TParameters, TResult>descriptors and a newModule.ExecuteAsync/Broker.ExecuteAsyncpipeline. - Replace string-based event subscription metadata with static
Event<TEventArgs, TEventParams>descriptors. - Update module serializer contexts to generate
JsonTypeInfofor parameter/result types (instead of command types), and remove many command wrapper classes.
Reviewed changes
Copilot reviewed 83 out of 83 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/test/webdriver/BiDi/Session/SessionTests.cs | Updates custom-module test usage to the new descriptor-based ExecuteAsync path. |
| dotnet/src/webdriver/BiDi/WebExtension/WebExtensionModule.cs | Converts WebExtension commands to static Command<,> descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/WebExtension/Uninstall.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/WebExtension/Install.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Storage/StorageModule.cs | Converts storage commands to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Storage/SetCookie.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Storage/GetCookies.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Storage/DeleteCookies.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Speculation/SpeculationModule.cs | Converts event subscription to static Event<,> descriptor usage. |
| dotnet/src/webdriver/BiDi/Session/Unsubscribe.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Session/Subscribe.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Session/Status.cs | Removes per-call command wrapper class; keeps result/options types. |
| dotnet/src/webdriver/BiDi/Session/SessionModule.cs | Converts session commands to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Session/New.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Session/End.cs | Removes per-call command wrapper class; keeps result/options types. |
| dotnet/src/webdriver/BiDi/Script/ScriptModule.cs | Converts script commands/events to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Script/RemovePreloadScript.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Script/GetRealms.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Script/Evaluate.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Script/Disown.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Script/CallFunction.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Script/AddPreloadScript.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Permissions/SetPermission.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Permissions/PermissionsModule.cs | Converts permissions command to static descriptor and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Network/SetExtraHeaders.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/SetCacheBehavior.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/RemoveIntercept.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/RemoveDataCollector.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/ProvideResponse.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/NetworkModule.cs | Converts network commands/events to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Network/GetData.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/FailRequest.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/ContinueWithAuth.cs | Removes per-call command wrapper class; keeps polymorphic parameter type hierarchy. |
| dotnet/src/webdriver/BiDi/Network/ContinueResponse.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/ContinueRequest.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/AddIntercept.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Network/AddDataCollector.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Module.cs | Replaces protected execution/subscription API with descriptor-based overloads. |
| dotnet/src/webdriver/BiDi/Log/LogModule.cs | Converts log event subscription to static Event<,> descriptor usage. |
| dotnet/src/webdriver/BiDi/Input/SetFiles.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Input/ReleaseActions.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Input/PerformActions.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Input/InputModule.cs | Converts input commands/event to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Event.cs | Introduces Event<TEventArgs, TEventParams> descriptor type for static event metadata. |
| dotnet/src/webdriver/BiDi/Emulation/SetUserAgentOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetTouchOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetTimezoneOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetScrollbarTypeOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetScriptingEnabled.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetScreenSettingsOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetScreenOrientationOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetNetworkConditions.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetLocaleOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/SetGeolocationOverride.cs | Removes per-call command wrapper class; keeps polymorphic parameter type hierarchy. |
| dotnet/src/webdriver/BiDi/Emulation/SetForcedColorsModeThemeOverride.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Emulation/EmulationModule.cs | Converts emulation commands to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/Command.cs | Replaces class-based command model with Command<TParameters, TResult> descriptor and keeps Parameters/EmptyResult. |
| dotnet/src/webdriver/BiDi/BrowsingContext/TraverseHistory.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/SetViewport.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Reload.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Print.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Navigate.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/LocateNodes.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/HandleUserPrompt.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/GetTree.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Create.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Close.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/CaptureScreenshot.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextStorageModule.cs | Makes browsing-context storage module implementation internal. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextScriptModule.cs | Makes browsing-context script module implementation internal. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs | Makes browsing-context network module implementation internal. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs | Converts browsingContext commands/events to static descriptors and updates JSON context registrations. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs | Makes browsing-context log module implementation internal. |
| dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextInputModule.cs | Makes browsing-context input module implementation internal. |
| dotnet/src/webdriver/BiDi/BrowsingContext/Activate.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Browser/RemoveUserContext.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Browser/GetUserContexts.cs | Removes per-call command wrapper class; keeps result/options types. |
| dotnet/src/webdriver/BiDi/Browser/GetClientWindows.cs | Removes per-call command wrapper class; keeps result/options types. |
| dotnet/src/webdriver/BiDi/Browser/CreateUserContext.cs | Removes per-call command wrapper class; keeps parameter/options/result types. |
| dotnet/src/webdriver/BiDi/Browser/Close.cs | Removes per-call command wrapper class; keeps result/options types. |
| dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs | Converts browser commands to static descriptors, updates JSON context registrations, and changes visibility. |
| dotnet/src/webdriver/BiDi/Broker.cs | Updates execution/subscription plumbing to use descriptors; writes command JSON envelopes manually; adjusts pending-command removal semantics. |
Comment on lines
23
to
26
| namespace OpenQA.Selenium.BiDi.Browser; | ||
|
|
||
| public sealed class BrowserModule : Module, IBrowserModule | ||
| internal sealed class BrowserModule : Module, IBrowserModule | ||
| { |
There was a problem hiding this comment.
This changes BrowserModule from public to internal, which is a breaking API change (and also prevents use via IBiDi.AsModule<T>() for built-in modules). The PR description labels this as a non-breaking change; either keep the type public (possibly hidden/obsolete) or update the PR scope/notes to reflect a breaking change and follow the repo deprecation policy.
Comment on lines
+26
to
31
| protected Task<TResult> ExecuteAsync<TParameters, TResult>(Command<TParameters, TResult> descriptor, TParameters @params, CommandOptions? options, CancellationToken cancellationToken) | ||
| where TParameters : Parameters | ||
| where TResult : EmptyResult | ||
| { | ||
| return Broker.ExecuteCommandAsync(command, options, jsonCommandTypeInfo, jsonResultTypeInfo, cancellationToken); | ||
| return Broker.ExecuteAsync(descriptor, @params, options, cancellationToken); | ||
| } |
There was a problem hiding this comment.
Module is a public base type and this change replaces the protected ExecuteCommandAsync/SubscribeAsync extensibility surface. Any external Module subclasses (or custom commands/events built on the previous API) will no longer compile. Consider keeping the old protected overloads as deprecated shims delegating to the new descriptor-based methods to preserve API/ABI compatibility.
Comment on lines
+24
to
28
| public readonly record struct Command<TParameters, TResult>( | ||
| string Method, | ||
| JsonTypeInfo<TParameters> ParamsTypeInfo, | ||
| JsonTypeInfo<TResult> ResultTypeInfo) | ||
| where TParameters : Parameters |
There was a problem hiding this comment.
The previous public Command inheritance model is removed in favor of a different Command<,> descriptor type. This is a breaking change for consumers who implemented custom commands (and any code referencing the old base classes). Consider retaining the old classes as deprecated wrappers/adapters so existing custom command implementations continue to work.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of #17330
🔗 Related Issues
Contributes to #16095
💥 What does this PR do?
....
🔄 Types of changes