[dotnet] [bidi] Separate empty and base result

[RenderMichael]

User description

💥 What does this PR do?

The EmptyResult does two different things: act as a base type for all BiDi results, and act as a void result. We can separate these two responsibilities to two different types, leading to clearer code. Additionally, we can rest assured that any EmptyResult we get is not non-empty result without at runtime.

---

PR Type

Enhancement, Other

---

Description

• Replaced EmptyResult with BiDiResult as the base type for BiDi command results.

• Refactored multiple command result classes to inherit from BiDiResult.

• Updated serialization and deserialization logic to accommodate the new BiDiResult type.

• Improved code clarity by separating void results (EmptyResult) from the base result type.

---

Changes walkthrough 📝

Relevant files

Enhancement

25 files Broker.cs Update command execution to use `BiDiResult`                          +6/-6      Command.cs Introduce `BiDiResult` and refactor command base                  +5/-2      BiDiJsonSerializerContext.cs Update JSON serialization for `BiDiResult`                              +1/-1      InputOriginConverter.cs Adjust serialization logic for shared references                  +3/-4      Message.cs Update message success handling to use `BiDiResult`            +1/-1      GetClientWindowsCommand.cs Refactor `GetClientWindowsResult` to inherit `BiDiResult` +1/-1      GetUserContextsCommand.cs Refactor `GetUserContextsResult` to inherit `BiDiResult`  +1/-1      UserContextInfo.cs Update `UserContextInfo` to inherit `BiDiResult`                  +1/-1      CaptureScreenshotCommand.cs Refactor `CaptureScreenshotResult` to inherit `BiDiResult` +1/-1      CreateCommand.cs Update `CreateResult` to inherit `BiDiResult`                        +1/-1      GetTreeCommand.cs Refactor `GetTreeResult` to inherit `BiDiResult`                  +1/-1      LocateNodesCommand.cs Update `LocateNodesResult` to inherit `BiDiResult`              +1/-1      NavigateCommand.cs Refactor `NavigateResult` to inherit `BiDiResult`                +1/-1      PrintCommand.cs Update `PrintResult` to inherit `BiDiResult`                          +1/-1      TraverseHistoryCommand.cs Refactor `TraverseHistoryResult` to inherit `BiDiResult`  +1/-1      AddInterceptCommand.cs Update `AddInterceptResult` to inherit `BiDiResult`            +1/-1      AddPreloadScriptCommand.cs Refactor `AddPreloadScriptResult` to inherit `BiDiResult` +1/-1      EvaluateCommand.cs Update `EvaluateResult` hierarchy to inherit `BiDiResult` +1/-1      GetRealmsCommand.cs Refactor `GetRealmsResult` to inherit `BiDiResult`              +1/-1      NewCommand.cs Update `NewResult` to inherit `BiDiResult`                              +1/-1      StatusCommand.cs Refactor `StatusResult` to inherit `BiDiResult`                    +1/-1      SubscribeCommand.cs Update `SubscribeResult` to inherit `BiDiResult`                  +1/-1      DeleteCookiesCommand.cs Refactor `DeleteCookiesResult` to inherit `BiDiResult`      +1/-1      GetCookiesCommand.cs Update `GetCookiesResult` to inherit `BiDiResult`                +1/-1      SetCookieCommand.cs Refactor `SetCookieResult` to inherit `BiDiResult`              +1/-1

---

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:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Serialization The Write method for ElementOrigin is missing the actual serialization of the element.Element property. The code adds the property name but doesn't complete the serialization process. else if (value is ElementOrigin element) { writer.WriteStartObject(); writer.WriteString("type", "element"); writer.WritePropertyName("element"); JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>()); writer.WriteEndObject(); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix serialization method call The code is using options.GetTypeInfo() which appears to be a custom extension method, but there's no indication that this method exists in the standard JsonSerializerOptions class. This could lead to runtime errors if the method is not properly defined or accessible. dotnet/src/webdriver/BiDi/Communication/Json/Converters/InputOriginConverter.cs [50-51] writer.WritePropertyName("element"); -JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>()); +JsonSerializer.Serialize(writer, element.Element, options); Apply this suggestion Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a potential issue with the serialization method. The PR changed the serialization from using options directly to using options.GetTypeInfo<ISharedReference>(), which is a custom extension method that might not be properly defined or accessible, potentially causing runtime errors. Reverting to the original approach would be safer.	High
Learned best practice	Add null validation before accessing object properties to prevent potential NullReferenceExceptions The code is accessing element.Element without first checking if it's null. This could lead to a NullReferenceException if the Element property is null. Add a null check before serializing the element to prevent potential runtime exceptions. dotnet/src/webdriver/BiDi/Communication/Json/Converters/InputOriginConverter.cs [50-51] writer.WritePropertyName("element"); +ArgumentNullException.ThrowIfNull(element.Element, nameof(element.Element)); JsonSerializer.Serialize(writer, element.Element, options.GetTypeInfo<ISharedReference>()); Apply this suggestion Suggestion importance[1-10]: 6	Low
More

[RenderMichael]

Not conveyed in this PR: all the places where we keep the type as EmptyResult

[nvborisenko]

Related to #15562, let's discuss there our expectations.

[nvborisenko]

The EmptyResult does two different things: act as a base type for all BiDi results, and act as a void result.

True, we will avoid user facing Task DoSomething() and rework to Task<Result> DoSomething() in #15562. If we agree, then separating BiDiResult and EmptyResult is unnecessary I think.