[dotnet] [bidi] Hide Broker as internal implementation

[nvborisenko]

User description

This pull request refactors the Broker and BrowserModule classes in the BiDi WebDriver implementation, focusing on improving encapsulation and simplifying the command execution interface. The most significant changes include making the Broker class internal, consolidating subscription methods, and updating the BrowserModule to use its own command execution method instead of delegating to the Broker.

💥 What does this PR do?

Encapsulation and API Surface Changes

• Changed the visibility of the Broker class from public to internal to restrict its usage to within the assembly, improving encapsulation.

Command Execution Refactoring

• Updated all command execution calls in BrowserModule to use its own ExecuteCommandAsync method instead of calling Broker.ExecuteCommandAsync, simplifying the module's interface and reducing coupling.

Subscription API Simplification

• Removed overloads of the SubscribeAsync method in Broker that accepted delegates (Action<TEventArgs> and Func<TEventArgs, Task>), consolidating to a single version that takes an EventHandler. This reduces API surface and potential confusion.

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Enhancement

---

Description

• Made Broker class internal to restrict public API surface

• Consolidated subscription methods in Broker by removing delegate overloads

• Added protected helper methods in Module base class for command execution and subscriptions

• Updated all module implementations to use inherited methods instead of direct Broker calls

• Fixed typo: "occured" to "occurred" in error messages

---

Diagram Walkthrough

flowchart LR
  A["Broker class<br/>public → internal"] -->|delegates to| B["Module base class<br/>protected methods"]
  B -->|ExecuteCommandAsync| C["All Module subclasses<br/>use inherited methods"]
  B -->|SubscribeAsync| C
  D["Removed delegate<br/>overloads"] -->|consolidated to| E["Single EventHandler<br/>based method"]

Loading

File Walkthrough

	Relevant files
Encapsulation	1 files Broker.cs Made Broker internal and consolidated subscription API      +4/-18
Refactoring	12 files Module.cs Added protected helper methods for command execution          +25/-1    BrowserModule.cs Updated to use inherited ExecuteCommandAsync method            +8/-8      BrowsingContextModule.cs Replaced Broker calls with inherited module methods            +40/-40  EmulationModule.cs Updated command execution to use module methods                    +10/-10  InputModule.cs Replaced Broker method calls with inherited methods            +5/-5      LogModule.cs Updated subscription calls to use module methods                  +2/-2      NetworkModule.cs Replaced all Broker calls with inherited methods                  +24/-24  PermissionsModule.cs Updated to use inherited ExecuteCommandAsync method            +1/-1      ScriptModule.cs Replaced Broker method calls with inherited methods            +12/-12  SessionModule.cs Updated command execution to use module methods                    +5/-5      StorageModule.cs Replaced Broker calls with inherited methods                          +3/-3      WebExtensionModule.cs Updated to use inherited ExecuteCommandAsync method            +2/-2

---

[qodo-code-review]

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
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation 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: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Exception detail logged: New error logs interpolate ex directly, which may include stack traces and sensitive details depending on exception content and where logs are surfaced. Referred Code _logger.Error($"Unhandled error occurred while processing remote message: {ex}"); } } } } catch (Exception ex) when (ex is not OperationCanceledException) { if (_logger.IsEnabled(LogEventLevel.Error)) { _logger.Error($"Unhandled error occurred while receiving remote messages: {ex}"); } 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: Possible sensitive logs: The new _logger.Error(...) messages include full exception rendering ({ex}), which can inadvertently log sensitive data contained in exception messages/stack traces. Referred Code _logger.Error($"Unhandled error occurred while processing remote message: {ex}"); } } } } catch (Exception ex) when (ex is not OperationCanceledException) { if (_logger.IsEnabled(LogEventLevel.Error)) { _logger.Error($"Unhandled error occurred while receiving remote messages: {ex}"); } 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

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Relax result type constraint Relax the generic constraint on ExecuteCommandAsync from where TResult : EmptyResult to where TResult : class to allow all valid result types and fix a compilation error. dotnet/src/webdriver/BiDi/Module.cs [33-35] protected Task<TResult> ExecuteCommandAsync<TCommand, TResult>(TCommand command, CommandOptions? options, JsonTypeInfo<TCommand> jsonCommandTypeInfo, JsonTypeInfo<TResult> jsonResultTypeInfo) where TCommand : Command - where TResult : EmptyResult + where TResult : class Apply / Chat Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical compilation error. The generic constraint where TResult : EmptyResult is incorrect for most command results, and relaxing it is necessary for the code to compile and function as intended.	High
	Fix array literal syntax Replace the invalid array literal syntax [eventName] with a valid array creation expression, such as new[] { eventName }, to fix a compilation error. dotnet/src/webdriver/BiDi/Broker.cs [155] -var subscribeResult = await _bidi.SessionModule.SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }).ConfigureAwait(false); +var subscribeResult = await _bidi.SessionModule.SubscribeAsync(new[] { eventName }, new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }).ConfigureAwait(false); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies invalid C# syntax ([eventName]) that will cause a compilation failure. The proposed fix to use new[] { eventName } is the correct way to create an array inline and is essential for the code to compile.	High
	Synchronize handler addition Wrap the addition of an event handler to the handlers list within a lock statement to prevent race conditions and ensure thread-safe modification. dotnet/src/webdriver/BiDi/Broker.cs [153-157] -var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []); +var handlers = _eventHandlers.GetOrAdd(eventName, _ => new List<EventHandler>()); ... -handlers.Add(eventHandler); +lock (handlers) +{ + handlers.Add(eventHandler); +} [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential race condition when adding event handlers from multiple threads. Using a lock is a valid and important fix to ensure thread safety and prevent data corruption in a concurrent environment.	Medium
Learned best practice	Validate inputs before use Validate eventName for null/whitespace and validate the delegate/type info arguments for null before constructing handlers and calling into Broker to harden the API boundary. dotnet/src/webdriver/BiDi/Module.cs [40-52] protected Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { + ArgumentException.ThrowIfNullOrWhiteSpace(eventName); + ArgumentNullException.ThrowIfNull(action); + ArgumentNullException.ThrowIfNull(jsonTypeInfo); + var eventHandler = new SyncEventHandler<TEventArgs>(eventName, action); return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { + ArgumentException.ThrowIfNullOrWhiteSpace(eventName); + ArgumentNullException.ThrowIfNull(func); + ArgumentNullException.ThrowIfNull(jsonTypeInfo); + var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func); return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Add validation and null/blank guards at integration boundaries before using inputs (e.g., event names, handlers) to prevent unexpected runtime errors.	Low
General	Make subscription helper method protected Change the SubscribeAsync method that accepts a Func<TEventArgs, Task> from public to protected to match its Action counterpart, ensuring consistent encapsulation. dotnet/src/webdriver/BiDi/Module.cs [47-52] -public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) +protected Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo) where TEventArgs : EventArgs { var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func); return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo); } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies an access modifier inconsistency between two similar helper methods, and changing it to protected improves encapsulation and API consistency.	Low
More