Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Nov 15, 2025

User description

Get rid of BiDi/Communication folder.

Simplifies things.

💡 Additional Considerations

Not sure what to do with BiDi/Json folder.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Eliminate BiDi/Communication namespace hierarchy

  • Move communication classes to BiDi root namespace

  • Reorganize Json converters under BiDi/Json namespace

  • Update all import statements across 100+ files


Diagram Walkthrough

flowchart LR
  A["BiDi/Communication<br/>namespace hierarchy"] -->|"Flatten"| B["BiDi root<br/>namespace"]
  C["BiDi/Communication/Json<br/>converters"] -->|"Relocate"| D["BiDi/Json<br/>namespace"]
  B -->|"Contains"| E["Broker, Command,<br/>EventHandler"]
  D -->|"Contains"| F["Converters,<br/>Internal utilities"]
Loading

File Walkthrough

Relevant files
Refactoring
20 files
BiDi.cs
Update namespace imports for Json converters                         
+2/-3     
Broker.cs
Move Broker to BiDi root namespace                                             
+1/-2     
Command.cs
Move Command base class to BiDi namespace                               
+1/-2     
CommandOptions.cs
Move CommandOptions to BiDi namespace                                       
+1/-1     
EventHandler.cs
Move EventHandler to BiDi namespace                                           
+1/-1     
ITransport.cs
Move ITransport interface to BiDi namespace                           
+1/-1     
WebSocketTransport.cs
Move WebSocketTransport to BiDi namespace                               
+4/-4     
BiDiJsonSerializerContext.cs
Update namespace to BiDi.Json                                                       
+1/-1     
BrowserClientWindowConverter.cs
Update namespace to BiDi.Json.Converters                                 
+1/-1     
CamelCaseEnumConverter.cs
Update namespace to BiDi.Json.Converters                                 
+1/-1     
InputSourceActionsConverter.cs
Update namespace and imports for Json converters                 
+2/-2     
DownloadEndEventArgsConverter.cs
Update namespace to BiDi.Json.Converters.Polymorphic         
+2/-2     
JsonExtensions.cs
Update namespace to BiDi.Json.Internal                                     
+1/-1     
BrowserModule.cs
Remove Communication namespace import                                       
+0/-1     
ClientWindow.cs
Update Json.Converters import path                                             
+1/-1     
BrowsingContext.cs
Remove Communication namespace import                                       
+1/-2     
Module.cs
Update namespace imports for Json                                               
+1/-2     
Subscription.cs
Update EventHandler reference to root namespace                   
+3/-3     
RemoteValue.cs
Update Json converters import paths                                           
+2/-2     
AddDataCollectorCommand.cs
Update Json.Converters import path                                             
+1/-2     
Additional files
101 files
ClientWindowInfo.cs +1/-1     
CloseCommand.cs +0/-2     
CreateUserContextCommand.cs +0/-2     
GetClientWindowsCommand.cs +0/-1     
GetUserContextsCommand.cs +0/-1     
RemoveUserContextCommand.cs +0/-2     
SetDownloadBehaviorCommand.cs +0/-1     
UserContextInfo.cs +0/-2     
ActivateCommand.cs +0/-2     
BrowsingContextModule.cs +0/-1     
CaptureScreenshotCommand.cs +1/-2     
CloseCommand.cs +0/-2     
CreateCommand.cs +1/-2     
DownloadEndEventArgs.cs +1/-1     
GetTreeCommand.cs +0/-1     
HandleUserPromptCommand.cs +0/-2     
LocateNodesCommand.cs +0/-1     
Locator.cs +1/-1     
NavigateCommand.cs +1/-2     
Navigation.cs +1/-1     
PrintCommand.cs +1/-2     
ReloadCommand.cs +0/-2     
SetViewportCommand.cs +0/-2     
TraverseHistoryCommand.cs +0/-2     
UserPromptOpenedEventArgs.cs +1/-1     
EmulationModule.cs +0/-1     
SetForcedColorsModeThemeOverrideCommand.cs +1/-2     
SetGeolocationOverrideCommand.cs +0/-1     
SetLocaleOverrideCommand.cs +0/-1     
SetScreenOrientationOverrideCommand.cs +1/-2     
SetScriptingEnabledCommand.cs +0/-1     
SetTimezoneOverrideCommand.cs +0/-1     
SetUserAgentOverrideCommand.cs +0/-1     
InputModule.cs +0/-1     
Origin.cs +1/-1     
PerformActionsCommand.cs +0/-1     
ReleaseActionsCommand.cs +0/-2     
SetFilesCommand.cs +0/-1     
SourceActions.cs +2/-2     
BrowserUserContextConverter.cs +1/-1     
BrowsingContextConverter.cs +1/-1     
ChannelConverter.cs +1/-1     
CollectorConverter.cs +1/-1     
DateTimeOffsetConverter.cs +1/-1     
HandleConverter.cs +1/-1     
InputOriginConverter.cs +1/-1     
InterceptConverter.cs +1/-1     
InternalIdConverter.cs +1/-1     
KebabCaseEnumConverter.cs +1/-1     
NavigationConverter.cs +1/-1     
EvaluateResultConverter.cs +2/-2     
LogEntryConverter.cs +2/-2     
RealmInfoConverter.cs +2/-2     
RemoteValueConverter.cs +2/-2     
PreloadScriptConverter.cs +1/-1     
PrintPageRangeConverter.cs +1/-1     
RealmConverter.cs +1/-1     
RequestConverter.cs +1/-1     
SpecialNumberConverter.cs +1/-1     
SubscriptionConverter.cs +1/-1     
WebExtensionConverter.cs +1/-1     
LogEntry.cs +2/-2     
LogModule.cs +1/-2     
AddInterceptCommand.cs +1/-2     
ContinueRequestCommand.cs +0/-1     
ContinueResponseCommand.cs +0/-1     
ContinueWithAuthCommand.cs +0/-1     
Cookie.cs +1/-1     
FailRequestCommand.cs +0/-2     
GetDataCommand.cs +0/-2     
Initiator.cs +1/-1     
NetworkModule.cs +0/-1     
ProvideResponseCommand.cs +0/-1     
RemoveDataCollectorCommand.cs +0/-2     
RemoveInterceptCommand.cs +0/-2     
Request.cs +1/-1     
SetCacheBehaviorCommand.cs +1/-2     
SetExtraHeadersCommand.cs +0/-1     
AddPreloadScriptCommand.cs +0/-1     
CallFunctionCommand.cs +0/-1     
Channel.cs +1/-1     
DisownCommand.cs +0/-1     
EvaluateCommand.cs +1/-2     
GetRealmsCommand.cs +0/-1     
LocalValue.cs +1/-1     
RealmInfo.cs +1/-1     
RealmType.cs +1/-1     
RemovePreloadScriptCommand.cs +0/-2     
ResultOwnership.cs +1/-1     
ScriptModule.cs +0/-1     
SerializationOptions.cs +1/-1     
EndCommand.cs +0/-2     
NewCommand.cs +0/-2     
SessionModule.cs +0/-1     
StatusCommand.cs +0/-2     
SubscribeCommand.cs +0/-1     
Subscription.cs +1/-1     
UnsubscribeCommand.cs +0/-1     
UserPromptHandler.cs +1/-1     
DeleteCookiesCommand.cs +0/-2     
Additional files not shown

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 15, 2025

