[dotnet] [bidi] Encapsulate transport inside Broker

[RenderMichael]

User description

Description

Encapsulate transport inside Broker

Motivation and Context

Better encapsulation, works towards custom BiDi modules, such as #15329

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement

---

Description

• Encapsulated the transport mechanism within the Broker class.

• Replaced direct ITransport usage with WebSocketTransport instantiation in Broker.

• Introduced DisposeAsyncCore for better disposal pattern adherence.

• Simplified BiDi initialization by removing direct transport dependency.

---

Changes walkthrough 📝

Relevant files

Enhancement

BiDi.cs Refactored `BiDi` to encapsulate transport dependency        dotnet/src/webdriver/BiDi/BiDi.cs Removed direct dependency on ITransport in BiDi. Modified DisposeAsync to use DisposeAsyncCore and suppress finalization. Simplified Broker initialization by passing Uri instead of ITransport. +7/-6      Broker.cs Refactored `Broker` to manage transport internally              dotnet/src/webdriver/BiDi/Communication/Broker.cs Encapsulated transport creation within Broker using WebSocketTransport. Added DisposeAsyncCore for structured disposal of resources. Removed external dependency on ITransport during Broker initialization. +11/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 15329 - PR Code Verified Compliant requirements: • Implementation does not rely on internal members • Exposes minimal required functionality to make it possible to implement new modules Requires further human verification: • Verify if the changes fully enable implementing BiDi Permissions module as described in the ticket • Confirm if the refactoring supports both usage examples mentioned in the ticket

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Resource Disposal The transport disposal is now handled in DisposeAsyncCore but there's no null check before disposing. Consider adding a null check to prevent potential NullReferenceException if _transport is null. _transport.Dispose(); Empty Block There appears to be an empty code block that might be a leftover from the refactoring. This should be removed to avoid confusion. { await _broker.DisposeAsync().ConfigureAwait(false); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Wait for all tasks The disposal method should also wait for the _receivingMessageTask to complete, similar to how it waits for the _eventEmitterTask. This ensures all async operations are properly completed before disposal. dotnet/src/webdriver/BiDi/Communication/Broker.cs [301-313] public virtual async ValueTask DisposeAsyncCore() { _pendingEvents.CompleteAdding(); _receiveMessagesCancellationTokenSource?.Cancel(); if (_eventEmitterTask is not null) { await _eventEmitterTask.ConfigureAwait(false); } - _transport.Dispose(); + if (_receivingMessageTask is not null) + { + await _receivingMessageTask.ConfigureAwait(false); + } + + _transport?.Dispose(); } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: This suggestion addresses a significant issue by ensuring that all asynchronous operations complete before disposal. Waiting for _receivingMessageTask is as important as waiting for _eventEmitterTask to prevent potential resource leaks or incomplete operations.	Medium
	Add null check The _transport field might be null when accessed. Add a null check before calling Dispose() to prevent a potential NullReferenceException. dotnet/src/webdriver/BiDi/Communication/Broker.cs [301-313] public virtual async ValueTask DisposeAsyncCore() { _pendingEvents.CompleteAdding(); _receiveMessagesCancellationTokenSource?.Cancel(); if (_eventEmitterTask is not null) { await _eventEmitterTask.ConfigureAwait(false); } - _transport.Dispose(); + _transport?.Dispose(); } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: Adding a null check for _transport before calling Dispose() is a good defensive programming practice that prevents potential NullReferenceExceptions. This is particularly important in disposal methods to ensure clean resource cleanup.	Medium
Learned best practice	Add null validation for constructor parameters to prevent NullReferenceException The constructor accepts a url parameter but doesn't validate it for null before using it. Add a null check using ArgumentNullException.ThrowIfNull() to ensure the parameter is not null before creating a Uri from it. dotnet/src/webdriver/BiDi/BiDi.cs [39-46] internal BiDi(string url) { + ArgumentNullException.ThrowIfNull(url, nameof(url)); var uri = new Uri(url); _broker = new Broker(this, uri); _sessionModule = new Lazy<Modules.Session.SessionModule>(() => new Modules.Session.SessionModule(_broker)); _browsingContextModule = new Lazy<Modules.BrowsingContext.BrowsingContextModule>(() => new Modules.BrowsingContext.BrowsingContextModule(_broker)); Apply this suggestion Suggestion importance[1-10]: 6	Low
More

[RenderMichael]

Formatting build failure unrelated:

--- a/py/test/selenium/webdriver/common/selenium_manager_tests.py
+++ b/py/test/selenium/webdriver/common/selenium_manager_tests.py

Test failures not related:

//py:common-chrome-bidi-test/selenium/webdriver/common/bidi_tests.py
//py:common-edge-bidi-test/selenium/webdriver/common/bidi_tests.py
//rb/spec/integration/selenium/webdriver:action_builder-chrome-remote
//rb/spec/integration/selenium/webdriver:driver-chrome
//rb/spec/integration/selenium/webdriver:driver-chrome-remote
//rb/spec/integration/selenium/webdriver:driver-firefox-beta
//rb/spec/integration/selenium/webdriver:manager-chrome
//rb/spec/integration/selenium/webdriver:select-chrome
//rb/spec/integration/selenium/webdriver:select-chrome-remote