Skip to content

[bidi] Update browsing context create method#13766

Merged
pujagani merged 5 commits intoSeleniumHQ:trunkfrom
pujagani:update-browsing-context-create
Apr 24, 2024
Merged

[bidi] Update browsing context create method#13766
pujagani merged 5 commits intoSeleniumHQ:trunkfrom
pujagani:update-browsing-context-create

Conversation

@pujagani
Copy link
Copy Markdown
Contributor

@pujagani pujagani commented Apr 2, 2024 •
edited by qodo-code-review bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Keeping with the evolving BiDi spec for browsing context create method - https://w3c.github.io/webdriver-bidi/#command-browsingContext-create. Added CreateContextParameters class.

Motivation and Context

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) - > JS doesn't not support overloaded constructor so the referenceContext is replaced with CreateContextParameters class.

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.

Type

enhancement, bug_fix

Description

  • Introduced CreateContextParameters class in both Java and JavaScript to support the evolving BiDi spec for browsing context creation.
  • Deprecated the constructor that uses referenceContextId in favor of the new CreateContextParameters in Java.
  • Updated BrowsingContext constructors and methods in Java and JavaScript to utilize CreateContextParameters.
  • Added and updated tests in Java and JavaScript to cover the new browsing context creation process.

Changes walkthrough

Relevant files
Enhancement
BrowsingContext.java
Update BrowsingContext Constructors for BiDi Compliance   

java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java

  • Deprecated the constructor with referenceContextId parameter.
  • Added a new constructor using CreateContextParameters.
  • Modified create method to support CreateContextParameters.
    		•  +20/-6   
    CreateContextParameters.java
    Add CreateContextParameters Class for Browsing Context Creation

    java/src/org/openqa/selenium/bidi/browsingcontext/CreateContextParameters.java

  • Introduced CreateContextParameters class with methods to build
    parameters for browsing context creation.
    		•  +51/-0   
    browsingContext.js
    Refactor BrowsingContext Initialization and Creation         

    javascript/node/selenium-webdriver/bidi/browsingContext.js

  • Updated init and create methods to support CreateContextParameters.
  • Added validation for parameters in create method.
    		•  +35/-12 
    createContextParameters.js
    Implement CreateContextParameters Class for Browsing Context Creation

    javascript/node/selenium-webdriver/bidi/createContextParameters.js

  • Created CreateContextParameters class with methods to set up browsing
    context creation parameters.
    		•  +50/-0   
    Tests
    BrowsingContextTest.java
    Update BrowsingContext Tests to Use New Constructor           

    java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java

  • Updated tests to use the new CreateContextParameters constructor.
  • Added a new test for creating a context with all parameters.
    		•  +28/-2   
    browsingcontext_test.js
    Add Tests for BrowsingContext Creation with New Parameters

    javascript/node/selenium-webdriver/test/bidi/browsingcontext_test.js

  • Added tests for creating browsing contexts with new
    CreateContextParameters.
    		•  +18/-2   

    ✨ PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @qodo-code-review qodo-code-review bot added P-enhancement PR with a new feature bug_fix labels Apr 2, 2024
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Apr 2, 2024

    PR Description updated to latest commit (fbfc664)

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Apr 2, 2024

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR introduces significant changes to the browsing context creation logic, including deprecation of an existing constructor and addition of a new class for creating context parameters. The changes span across multiple files in both Java and JavaScript implementations, requiring a thorough review to ensure compatibility and adherence to the BiDi spec. However, the PR is well-structured and focused on a single feature enhancement, which simplifies the review process.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Deprecated Constructor Usage: The PR deprecates an existing constructor in favor of a new method that uses CreateContextParameters. It's important to ensure that this change is clearly communicated to users to prevent confusion.

    Error Handling in JavaScript: The create method in browsingContext.js throws generic errors for invalid parameters. It might be beneficial to provide more specific error messages for better debugging and user experience.

    Type Checking: In createContextParameters.js, the type checking for parameters (e.g., background, userContext) uses typeof which is good. However, ensuring these checks are robust and cover all edge cases is crucial to prevent runtime errors.

    🔒 Security concerns

    No
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review bot commented Apr 2, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add a check to ensure the WebDriver instance supports BiDi protocol before casting.

    It's recommended to check if driver is an instance of HasBiDi before casting it to avoid
    potential ClassCastException. This check should be performed in the constructor that
    receives CreateContextParameters as an argument.

    java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java [135-136]

    +if (!(driver instanceof HasBiDi)) {
+  throw new IllegalArgumentException("WebDriver instance must support BiDi protocol");
+}
 this.bidi = ((HasBiDi) driver).getBiDi();
    Change asMap method to return an object for consistency.

    The asMap method should return an object instead of a Map to be consistent with the
    expected parameter type in the create method of BrowsingContext.

    javascript/node/selenium-webdriver/bidi/createContextParameters.js [46-47]

    -return this.#map
+return Object.fromEntries(this.#map)
    Add null check for WindowType parameter in the constructor.

    Consider adding validation for the WindowType parameter in the constructor to ensure it's
    not null. This prevents potential NullPointerException when calling windowType.toString()
    in the toMap method.

    java/src/org/openqa/selenium/bidi/browsingcontext/CreateContextParameters.java [28-29]

     public CreateContextParameters(WindowType type) {
-  this.windowType = type;
+  this.windowType = Objects.requireNonNull(type, "WindowType cannot be null");
 }
    Bug
    Fix the condition to correctly check the instance of CreateContextParameters.

    The condition in the create method should check if createParameters is an instance of
    CreateContextParameters correctly. The current condition always evaluates to false due to
    a misplaced parenthesis.

    javascript/node/selenium-webdriver/bidi/browsingContext.js [111-113]

    -if (createParameters !== undefined && (!createParameters) instanceof CreateContextParameters) {
+if (createParameters !== undefined && !(createParameters instanceof CreateContextParameters)) {
    Maintainability
    Refactor init method into smaller, more focused methods.

    To improve code readability and error handling, consider splitting the init method into
    smaller methods, each handling a specific case (e.g., initializing with a
    browsingContextId, type, or createParameters).

    javascript/node/selenium-webdriver/bidi/browsingContext.js [83-105]

    -async init({ browsingContextId = undefined, type = undefined, createParameters = undefined }) {
+async initWithBrowsingContextId(browsingContextId) {
+  ...
+}
+async initWithType(type) {
+  ...
+}
+async initWithCreateParameters(createParameters) {
   ...
 }
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    @codecov-commenter
    Copy link
    Copy Markdown

    codecov-commenter commented Apr 2, 2024 •
    edited
    Loading

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.63%. Comparing base (37565af) to head (08c1639).
    Report is 1 commits behind head on trunk.

    ❗ Current head 08c1639 differs from pull request most recent head dbcf179. Consider uploading reports for the commit dbcf179 to get more accurate results

    ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

    Additional details and impacted files 
    @@           Coverage Diff           @@
##            trunk   #13766   +/-   ##
=======================================
  Coverage   58.63%   58.63%           
=======================================
  Files          86       86           
  Lines        5272     5272           
  Branches      218      218           
=======================================
  Hits         3091     3091           
  Misses       1963     1963           
  Partials      218      218

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @pujagani pujagani force-pushed the update-browsing-context-create branch from e83526b to c6c96c5 Compare April 23, 2024 10:52
    @pujagani pujagani merged commit 4a6c384 into SeleniumHQ:trunk Apr 24, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 3

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @pujagani @codecov-commenter