PR Compliance Guide 🔍

(Compliance updated until commit eb3db9d)

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
🟢
Generic: Meaningful Naming and Self-Documenting Code

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

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

Status:
Audit logging: The changes largely refactor namespaces and adjust unsubscribe logic, and there is no
evidence in the new code lines that critical actions (e.g., subscribe/unsubscribe
operations) are being logged with user, timestamp, and outcome, but existing logging may
exist outside the shown diff.

Referred Code
public async Task UnsubscribeAsync(Subscription subscription)
{
    var eventHandlers = _eventHandlers[subscription.EventHandler.EventName];

    eventHandlers.Remove(subscription.EventHandler);

    await _bidi.SessionModule.UnsubscribeAsync([subscription.SubscriptionId]).ConfigureAwait(false);
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

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

Status:
Error handling: New imports and namespace changes are introduced but the diff does not show added
try/catch or contextual error handling for transport operations; however, full method
bodies are not included, so adequacy of error handling cannot be determined from the new
lines alone.

Referred Code
using OpenQA.Selenium.Internal.Logging;
using System;
using System.IO;
using System.Net.WebSockets;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace OpenQA.Selenium.BiDi;

class WebSocketTransport(Uri _uri) : ITransport, IDisposable
{

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

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

Status:
User-facing errors: The new code does not introduce user-facing error messages, but without visibility into
surrounding error paths it is unclear whether detailed internal errors might be exposed
elsewhere after these refactors.

Referred Code
using System.Threading;
using System.Threading.Tasks;

namespace OpenQA.Selenium.BiDi;

public sealed class Broker : IAsyncDisposable
{

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

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

Status:
Input validation: The changes primarily relocate JSON converters and namespaces without adding or removing
validation; security of input handling depends on converter implementations not shown
modified in this diff.

Referred Code
using System.Text.Json;
using System.Text.Json.Serialization;

namespace OpenQA.Selenium.BiDi.Json.Converters;

// Serializes and deserializes double into a BiDi spec-compliant number value.
// See https://w3c.github.io/webdriver-bidi/#type-script-PrimitiveProtocolValue

Learn more about managing compliance generic rules or creating your own custom rules

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

Previous compliance checks

Compliance check up to commit 892902a
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
🟢
Generic: Meaningful Naming and Self-Documenting Code

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

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

Status:
Audit logging: The refactor adds or changes namespaces and transports without adding audit logs for
critical actions (e.g., transport open/close, send/receive) making it unclear whether
audit trail requirements are still met.

Referred Code
using OpenQA.Selenium.Internal.Logging;
using System;
using System.IO;
using System.Net.WebSockets;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace OpenQA.Selenium.BiDi;

class WebSocketTransport(Uri _uri) : ITransport, IDisposable
{

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

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

Status:
Error handling: Interface and transport refactors show no new error handling or context for failures in
connect/IO operations, so verification is needed to ensure exceptions are still properly
handled and logged elsewhere.

Referred Code
using System.Threading;
using System;

namespace OpenQA.Selenium.BiDi;

interface ITransport : IDisposable
{

Learn more about managing compliance generic rules or creating your own custom rules

@nvborisenko
Copy link
Member Author

Now or never. I always don't like Communication folder.

cc @YevgeniyShunevych @RenderMichael

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Reconsider moving internal communication classes

Avoid moving internal communication classes like Broker and WebSocketTransport
into the public OpenQA.Selenium.BiDi namespace. Instead, they should reside in a
distinct internal namespace to keep the public API clean.

Examples:

dotnet/src/webdriver/BiDi/Broker.cs [30-32]
namespace OpenQA.Selenium.BiDi;

public sealed class Broker : IAsyncDisposable
dotnet/src/webdriver/BiDi/WebSocketTransport.cs [28-30]

Solution Walkthrough:

Before:

// File: dotnet/src/webdriver/BiDi/Broker.cs
namespace OpenQA.Selenium.BiDi;

public sealed class Broker : IAsyncDisposable
{
    // ... implementation ...
}

// File: dotnet/src/webdriver/BiDi/WebSocketTransport.cs
namespace OpenQA.Selenium.BiDi;

class WebSocketTransport(Uri _uri) : ITransport, IDisposable
{
    // ... implementation ...
}

After:

// File: dotnet/src/webdriver/BiDi/Communication/Broker.cs
namespace OpenQA.Selenium.BiDi.Communication;

// Consider making this class internal if not used publicly
public sealed class Broker : IAsyncDisposable
{
    // ... implementation ...
}

// File: dotnet/src/webdriver/BiDi/Communication/WebSocketTransport.cs
namespace OpenQA.Selenium.BiDi.Communication;

internal class WebSocketTransport(Uri _uri) : ITransport, IDisposable
{
    // ... implementation ...
}
Suggestion importance[1-10]: 9

__

Why: This is a critical architectural suggestion that correctly points out that the PR pollutes the public API namespace with internal implementation classes like Broker, which is a significant design regression.

High
General
Use a type alias to resolve ambiguity

Use a type alias for EventHandler to resolve potential ambiguity with
System.EventHandler and improve code clarity.

dotnet/src/webdriver/BiDi/Subscription.cs [31-37]

-private readonly EventHandler _eventHandler;
+private readonly BiDiEventHandler _eventHandler;
 
-internal Subscription(Session.Subscription subscription, Broker broker, EventHandler eventHandler)
+internal Subscription(Session.Subscription subscription, Broker broker, BiDiEventHandler eventHandler)
 {
     _subscription = subscription;
     _broker = broker;
     _eventHandler = eventHandler;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential type name ambiguity between OpenQA.Selenium.BiDi.EventHandler and System.EventHandler, proposing a type alias for clarity, which is a valid code improvement for maintainability.

Low
  • Update

@YevgeniyShunevych
Copy link
Contributor

Generally, @nvborisenko, I support such changes that reduce the number of namespaces.

@nvborisenko
Copy link
Member Author

In general I also like this simplification. No mistic Communication folder (or it had been Transport, or Protocol?. No matters).

Now we have only one mistic Json folder, but it is OK for now. Merging this PR earlier, before module extensions are public.

@nvborisenko nvborisenko merged commit e535ce2 into SeleniumHQ:trunk Nov 16, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-get-rid-of-communication branch November 16, 2025 11:19
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