Skip to content
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

[java][bidi]: implement getClientWindows method #14869

Open
wants to merge 16 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Dec 6, 2024

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

Implemented getClientWindows method in java for BiDi and added the test.

BiDi spec - https://w3c.github.io/webdriver-bidi/#command-browser-getClientWindows

Currently the PR is a draft as the above method is not released for stable, tested out with firefox nightly and chrome canary.

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, Tests


Description

  • Implemented getClientWindows method for BiDi in Java.

  • Added ClientWindow and ClientWindowInfo classes for window management.

  • Introduced tests for getClientWindows functionality.

  • Updated Bazel build files to include new dependencies and modules.


Changes walkthrough 📝

Relevant files
Enhancement
ClientWindow.java
Introduced `ClientWindow` class for window identification

java/src/org/openqa/selenium/bidi/browser/ClientWindow.java

  • Added ClientWindow class with an id field.
  • Provided a constructor and getter for id.
  • +30/-0   
    ClientWindowInfo.java
    Added `ClientWindowInfo` class for window details               

    java/src/org/openqa/selenium/bidi/browser/ClientWindowInfo.java

  • Added ClientWindowInfo class with window properties.
  • Implemented a static method to parse JSON into ClientWindowInfo.
  • Provided getters for all window properties.
  • +73/-0   
    Browser.java
    Enhanced `Browser` module with `getClientWindows` method 

    java/src/org/openqa/selenium/bidi/module/Browser.java

  • Added getClientWindows method to fetch client windows.
  • Introduced mappers for parsing JSON responses into ClientWindowInfo.
  • +23/-0   
    Tests
    BrowserCommandsTest.java
    Added tests for `getClientWindows` functionality                 

    java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java

  • Added a test for the getClientWindows method.
  • Verified the correctness of ClientWindowInfo properties.
  • +13/-0   
    Configuration changes
    BUILD.bazel
    Added Bazel build file for `browser` package                         

    java/src/org/openqa/selenium/bidi/browser/BUILD.bazel

  • Created Bazel build file for the browser package.
  • Included dependencies for the new ClientWindow and ClientWindowInfo
    classes.
  • +26/-0   
    BUILD.bazel
    Updated Bazel build for `Browser` module                                 

    java/src/org/openqa/selenium/bidi/module/BUILD.bazel

    • Updated Bazel build file to include browser package dependency.
    +1/-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.
  • @navin772 navin772 marked this pull request as ready for review February 5, 2025 13:12
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Null Handling

    The fromJson method does not handle null values for width, height, x, y coordinates which could be returned by the API. This may cause NullPointerException when accessing these values.

    public static ClientWindowInfo fromJson(Map<String, Object> map) {
      return new ClientWindowInfo(
          (String) map.get("clientWindow"),
          (String) map.get("state"),
          ((Number) map.get("width")).intValue(),
          ((Number) map.get("height")).intValue(),
          ((Number) map.get("x")).intValue(),
          ((Number) map.get("y")).intValue());
    }
    Test Coverage

    The test only validates presence of clientWindow and state. Additional assertions should verify width, height, x, y coordinates and test different window states.

      List<ClientWindowInfo> clientWindows = browser.getClientWindows();
    
      assertThat(clientWindows).isNotNull();
      assertThat(clientWindows.size()).isGreaterThan(0);
    
      ClientWindowInfo windowInfo = clientWindows.get(0);
      assertThat(windowInfo.getClientWindow()).isNotNull();
      assertThat(windowInfo.getState()).isNotNull();
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent null pointer exceptions

    Add null checks in fromJson() method to handle cases where map values might be
    null, preventing potential NullPointerException when accessing window
    properties.

    java/src/org/openqa/selenium/bidi/browser/ClientWindowInfo.java [40-48]

     public static ClientWindowInfo fromJson(Map<String, Object> map) {
    -    return new ClientWindowInfo(
    -        (String) map.get("clientWindow"),
    -        (String) map.get("state"),
    -        ((Number) map.get("width")).intValue(),
    -        ((Number) map.get("height")).intValue(),
    -        ((Number) map.get("x")).intValue(),
    -        ((Number) map.get("y")).intValue());
    +    String clientWindow = (String) map.get("clientWindow");
    +    String state = (String) map.get("state");
    +    Integer width = map.get("width") != null ? ((Number) map.get("width")).intValue() : null;
    +    Integer height = map.get("height") != null ? ((Number) map.get("height")).intValue() : null;
    +    Integer x = map.get("x") != null ? ((Number) map.get("x")).intValue() : null;
    +    Integer y = map.get("y") != null ? ((Number) map.get("y")).intValue() : null;
    +    return new ClientWindowInfo(clientWindow, state, width, height, x, y);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion addresses a critical issue by adding null checks to prevent potential NullPointerExceptions when parsing JSON data. This is particularly important for robust API handling where some window properties might be optional.

    High
    Handle null response data safely

    Add null check for clientWindowsResponse to prevent NullPointerException if the
    response doesn't contain the expected 'clientWindows' key.

    java/src/org/openqa/selenium/bidi/module/Browser.java [59-63]

     List<Map<String, Object>> clientWindowsResponse =
         (List<Map<String, Object>>) response.get("clientWindows");
    +List<ClientWindowInfo> clientWindows = new ArrayList<>();
    +if (clientWindowsResponse != null) {
    +    clientWindowsResponse.forEach(map -> clientWindows.add(ClientWindowInfo.fromJson(map)));
    +}
     
    -List<ClientWindowInfo> clientWindows = new ArrayList<>();
    -clientWindowsResponse.forEach(map -> clientWindows.add(ClientWindowInfo.fromJson(map)));
    -
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds important null-safety check for the clientWindows response, preventing potential runtime crashes if the API response is missing the expected data structure.

    Medium

    @navin772
    Copy link
    Member Author

    navin772 commented Feb 5, 2025

    Hi @pujagani, chrome 133 and Firefox 135 is released in stable channel and I have tested this BiDi method.
    I will open another PR for JS also.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants