Skip to content

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 6, 2025

User description

This enables cross-module use of these types, as well as {de}serialization by users.


PR Type

Enhancement


Description

  • Move JSON converter attributes from centralized Broker to individual types

  • Enable cross-module serialization and user-facing deserialization

  • Add JsonConverter attributes to 20+ BiDi types

  • Remove converter registrations from centralized options


Diagram Walkthrough

flowchart LR
  A["Centralized Broker"] -- "Remove converters" --> B["Individual Types"]
  B -- "Add JsonConverter attributes" --> C["Cross-module Usage"]
  C -- "Enable" --> D["User Serialization"]
Loading

File Walkthrough

Relevant files
Enhancement
20 files
ClientWindow.cs
Add JsonConverter attribute to ClientWindow                           
+4/-0     
GetClientWindowsCommand.cs
Add JsonConverter to GetClientWindowsResult                           
+3/-0     
GetUserContextsCommand.cs
Add JsonConverter to GetUserContextsResult                             
+3/-0     
DownloadEndEventArgs.cs
Add JsonConverter to DownloadEndEventArgs                               
+3/-0     
GetTreeCommand.cs
Add JsonConverter to GetTreeResult                                             
+3/-0     
LocateNodesCommand.cs
Add JsonConverter to LocateNodesResult                                     
+3/-0     
Navigation.cs
Add JsonConverter attribute to Navigation                               
+4/-0     
PrintCommand.cs
Add JsonConverter to PrintPageRange                                           
+1/-0     
Broker.cs
Remove centralized JSON converter registrations                   
+1/-25   
Origin.cs
Add JsonConverter attribute to Origin                                       
+4/-0     
SourceActions.cs
Add JsonConverter to SourceActions                                             
+2/-0     
LogEntry.cs
Add JsonConverter attribute to LogEntry                                   
+2/-0     
Request.cs
Add JsonConverter attribute to Request                                     
+4/-0     
Channel.cs
Add JsonConverter attribute to Channel                                     
+4/-0     
EvaluateCommand.cs
Add JsonConverter to EvaluateResult                                           
+3/-0     
GetRealmsCommand.cs
Add JsonConverter to GetRealmsResult                                         
+3/-0     
RealmInfo.cs
Add JsonConverter attribute to RealmInfo                                 
+3/-0     
RemoteValue.cs
Add JsonConverter to RemoteValue                                                 
+2/-1     
Subscription.cs
Add JsonConverter attribute to Subscription                           
+4/-0     
GetCookiesCommand.cs
Add JsonConverter to GetCookiesResult                                       
+2/-0     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Oct 6, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 6, 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
🎫 No ticket provided
  • Create ticket/issue
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.

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

Copy link
Contributor

qodo-merge-pro bot commented Oct 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Add constructor null checks

Guard constructor parameters against null and validate url to fail fast. Throw
ArgumentNullException when inputs are invalid.

dotnet/src/webdriver/BiDi/Communication/Broker.cs [58-88]

 internal Broker(BiDi bidi, Uri url)
 {
+    if (bidi is null) throw new ArgumentNullException(nameof(bidi));
+    if (url is null) throw new ArgumentNullException(nameof(url));
+
     _bidi = bidi;
     _transport = new WebSocketTransport(url);
 
     var jsonSerializerOptions = 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(_bidi),
             new BrowserUserContextConverter(_bidi),
             new CollectorConverter(_bidi),
             new InterceptConverter(_bidi),
             new HandleConverter(_bidi),
             new InternalIdConverter(_bidi),
             new PreloadScriptConverter(_bidi),
             new RealmConverter(_bidi),
             new DateTimeOffsetConverter(),
             new WebExtensionConverter(_bidi),
         }
     };
 
     _jsonSerializerContext = new BiDiJsonSerializerContext(jsonSerializerOptions);
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and object state early to prevent null dereferences and subtle logic errors.

Low
  • More

Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

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

Thanks. I am still don't know what to do with contextual converters (BiDi instance in ctor).

@RenderMichael RenderMichael merged commit 04a927e into SeleniumHQ:trunk Oct 6, 2025
13 checks passed
@RenderMichael RenderMichael deleted the bidi-json-converters branch October 6, 2025 20:50
@RenderMichael
Copy link
Contributor Author

I am still don't know what to do with contextual converters (BiDi instance in ctor).

@nvborisenko I have some ideas, I will implement shortly

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