Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 9, 2025

User description

Recently bidi specification started to use specific command result type for all commands, instead of just EmptyResult. And it is good.

Contributes to #16095

💥 What does this PR do?

Add result types for all commands, avoid EmptyResult.

🔧 Implementation Notes

CallFunctionAsync still returns EvaluateResult, I cannot translate spec to .NET in elegant way.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

  • Replace EmptyResult with specific result types for all BiDi commands

  • Add new result classes inheriting from EmptyResult for type safety

  • Update JSON serialization context for new result types

  • Maintain backward compatibility through inheritance


Diagram Walkthrough

flowchart LR
  EmptyResult["EmptyResult (base)"] --> SpecificResults["Specific Result Types"]
  Commands["BiDi Commands"] --> SpecificResults
  SpecificResults --> JsonContext["JSON Serialization Context"]
  SpecificResults --> TypeSafety["Enhanced Type Safety"]
Loading

File Walkthrough

Relevant files
Enhancement
46 files
BrowserModule.cs
Replace EmptyResult with specific Browser result types     
+12/-12 
CloseCommand.cs
Add CloseResult type for browser close command                     
+3/-1     
CreateUserContextCommand.cs
Add CreateUserContextResult inheriting from UserContextInfo
+3/-1     
RemoveUserContextCommand.cs
Add RemoveUserContextResult for remove command                     
+3/-1     
SetDownloadBehaviorCommand.cs
Add SetDownloadBehaviorResult for download behavior           
+3/-1     
UserContextInfo.cs
Change UserContextInfo from sealed to base record               
+1/-1     
ActivateCommand.cs
Add ActivateResult for browsing context activation             
+3/-1     
BrowsingContext.cs
Update method signatures with specific result types           
+5/-5     
BrowsingContextInputModule.cs
Replace EmptyResult with specific Input result types         
+3/-3     
BrowsingContextModule.cs
Update all browsing context methods with specific results
+10/-10 
BrowsingContextNetworkModule.cs
Add SetCacheBehaviorResult for network cache behavior       
+1/-1     
CloseCommand.cs
Add CloseResult for browsing context close                             
+3/-1     
HandleUserPromptCommand.cs
Add HandleUserPromptResult for user prompt handling           
+3/-1     
NavigateCommand.cs
Change NavigateResult from sealed to base record                 
+1/-1     
ReloadCommand.cs
Add ReloadResult inheriting from NavigateResult                   
+3/-1     
SetViewportCommand.cs
Add SetViewportResult for viewport configuration                 
+3/-1     
EmulationModule.cs
Replace EmptyResult with specific Emulation result types 
+18/-18 
SetForcedColorsModeThemeOverrideCommand.cs
Add SetForcedColorsModeThemeOverrideResult                             
+3/-1     
SetGeolocationOverrideCommand.cs
Add SetGeolocationOverrideResult for geolocation commands
+3/-1     
SetLocaleOverrideCommand.cs
Add SetLocaleOverrideResult for locale override                   
+3/-1     
SetScreenOrientationOverrideCommand.cs
Add SetScreenOrientationOverrideResult                                     
+3/-1     
SetScriptingEnabledCommand.cs
Add SetScriptingEnabledResult for scripting control           
+3/-1     
SetTimezoneOverrideCommand.cs
Add SetTimezoneOverrideResult for timezone override           
+3/-1     
SetUserAgentOverrideCommand.cs
Add SetUserAgentOverrideResult for user agent                       
+3/-1     
InputModule.cs
Replace EmptyResult with specific Input result types         
+6/-6     
PerformActionsCommand.cs
Add PerformActionsResult for action performance                   
+3/-1     
ReleaseActionsCommand.cs
Add ReleaseActionsResult for action release                           
+3/-1     
SetFilesCommand.cs
Add SetFilesResult for file input handling                             
+3/-1     
ContinueRequestCommand.cs
Add ContinueRequestResult for request continuation             
+3/-1     
ContinueResponseCommand.cs
Add ContinueResponseResult for response continuation         
+3/-1     
ContinueWithAuthCommand.cs
Add ContinueWithAuthResult for authentication handling     
+2/-1     
FailRequestCommand.cs
Add FailRequestResult for request failure                               
+3/-1     
NetworkModule.cs
Replace EmptyResult with specific Network result types     
+22/-22 
ProvideResponseCommand.cs
Add ProvideResponseResult for response provision                 
+3/-1     
RemoveDataCollectorCommand.cs
Add RemoveDataCollectorResult for data collector removal 
+3/-1     
RemoveInterceptCommand.cs
Add RemoveInterceptResult for intercept removal                   
+3/-1     
SetCacheBehaviorCommand.cs
Add SetCacheBehaviorResult for cache behavior                       
+3/-1     
SetExtraHeadersCommand.cs
Add SetExtraHeadersResult for header configuration             
+3/-1     
DisownCommand.cs
Add DisownResult and DisownOptions for script disown         
+5/-1     
RemovePreloadScriptCommand.cs
Add RemovePreloadScriptResult for script removal                 
+3/-1     
ScriptModule.cs
Add DisownAsync method and update result types                     
+10/-2   
EndCommand.cs
Add EndResult for session termination                                       
+3/-1     
SessionModule.cs
Replace EmptyResult with specific Session result types     
+4/-4     
UnsubscribeCommand.cs
Add UnsubscribeResult for subscription management               
+3/-1     
UninstallCommand.cs
Add UninstallResult for extension uninstallation                 
+3/-1     
WebExtensionModule.cs
Replace EmptyResult with UninstallResult                                 
+2/-2     
Formatting
1 files
GetClientWindowsCommand.cs
Remove extra whitespace formatting cleanup                             
+0/-2     
Configuration changes
1 files
BiDiJsonSerializerContext.cs
Add JSON serialization for all new result types                   
+35/-1   
Tests
1 files
BrowserTest.cs
Update tests to handle new result types                                   
+12/-8   

