Skip to content

[java][BiDi] implement web extensions #15660

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

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

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Apr 23, 2025

User description

🔗 Related Issues

Implements #15585

💥 What does this PR do?

Implements #15585 for Java binding

🔧 Implementation Notes

The PR will fail on stable firefox and needs firefox to be 138 version as stated in #15585 (comment)

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Add BiDi web extension install/uninstall API for Java

    • Implements webExtension.install and webExtension.uninstall commands
    • Supports extension install via path, archive, or base64
  • Introduce new classes for extension data handling

  • Add comprehensive tests for extension installation methods

  • Update Bazel build files for new module and test integration


Changes walkthrough 📝

Relevant files
Enhancement
7 files
ExtensionArchivePath.java
Add class for extension install via archive path                 
+34/-0   
ExtensionBase64Encoded.java
Add class for extension install via base64 encoding           
+34/-0   
ExtensionData.java
Add abstract base class for extension data types                 
+24/-0   
ExtensionPath.java
Add class for extension install via directory path             
+34/-0   
InstallExtensionParameters.java
Add parameters class for extension installation                   
+31/-0   
UninstallExtensionParameters.java
Add parameters class for extension uninstallation               
+29/-0   
WebExtension.java
Add main API for BiDi web extension install/uninstall       
+50/-0   
Configuration changes
5 files
BUILD.bazel
Add Bazel build file for webextension module                         
+25/-0   
BUILD.bazel
Add webextension dependency to BiDi module build                 
+1/-0     
BUILD.bazel
Add webextension dependency to remote module build             
+1/-0     
BUILD.bazel
Add webextension dependency to BiDi input test build         
+1/-0     
BUILD.bazel
Add Bazel build file for webextension tests                           
+37/-0   
Tests
1 files
WebExtensionTest.java
Add tests for BiDi web extension install/uninstall             
+76/-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.
  • @selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Apr 23, 2025
    @Delta456 Delta456 marked this pull request as ready for review May 14, 2025 10:42
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15585 - PR Code Verified

    Compliant requirements:

    • Implement WebDriver BiDi commands for installing and uninstalling web extensions
    • Implement the following BiDi commands:
      • webExtension.install
      • webExtension.uninstall

    Requires further human verification:

    • Support Chrome and Firefox implementations (PR mentions it will fail on stable Firefox and needs Firefox version 138)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Variable Naming Inconsistency

    The test class uses inconsistent variable naming conventions. In some methods the WebExtension variable is capitalized (Extension) while in others it's lowercase (extension).

    WebExtension Extension = new WebExtension(driver);
    var ex =
    Assertion Style

    The tests use direct assert statements instead of more descriptive assertion libraries that would provide better failure messages.

    assert exIn.get("extension").equals(extensionData.get("id"));

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Learned
    best practice
    Add parameter null check

    Add a null check for the path parameter in the constructor to prevent
    NullPointerException. Use the Require utility class that's already being used
    elsewhere in the codebase to validate the input parameter.

    java/src/org/openqa/selenium/bidi/webextension/ExtensionArchivePath.java [25-27]

     public ExtensionArchivePath(String path) {
    -  this.path = path;
    +  this.path = org.openqa.selenium.internal.Require.nonNull("path", path);
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why:
    Relevant best practice - Add null checks for parameters and properties before using them to prevent NullReferenceExceptions.

    Low
    General
    Fix variable naming convention

    Variable naming convention is inconsistent. The variable Extension starts with
    an uppercase letter, which violates Java naming conventions. Use camelCase for
    variable names to maintain consistency with the rest of the codebase.

    java/test/org/openqa/selenium/bidi/webextension/WebExtensionTest.java [53-57]

    -WebExtension Extension = new WebExtension(driver);
    +WebExtension extension = new WebExtension(driver);
     var ex =
    -    Extension.install(
    +    extension.install(
             new InstallExtensionParameters(new ExtensionArchivePath(path.toString())));
     assert ex.get("extension").equals(extensionData.get("id"));
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion correctly identifies and fixes a violation of Java variable naming conventions, improving code readability and maintainability, but it does not affect functionality or correctness.

    Low
    • More

    @Delta456 Delta456 requested a review from pujagani May 14, 2025 10:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants