[dotnet] [bidi] Remove IEnumerable of command results

[nvborisenko]

User description

This contributes to #16095

💥 What does this PR do?

Remove syntax sugar and be more straightforward. Simplifying things as much possible. Any other sugar can be treated as "alternative API" via extensions.

💡 Additional Considerations

🔄 Types of changes

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

---

PR Type

Enhancement

---

Description

• Remove IEnumerable implementation from BiDi command results

• Simplify result classes to use primary constructors

• Delete custom JSON converters for enumerable results

• Update test code to access properties directly

---

Diagram Walkthrough

flowchart LR
  A["Command Result Classes"] --> B["Remove IEnumerable Interface"]
  B --> C["Use Primary Constructors"]
  C --> D["Delete JSON Converters"]
  D --> E["Update Test Access Patterns"]

Loading

File Walkthrough

	Relevant files
Enhancement	14 files GetClientWindowsCommand.cs Simplify GetClientWindowsResult to primary constructor      +1/-20    GetUserContextsCommand.cs Simplify GetUserContextsResult to primary constructor        +1/-18    BrowsingContextScriptModule.cs Update GetRealms call to access Realms property                    +1/-1      GetTreeCommand.cs Simplify GetTreeResult to primary constructor                        +1/-17    LocateNodesCommand.cs Simplify LocateNodesResult to primary constructor                +1/-18    Broker.cs Remove enumerable JSON converter registrations                      +0/-6      GetClientWindowsResultConverter.cs Delete GetClientWindowsResult JSON converter                          +0/-43    GetCookiesResultConverter.cs Delete GetCookiesResult JSON converter                                      +0/-45    GetRealmsResultConverter.cs Delete GetRealmsResult JSON converter                                        +0/-43    GetTreeResultConverter.cs Delete GetTreeResult JSON converter                                            +0/-43    GetUserContextsResultConverter.cs Delete GetUserContextsResult JSON converter                            +0/-43    LocateNodesResultConverter.cs Delete LocateNodesResult JSON converter                                    +0/-44    GetRealmsCommand.cs Simplify GetRealmsResult to primary constructor                    +1/-18    GetCookiesCommand.cs Simplify GetCookiesResult to primary constructor                  +1/-21
Tests	8 files BiDiFixture.cs Update test to access Contexts property                                    +1/-1      BrowserTest.cs Update tests to access result properties                                  +12/-12  BrowsingContextTest.cs Update tests to access Contexts property                                  +12/-12  SetFilesTest.cs Update test to access Nodes property                                          +2/-2      CallFunctionParameterTest.cs Update test to access Realms property                                        +5/-5      EvaluateParametersTest.cs Update test to access Realms property                                        +5/-5      ScriptCommandsTest.cs Update tests to access Realms property                                      +14/-14  StorageTest.cs Update tests to access Cookies property                                    +24/-24

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 16095 - Partially compliant Compliant requirements: Remove overreaching syntax sugar like IReadOnlyList on Result classes and associated JSON converters; provide optional extensions instead. Use EmptyResult as return type instead of void for all methods. Non-compliant requirements: Move/introduce driver.AsBiDiAsync() on AnyDriver/IWebDriver so BiDi lifetime is owned by the driver. Revisit InterceptRequestAsync-style helpers; prefer extension-based convenience APIs atop public methods. Use normal types as event args (inheritance topic). Requires further human verification: Validate end-to-end that removal of IEnumerable interfaces does not break downstream consumers or docs. Confirm public API surface aligns with intended BiDi stability plan across packages (Selenium.WebDriver -> Selenium.WebDriver.BiDi).

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue GetRealmsAsync now returns Realms property from result. Confirm that null handling and default options remain correct and that previous callers expecting IEnumerable still behave via updated tests. public async Task<IReadOnlyList<RealmInfo>> GetRealmsAsync(GetRealmsOptions? options = null) { options ??= new(); options.Context = context; return (await scriptModule.GetRealmsAsync(options).ConfigureAwait(false)).Realms; } Serialization Change Removal of enumerable JSON converters may impact deserialization of older payload shapes. Verify all affected commands map correctly to new record shapes and no converters are still needed. // https://github.com/dotnet/runtime/issues/72604 new Json.Converters.Polymorphic.EvaluateResultConverter(), new Json.Converters.Polymorphic.RemoteValueConverter(), new Json.Converters.Polymorphic.RealmInfoConverter(), new Json.Converters.Polymorphic.LogEntryConverter(), // // Enumerable new Json.Converters.Enumerable.InputSourceActionsConverter(), } }; _jsonSerializerContext = new BiDiJsonSerializerContext(jsonSerializerOptions); API Break GetCookiesResult now exposes Cookies and PartitionKey properties only; indexer and Count removed. Ensure all public usages and samples are updated and that ordering guarantees (if any) are documented. public sealed class GetCookiesOptions : CommandOptions { public CookieFilter? Filter { get; set; } public PartitionDescriptor? Partition { get; set; } } public sealed record GetCookiesResult(IReadOnlyList<Network.Cookie> Cookies, PartitionKey PartitionKey) : EmptyResult; public sealed record CookieFilter { public string? Name { get; set; }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Ensure JSON deserialization works You removed custom JSON converters for enumerable results and changed result types to primary-constructor records with collection properties, but did not show how System.Text.Json will now bind nested arrays (e.g., "realms", "contexts", "cookies") into those properties. Verify the default serializer configuration maps the response payloads to the new record shapes, or add minimal converters/JsonPropertyName attributes to prevent runtime deserialization failures across all affected commands. Examples: dotnet/src/webdriver/BiDi/BrowsingContext/GetTreeCommand.cs [50] public sealed record GetTreeResult(IReadOnlyList<BrowsingContextInfo> Contexts) : EmptyResult; dotnet/src/webdriver/BiDi/Communication/Broker.cs [95-103] new Json.Converters.Polymorphic.EvaluateResultConverter(), new Json.Converters.Polymorphic.RemoteValueConverter(), new Json.Converters.Polymorphic.RealmInfoConverter(), new Json.Converters.Polymorphic.LogEntryConverter(), // // Enumerable new Json.Converters.Enumerable.InputSourceActionsConverter(), } Solution Walkthrough: Before: // In Broker.cs new JsonSerializerOptions { Converters = { // ... new Json.Converters.Enumerable.GetTreeResultConverter(), // ... } }; // In GetTreeCommand.cs public sealed record GetTreeResult : EmptyResult, IReadOnlyList<BrowsingContextInfo> { // ... manual implementation ... } After: // In Broker.cs new JsonSerializerOptions { Converters = { // GetTreeResultConverter is removed } }; // In GetTreeCommand.cs, to be robust against serializer settings: using System.Text.Json.Serialization; public sealed record GetTreeResult( [property: JsonPropertyName("contexts")] IReadOnlyList<BrowsingContextInfo> Contexts ) : EmptyResult; Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical potential failure point in JSON deserialization after removing custom converters, which could break multiple BiDi commands at runtime if the implicit mapping fails.	High
Possible issue	Null-safe realms access Guard against a null Realms collection to avoid a potential NullReferenceException if the deserializer or backend omits the property. Provide an empty list fallback to keep the public API stable. dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextScriptModule.cs [44] -return (await scriptModule.GetRealmsAsync(options).ConfigureAwait(false)).Realms; +var result = await scriptModule.GetRealmsAsync(options).ConfigureAwait(false); +return result.Realms ?? Array.Empty<RealmInfo>(); Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that Realms could be null after deserialization and proposes a robust fix using ?? Array.Empty<RealmInfo>(), which is good practice for methods returning collections.	Low
General	Null-safe empty assertion Accessing Cookies.Count directly can throw if Cookies is null after deserialization changes. Adjust the assertion to handle null safely to prevent test failures masking real issues. dotnet/src/webdriver/BiDi/Storage/StorageTest.cs [144] -Assert.That(cookiesResult.Cookies.Count, Is.EqualTo(0)); +Assert.That(cookiesResult.Cookies, Is.Empty); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out a potential null reference and suggests a more robust and idiomatic assertion Assert.That(cookiesResult.Cookies, Is.Empty), improving test code quality.	Low
Update

[RenderMichael]

I strongly like this direction!