Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented May 25, 2025

User description

🔗 Related Issues

💥 What does this PR do?

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

var userContext = await bidi.Browser.CreateUserContextAsync(new()
{
    AcceptInsecureCerts = true,
    Proxy = new Modules.Session.AutoDetectProxyConfiguration()
});

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Add AcceptInsecureCerts and Proxy options to CreateUserContext

  • Update command parameter handling for user context creation

  • Enhance .NET BiDi BrowserModule for flexible user context setup


Changes walkthrough 📝

Relevant files
Enhancement
BrowserModule.cs
Support AcceptInsecureCerts and Proxy in CreateUserContextAsync

dotnet/src/webdriver/BiDi/Modules/Browser/BrowserModule.cs

  • Pass AcceptInsecureCerts and Proxy options to CreateUserContext
    command
  • Construct command parameters from options for user context creation
  • Improve flexibility of CreateUserContextAsync method
  • +3/-2     
    CreateUserContextCommand.cs
    Add command parameters and options for user context creation

    dotnet/src/webdriver/BiDi/Modules/Browser/CreateUserContextCommand.cs

  • Add CreateUserContextCommandParameters record for new options
  • Update CreateUserContextCommand to accept parameters
  • Extend CreateUserContextOptions with AcceptInsecureCerts and Proxy
    properties
  • +10/-3   

    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 May 25, 2025
    @qodo-merge-pro
    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 "Error: ConnectFailure (Connection refused)" when instantiating Chrome Driver multiple times

    Requires further human verification:

    • Need to verify if the PR's implementation of AcceptInsecureCerts could potentially help with connection issues

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Firefox 42.0 doesn't trigger JavaScript in link's href on click()

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

    Incomplete Implementation

    The CreateUserContextAsync method is missing the implementation body in the old code. The PR adds the implementation, but it's unclear if this is a new feature or fixing a previously incomplete implementation.

    public async Task<UserContextInfo> CreateUserContextAsync(CreateUserContextOptions? options = null)
    {
        var @params = new CreateUserContextCommandParameters(options?.AcceptInsecureCerts, options?.Proxy);
    
        return await Broker.ExecuteCommandAsync<CreateUserContextCommand, UserContextInfo>(new CreateUserContextCommand(@params), options).ConfigureAwait(false);
    }

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove redundant parameter

    The options parameter is passed twice - once to create command parameters and
    again to the ExecuteCommandAsync method. This is redundant and could cause
    issues as the same options are processed twice. Remove the second options
    parameter.

    dotnet/src/webdriver/BiDi/Modules/Browser/BrowserModule.cs [34-36]

     var @params = new CreateUserContextCommandParameters(options?.AcceptInsecureCerts, options?.Proxy);
     
    -return await Broker.ExecuteCommandAsync<CreateUserContextCommand, UserContextInfo>(new CreateUserContextCommand(@params), options).ConfigureAwait(false);
    +return await Broker.ExecuteCommandAsync<CreateUserContextCommand, UserContextInfo>(new CreateUserContextCommand(@params), null).ConfigureAwait(false);
    • Apply / Chat
    Suggestion importance[1-10]: 4

    __

    Why: The suggestion to remove the second options parameter is debatable; options may be required by ExecuteCommandAsync for additional context or settings. Removing it could break intended extensibility or option handling, so while it may reduce redundancy, it risks losing necessary functionality. The suggestion is minor and should be carefully verified.

    Low
    • More

    @nvborisenko
    Copy link
    Member Author

    CI failure is unrelated to this changes.

    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.

    2 participants