Copy link
Contributor

qodo-merge-pro bot commented Oct 9, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #16095
🟢 Commands should return their specific Result types instead of a single object inside
Result; review all commands and return exact type.
🔴 Use EmptyResult as a valid return type for all methods instead of void.
Expose BiDi via AnyDriver/IWebDriver AsBiDiAsync and manage lifecycle/ownership.
Make hidden/internal methods like AddIntercept public.
Revisit InterceptRequestAsync and related helpers as extensions in a dedicated namespace.
Use normal types as event args (inheritance/topic).
Reduce overuse of syntax sugar like Result implementing IReadOnlyList; prefer extensions
and remove special JSON converters.
Command options should be immutable (construct once, no state changes).
Anything else follow-ups and fine tuning.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

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

Copy link
Contributor

qodo-merge-pro bot commented Oct 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify test by removing one context

Simplify the CanRemoveUserContext test by creating, removing, and then verifying
the removal of a single user context, rather than using two.

dotnet/test/common/BiDi/Browser/BrowserTest.cs [56-64]

-var userContext1 = await bidi.Browser.CreateUserContextAsync();
-var userContext2 = await bidi.Browser.CreateUserContextAsync();
+var userContext = await bidi.Browser.CreateUserContextAsync();
 
-await userContext2.UserContext.RemoveAsync();
+await userContext.UserContext.RemoveAsync();
 
 var userContexts = (await bidi.Browser.GetUserContextsAsync()).Select(uc => uc.UserContext);
 
-Assert.That(userContexts, Does.Contain(userContext1.UserContext));
-Assert.That(userContexts, Does.Not.Contain(userContext2.UserContext));
+Assert.That(userContexts, Does.Not.Contain(userContext.UserContext));

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly proposes simplifying the test logic for CanRemoveUserContext to focus on its primary purpose, which improves the test's clarity and maintainability.

Low
Learned
best practice
Clarify unused options purpose

Mark DisownOptions as used by ensuring the command path supports options and
mirror other commands’ patterns with a consistent public options class placement
or XML doc to avoid confusion.

dotnet/src/webdriver/BiDi/Script/DisownCommand.cs [28-32]

-internal sealed record DisownParameters(IEnumerable<Handle> Handles, Target Target) : Parameters;
-
+// Options remain for consistency with other commands; add summary to clarify usage.
+/// <summary>Options for script.disown (reserved for future extensibility).</summary>
 public sealed class DisownOptions : CommandOptions;
 
-public sealed record DisownResult : EmptyResult;
-
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Keep API and documentation accurate and consistent by aligning parameter names and annotations with interfaces; provide options parameter consistently across module methods.

Low
  • Update

@nvborisenko nvborisenko merged commit 8c4302c into SeleniumHQ:trunk Oct 9, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-result-type branch October 9, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants