Skip to content

[dotnet] [bidi] Support SetClientWindowState in Browser module#15533

Closed
nvborisenko wants to merge 10 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-set-client-window-state
Closed

[dotnet] [bidi] Support SetClientWindowState in Browser module#15533
nvborisenko wants to merge 10 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-set-client-window-state

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko commented Mar 29, 2025 •
edited by qodo-code-review bot
Loading

User description

https://w3c.github.io/webdriver-bidi/#command-browser-setClientWindowState

browser.SetClientWindowState = (
  method: "browser.setClientWindowState",
  params: browser.SetClientWindowStateParameters
)

browser.SetClientWindowStateParameters = {
  clientWindow: browser.ClientWindow,
  (browser.ClientWindowNamedState // browser.ClientWindowRectState)
}

browser.ClientWindowNamedState = (
  state: "fullscreen" / "maximized" / "minimized"
)

browser.ClientWindowRectState = (
  state: "normal",
  ? width: js-uint,
  ? height: js-uint,
  ? x: js-int,
  ? y: js-int,
)

Motivation and Context

Improve bidi coverage.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement

Description

• Add SetClientWindowState command support for BiDi Browser module
• Implement named state (fullscreen/maximized/minimized) and rect state options
• Add public API methods for setting client window states
• Include test coverage for new functionality

Changes walkthrough 📝

Relevant files
Enhancement
5 files
BrowserModule.cs
Add SetClientWindowState async methods                                     
+14/-0   
ClientWindow.cs
Add BiDi reference and SetState methods                                   
+16/-1   
ClientWindowInfo.cs
Inherit from EmptyResult base class                                           
+4/-1     
SetClientWindowStateCommand.cs
Create new command with parameters and options                     
+66/-0   
BrowserClientWindowConverter.cs
Accept BiDi parameter in constructor                                         
+8/-1     
Bug fix
1 files
Broker.cs
Pass BiDi instance to converter                                                   
+2/-2     
Configuration changes
1 files
BiDiJsonSerializerContext.cs
Register SetClientWindowStateCommand for serialization     
+1/-0     
Tests
1 files
BrowserTest.cs
Add tests for window state functionality                                 
+24/-0   
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-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 29, 2025 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit 0335935)

    Here are some key observations to aid the review process:

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

    Property Initialization

    Properties in SetClientWindowRectStateCommandParameters are initialized from Options in the constructor but can be overwritten by public setters, potentially causing inconsistent state between constructor parameters and property values.

     
    public int? Width { get; set; } = Options?.Width;

public int? Height { get; set; } = Options?.Height;

public int? X { get; set; } = Options?.X;

public int? Y { get; set; } = Options?.Y;
    Incomplete Tests

    Test methods have placeholder assertions that don't validate the actual functionality. The tests are also ignored, which means they won't run in CI/CD pipelines to catch regressions.

     
        // TODO: Make assertion meaningfull
    Assert.That(clientWindowInfo, Is.Not.Null);
}

[Test]
[Ignore("Not yet implemented by all vendors")]
public async Task CanSetClientWindowRectState()
{
    var clientWindows = await bidi.Browser.GetClientWindowsAsync();

    var clientWindowInfo = await clientWindows[0].ClientWindow.SetStateAsync(new() { Width = 500, Height = 300, X = 10, Y = 10 });

    // TODO: Make assertion meaningfull
    Assert.That(clientWindowInfo, Is.Not.Null);
    Missing Null Check

    The Read method doesn't handle the case where reader.GetString() returns null, which could cause a NullReferenceException when creating the ClientWindow instance.

     
    var id = reader.GetString();

return new ClientWindow(_bidi, id!);

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Mar 29, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use property getters

    The properties in SetClientWindowRectStateCommandParameters are initialized in
    the constructor but never updated if Options changes. Consider using property
    getters that directly access the Options object instead of initializing the
    properties once.

    dotnet/src/webdriver/BiDi/Modules/Browser/SetClientWindowStateCommand.cs [34-46]

     internal record SetClientWindowRectStateCommandParameters(ClientWindow ClientWindow, [property: JsonIgnore] SetClientWindowRectStateOptions? Options) : SetClientWindowStateCommandParameters(ClientWindow)
 {
     [JsonInclude]
     internal string State { get; } = "normal";
 
-    public int? Width { get; set; } = Options?.Width;
+    public int? Width => Options?.Width;
 
-    public int? Height { get; set; } = Options?.Height;
+    public int? Height => Options?.Height;
 
-    public int? X { get; set; } = Options?.X;
+    public int? X => Options?.X;
 
-    public int? Y { get; set; } = Options?.Y;
+    public int? Y => Options?.Y;
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential issue where the properties are initialized only once from Options in the constructor, but won't reflect any changes to Options after initialization. Using property getters ensures the values always reflect the current state of the Options object, improving correctness and maintainability.

    		Medium
    Learned
    best practice
    Add null validation for constructor parameters to prevent potential null reference exceptions

    The constructor accepts bidi and id parameters but doesn't validate them for
    null values. Since these parameters are required for the class to function
    properly, you should add null checks using ArgumentNullException.ThrowIfNull()
    to ensure they're not null before assigning them to properties.

    dotnet/src/webdriver/BiDi/Modules/Browser/ClientWindow.cs [26-30]

     internal ClientWindow(BiDi bidi, string id)
 {
+    ArgumentNullException.ThrowIfNull(bidi, nameof(bidi));
+    ArgumentNullException.ThrowIfNull(id, nameof(id));
+    
     BiDi = bidi;
     Id = id;
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • Update

    nvborisenko
    nvborisenko commented Mar 29, 2025
    View reviewed changes
    dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs Outdated
    [JsonSerializable(typeof(Modules.Browser.RemoveUserContextCommand))]
    [JsonSerializable(typeof(Modules.Browser.GetClientWindowsCommand))]
    [JsonSerializable(typeof(Modules.Browser.GetClientWindowsResult))]
    [JsonSerializable(typeof(Modules.Browser.SetClientWindowStateCommand))]
    Copy link
    Copy Markdown
    Member Author

    @nvborisenko nvborisenko Mar 29, 2025

    Choose a reason for hiding this comment

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

    I am not sure I have to add ClientWindowInfo type explicitly here, since it is already added by the code 3 lines below. @RenderMichael do you know? I cannot verify, the tests are ignored.

    We can leave this PR opened until we verify how it works with real browser. At this moment I verified serialization only.

    @nvborisenko nvborisenko mentioned this pull request Mar 31, 2025
    Closed
    @nvborisenko nvborisenko marked this pull request as draft April 14, 2025 16:58
    @nvborisenko
    Copy link
    Copy Markdown
    Member Author

    nvborisenko commented Apr 15, 2025

    In draft because of not supported by browsers yet.

    @nvborisenko nvborisenko mentioned this pull request Jun 18, 2025
    Merged
    @nvborisenko nvborisenko marked this pull request as ready for review June 19, 2025 19:07
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Jun 19, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @nvborisenko nvborisenko marked this pull request as draft July 9, 2025 22:15
    @github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label Mar 17, 2026
    @github-actions github-actions bot closed this Mar 31, 2026
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @RenderMichael RenderMichael Awaiting requested review from RenderMichael

    Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

    Assignees

    No one assigned

    Labels

    J-stale Applied to issues that become stale, and eventually closed. Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @nvborisenko