Skip to content

[dotnet] Handle negative zero BiDi response #15898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Member

Choose a reason for hiding this comment

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

In Pr description you mentioned:

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

I propose to remove it. My strong opinion: we should understand what we are doing.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like "we enable all serializations features supported by system.text.json, but we don't know whether it is necessary".

Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// <copyright file="BiDiDoubleConverter.cs" company="Selenium Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
// </copyright>

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace OpenQA.Selenium.BiDi.Communication.Json.Converters;

/// <summary>
/// Serializes and deserializes <see cref="double"/> into a
/// <see href="https://w3c.github.io/webdriver-bidi/#type-script-PrimitiveProtocolValue">BiDi spec-compliant number value</see>.
/// </summary>
internal sealed class BiDiDoubleConverter : JsonConverter<double>
Copy link
Member

Choose a reason for hiding this comment

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

I don't like BiDi entry in the name because it is already in BiDi namespace. Just DoubleConverter looks better, but it seems so abstract name. Probably SpecialNumberConverter is perfect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose BiDiDouble because it follows BiDi's unique concept of "number". The logic is unique to the BiDi spec.

I think you are right about Double though.

I like BiDiNumberConverter. What do you think of that?

Copy link
Member

Choose a reason for hiding this comment

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

I have even better proposal:

  • Declare two converters: NumberLocalValueConverter/NumberRemoteValueConverter which converts NumberLocalValue/NumberRemoteValue types (as I remember local value can be serialized only, remote - deserialized)
  • Register it globally in one place

It makes naming clearer, and it allows to use the same approach for all converters where all of them are still global. I still remember our challenge #15329 (comment). At least in this PR we don't not introduce new knowledge and follows current patterns.

{
public override double Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (reader.TryGetDouble(out double d))
{
return d;
}

var str = reader.GetString() ?? throw new JsonException();
Copy link
Member

Choose a reason for hiding this comment

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

We need a message what is going wrong.


if (str.Equals("-0", StringComparison.Ordinal))
Copy link
Member

Choose a reason for hiding this comment

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

InvariantCulture better? At least this comparer is used in json everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

An in other places please.

{
return -0.0;
}
else if (str.Equals("NaN", StringComparison.Ordinal))
{
return double.NaN;
}
else if (str.Equals("Infinity", StringComparison.Ordinal))
{
return double.PositiveInfinity;
}
else if (str.Equals("-Infinity", StringComparison.Ordinal))
{
return double.NegativeInfinity;
}

throw new JsonException();
}

public override void Write(Utf8JsonWriter writer, double value, JsonSerializerOptions options)
{
if (double.IsNaN(value))
{
writer.WriteStringValue("NaN");
}
else if (double.IsPositiveInfinity(value))
{
writer.WriteStringValue("Infinity");
}
else if (double.IsNegativeInfinity(value))
{
writer.WriteStringValue("-Infinity");
}
else if (IsNegativeZero(value))
{
writer.WriteStringValue("-0");
}
else
{
writer.WriteNumberValue(value);
}

static bool IsNegativeZero(double x)
Copy link
Member

Choose a reason for hiding this comment

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

I guess this method is unnecessary. Based on your comment you rely on == operator in unit tests. I hope there is simple way to do it better.

{
// 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);
}
}
}
3 changes: 2 additions & 1 deletion dotnet/src/webdriver/BiDi/Script/LocalValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
using System.Numerics;
using System.Text.Json.Serialization;
using System.Text.RegularExpressions;
using OpenQA.Selenium.BiDi.Communication.Json.Converters;

namespace OpenQA.Selenium.BiDi.Script;

Expand Down Expand Up @@ -282,7 +283,7 @@ private static LocalValue ReflectionBasedConvertFrom(object? value)

public abstract record PrimitiveProtocolLocalValue : LocalValue;

public record NumberLocalValue(double Value) : PrimitiveProtocolLocalValue
public record NumberLocalValue([property: JsonConverter(typeof(BiDiDoubleConverter))] double Value) : PrimitiveProtocolLocalValue
{
public static explicit operator NumberLocalValue(double n) => new NumberLocalValue(n);
}
Expand Down
3 changes: 2 additions & 1 deletion dotnet/src/webdriver/BiDi/Script/RemoteValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using System.Numerics;
using System.Text.Json;
using System.Text.Json.Serialization;
using OpenQA.Selenium.BiDi.Communication.Json.Converters;

namespace OpenQA.Selenium.BiDi.Script;

Expand Down Expand Up @@ -97,7 +98,7 @@ public abstract record RemoteValue
}
}

public record NumberRemoteValue(double Value) : PrimitiveProtocolRemoteValue;
public record NumberRemoteValue([property: JsonConverter(typeof(BiDiDoubleConverter))] double Value) : PrimitiveProtocolRemoteValue;

public record BooleanRemoteValue(bool Value) : PrimitiveProtocolRemoteValue;

Expand Down
2 changes: 0 additions & 2 deletions dotnet/test/common/BiDi/Script/CallFunctionLocalValueTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -221,8 +221,6 @@ public async Task CanCallFunctionWithArgumentNumberZero()
}

[Test]
[IgnoreBrowser(Selenium.Browser.Edge, "Chromium can't handle -0 argument as a number: https://github.com/w3c/webdriver-bidi/issues/887")]
[IgnoreBrowser(Selenium.Browser.Chrome, "Chromium can't handle -0 argument as a number: https://github.com/w3c/webdriver-bidi/issues/887")]
public async Task CanCallFunctionWithArgumentNumberNegativeZero()
{
var arg = new NumberLocalValue(double.NegativeZero);
Expand Down
Loading