Skip to content

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jun 13, 2025

User description

🔗 Related Issues

Per resolution in w3c/webdriver-bidi#887

💥 What does this PR do?

Allow handling of "-0" as a response value, with the understanding that this is intended behavior from BiDi.

🔧 Implementation Notes

Note that Broker.cs retains handling of numbers-as-strings, even if that is strictly speaking, not necessary to keep around.

I personally do not see the harm in keeping this as is, but I do not have a strong opinion.

// BiDi returns special numbers such as "NaN" as strings
// Additionally, -0 is returned as a string "-0"
NumberHandling = JsonNumberHandling.AllowNamedFloatingPointLiterals | JsonNumberHandling.AllowReadingFromString,

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Bug fix


Description

• Add BiDi JSON converter for handling negative zero double values
• Enable proper serialization/deserialization of special double values (-0, NaN, Infinity)
• Remove browser-specific test ignores for negative zero handling


Changes walkthrough 📝

Relevant files
Enhancement
BiDiDoubleConverter.cs
Add BiDi double JSON converter                                                     

dotnet/src/webdriver/BiDi/Communication/Json/Converters/BiDiDoubleConverter.cs

• Create new JSON converter for BiDi double values
• Handle special
cases: -0, NaN, Infinity, -Infinity
• Implement bit pattern comparison
for negative zero detection
• Provide spec-compliant
serialization/deserialization

+92/-0   
LocalValue.cs
Apply double converter to LocalValue                                         

dotnet/src/webdriver/BiDi/Script/LocalValue.cs

• Apply BiDiDoubleConverter to NumberLocalValue double property
• Add
converter import statement

+2/-1     
RemoteValue.cs
Apply double converter to RemoteValue                                       

dotnet/src/webdriver/BiDi/Script/RemoteValue.cs

• Apply BiDiDoubleConverter to NumberRemoteValue double property
• Add
converter import statement

+2/-1     
Tests
CallFunctionLocalValueTest.cs
Remove negative zero test ignores                                               

dotnet/test/common/BiDi/Script/CallFunctionLocalValueTest.cs

• Remove browser-specific test ignores for Chrome and Edge
• Enable
negative zero test for all browsers

+0/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 13, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ChromeDriver connection failure error after first instance
    • Resolve "ConnectFailure (Connection refused)" error for subsequent ChromeDriver instances
    • Ensure proper ChromeDriver instantiation on Ubuntu 16.04.4 with Chrome 65.0.3325.181 and ChromeDriver 2.35

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link's href attribute on click() method
    • Ensure compatibility with Firefox 42.0 32bit on 64bit machine
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0 and 2.48.2

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Exception Handling

    The Read method throws a generic JsonException without specific error messages when string parsing fails, which could make debugging difficult for developers using this converter.

        throw new JsonException();
    }
    Performance Concern

    The IsNegativeZero method uses BitConverter operations which may have performance implications when called frequently during JSON serialization of large datasets.

    static bool IsNegativeZero(double x)
    {
        // Negative zero is less trivial to test, because 0 == -0 is true
        // We need to do a bit pattern comparison
    
        return BitConverter.DoubleToInt64Bits(x) == BitConverter.DoubleToInt64Bits(-0.0);
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Optimize negative zero detection

    The bit pattern comparison can be optimized by avoiding the second
    BitConverter.DoubleToInt64Bits call. The bit pattern for negative zero is a
    known constant that can be precomputed.

    dotnet/src/webdriver/BiDi/Communication/Json/Converters/BiDiDoubleConverter.cs [84-90]

     static bool IsNegativeZero(double x)
     {
         // Negative zero is less trivial to test, because 0 == -0 is true
         // We need to do a bit pattern comparison
    -
    -    return BitConverter.DoubleToInt64Bits(x) == BitConverter.DoubleToInt64Bits(-0.0);
    +    const long NegativeZeroBits = unchecked((long)0x8000000000000000);
    +    return BitConverter.DoubleToInt64Bits(x) == NegativeZeroBits;
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies a micro-optimization by replacing a repeated calculation (BitConverter.DoubleToInt64Bits(-0.0)) with a pre-computed constant. This improves performance and readability by making the comparison value explicit.

    Low
    Improve null handling pattern

    The null-coalescing operator with exception throw creates an unnecessary
    intermediate variable. Consider using a more direct approach that handles the
    null case explicitly for better readability and performance.

    dotnet/src/webdriver/BiDi/Communication/Json/Converters/BiDiDoubleConverter.cs [39]

    -var str = reader.GetString() ?? throw new JsonException();
    +var str = reader.GetString();
    +if (str is null)
    +{
    +    throw new JsonException();
    +}
    • Apply / Chat
    Suggestion importance[1-10]: 2

    __

    Why: The suggestion replaces a modern C# 9.0 "throw expression" with a more verbose if-null check. This is a stylistic preference with negligible performance impact and can be seen as a step back from concise, modern syntax.

    Low
    • Update

    @nvborisenko
    Copy link
    Member

    Thanks Mike, in general this approach looks good, let's review minor comments.

    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you, Mike. Just minor comments.

    @RenderMichael RenderMichael merged commit 3b61afe into SeleniumHQ:trunk Oct 6, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the bidi-negative-zero branch October 6, 2025 17:09
    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