Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public async Task<ReloadResult> ReloadAsync(BrowsingContext context, ReloadOptio

public async Task<SetViewportResult> SetViewportAsync(BrowsingContext context, SetViewportOptions? options = null)
{
var @params = new SetViewportParameters(context, options?.Viewport, options?.DevicePixelRatio);
var @params = new SetViewportParameters(context, options?.Viewport ?? Optional<Viewport?>.None, options?.DevicePixelRatio ?? Optional<double?>.None);

return await Broker.ExecuteCommandAsync(new SetViewportCommand(@params), options, _jsonContext.SetViewportCommand, _jsonContext.SetViewportResult).ConfigureAwait(false);
}
Expand Down Expand Up @@ -292,4 +292,6 @@ protected override void Initialize(JsonSerializerOptions options)
[JsonSerializable(typeof(NavigationInfo))]
[JsonSerializable(typeof(UserPromptOpenedEventArgs))]
[JsonSerializable(typeof(UserPromptClosedEventArgs))]
[JsonSerializable(typeof(Viewport?))]
[JsonSerializable(typeof(double?))]
internal partial class BrowsingContextJsonSerializerContext : JsonSerializerContext;
10 changes: 6 additions & 4 deletions dotnet/src/webdriver/BiDi/BrowsingContext/SetViewportCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,23 @@
// under the License.
// </copyright>

using OpenQA.Selenium.BiDi.Json.Converters;
using System.Text.Json.Serialization;

namespace OpenQA.Selenium.BiDi.BrowsingContext;

internal sealed class SetViewportCommand(SetViewportParameters @params)
: Command<SetViewportParameters, SetViewportResult>(@params, "browsingContext.setViewport");

internal sealed record SetViewportParameters(BrowsingContext Context, [property: JsonConverter(typeof(OptionalConverter<Viewport?>))] Optional<Viewport?>? Viewport, [property: JsonConverter(typeof(OptionalConverter<double?>))] Optional<double?>? DevicePixelRatio) : Parameters;
Copy link
Member

Choose a reason for hiding this comment

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

I thought it was better for AOT, like StringEnumConverter where we specify this attribute with explicitly mentioning type (Viewport in this case). Additionally if we recall modules cross-referencing, I like the approach: explicitly specify converters.

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 forgot about AOT, honestly. We should really enable AOT analyzers to we have visibility on AOT-unfriendly things.

<!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
<!--<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
<IsAotCompatible>true</IsAotCompatible>
</PropertyGroup>-->

But either way, this convertor is nothing more than a reminder: all the logic is in [JsonIgnore(WhenWritingDefault)].

I asked the .NET devs if there's a nice way of doing this dotnet/runtime#122049

internal sealed record SetViewportParameters(
BrowsingContext Context,
[property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] Optional<Viewport?> Viewport,
[property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] Optional<double?> DevicePixelRatio) : Parameters;

public sealed class SetViewportOptions : CommandOptions
{
public Optional<Viewport?>? Viewport { get; set; }
public Optional<Viewport?> Viewport { get; set; }

public Optional<double?>? DevicePixelRatio { get; set; }
public Optional<double?> DevicePixelRatio { get; set; }
}

public readonly record struct Viewport(long Width, long Height);
Expand Down
51 changes: 0 additions & 51 deletions dotnet/src/webdriver/BiDi/Json/Converters/OptionalConverter.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// <copyright file="OptionalConverterFactory.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;
using System.Text.Json.Serialization.Metadata;

namespace OpenQA.Selenium.BiDi.Json.Converters;

internal sealed class OptionalConverterFactory : JsonConverterFactory
{
public override bool CanConvert(Type typeToConvert) => typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(Optional<>);

public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options)
{
var converter = (JsonConverter?)Activator.CreateInstance(typeof(OptionalConverter<>).MakeGenericType(typeToConvert.GenericTypeArguments[0]));

return converter;
}

internal sealed class OptionalConverter<T> : JsonConverter<Optional<T>>
{
public override bool HandleNull => true;

public override Optional<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var converter = (JsonTypeInfo<T>)options.GetTypeInfo(typeof(T));

T? value = JsonSerializer.Deserialize<T>(ref reader, converter);
return new Optional<T>(value);
}

public override void Write(Utf8JsonWriter writer, Optional<T> value, JsonSerializerOptions options)
{
if (value.TryGetValue(out var optionalValue))
{
var converter = (JsonTypeInfo<T?>)options.GetTypeInfo(typeof(T?));

JsonSerializer.Serialize(writer, optionalValue, converter);
}
else
{
throw new JsonException("This property should be annotated with [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]");
}
}
}
}

17 changes: 11 additions & 6 deletions dotnet/src/webdriver/BiDi/Optional.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,36 @@
// under the License.
// </copyright>

using OpenQA.Selenium.BiDi.Json.Converters;
using System;
using System.Text.Json.Serialization;

namespace OpenQA.Selenium.BiDi;

[JsonConverter(typeof(OptionalConverterFactory))]
public readonly record struct Optional<T>
{
private readonly T _value;
private readonly T? _value;
public bool HasValue { get; }

public T Value => HasValue
public T? Value => HasValue
? _value
: throw new InvalidOperationException("Optional has no value. Check IsSet first.");
: throw new InvalidOperationException($"Optional has no value. Check {nameof(HasValue)} first.");

public Optional(T value)
public Optional(T? value)
{
_value = value;
HasValue = true;
}

public bool TryGetValue(out T value)
public bool TryGetValue(out T? value)
{
value = _value;
return HasValue;
}

public static Optional<T> None => default;

// implicit conversion from T -> Optional<T>
public static implicit operator Optional<T>(T value) => new(value);
public static implicit operator Optional<T>(T? value) => new(value);
}
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,7 @@ public async Task CanSetViewport()
Assert.That(await GetHeightAsync(), Is.EqualTo(300));

await context.SetViewportAsync(new() { Viewport = new Viewport(250, 300) });
await context.SetViewportAsync(new() { Viewport = default }); // Sends nothing
await context.SetViewportAsync(new() { Viewport = Optional<Viewport?>.None }); // Sends nothing
Copy link
Member

Choose a reason for hiding this comment

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

What if default? Let's mention this case in test to not forget our target behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Optional<Viewport?>.None == default(Optional<Viewport?>). No difference, I just think it reads better.

Copy link
Member

Choose a reason for hiding this comment

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

True, I didn't introduce this helper in initial implementation. I propose just to cover default case additionally in the test.


Assert.That(await GetWidthAsync(), Is.EqualTo(250));
Assert.That(await GetHeightAsync(), Is.EqualTo(300));
Expand Down
Loading