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][js][rb][py][dotnet] Remove firefox cdp #15200

Merged
merged 17 commits into from
Feb 3, 2025

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jan 31, 2025

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.

Motivation and Context

Related to #11736

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


Description

  • Removed Firefox CDP (Chrome DevTools Protocol) support across all bindings.

    • Updated FirefoxDriver implementations in Java, .NET, Python, Ruby, and JavaScript.
    • Removed CDP-related methods, capabilities, and warnings.
  • Transitioned to WebDriver BiDi as the recommended protocol for Firefox.

  • Updated and removed tests related to Firefox CDP.

    • Adjusted test configurations and removed Firefox-specific CDP tests.

Changes walkthrough 📝

Relevant files
Enhancement
7 files
FirefoxDriver.java
Removed Firefox CDP support and related capabilities         
+2/-104 
FirefoxOptions.java
Removed CDP-related capability from Firefox options           
+0/-1     
FirefoxDriver.cs
Removed CDP support and related methods from .NET FirefoxDriver
+1/-86   
webdriver.js
Removed Firefox CDP support and added error for usage       
+2/-8     
driver.rb
Removed CDP support from Ruby FirefoxDriver                           
+0/-18   
webdriver.py
Removed Firefox CDP support and added error for usage       
+3/-15   
driver.rbs
Removed CDP-related methods from Ruby type signatures       
+0/-6     
Tests
4 files
CdpFacadeTest.java
Removed Firefox-specific CDP test annotations                       
+0/-4     
devtools_test.js
Removed Firefox from CDP-related tests                                     
+15/-18 
devtools_spec.rb
Removed Firefox-specific CDP tests                                             
+9/-28   
devtools_tests.py
Removed Firefox-specific CDP warnings in tests                     
+2/-7     
Configuration changes
1 files
BUILD.bazel
Removed Firefox from CDP test configurations                         
+0/-1     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @pujagani
    Copy link
    Contributor Author

    pujagani commented Feb 3, 2025

    Related blog post: SeleniumHQ/seleniumhq.github.io#2159

    @pujagani
    Copy link
    Contributor Author

    pujagani commented Feb 3, 2025

    Unable to reproduce the failing tests locally so not sure what is happening here. I do not think it is related to the changes made here.

    @pujagani pujagani marked this pull request as ready for review February 3, 2025 09:03
    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 3, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The error message for Firefox CDP removal should include more context about using WebDriver BiDi as the alternative, similar to the previous warning message.

    throw new Error('CDP support for Firefox is removed. Please switch to WebDriver BiDi.')
    Configuration Validation

    The remote.active-protocols preference is still being set to 3 despite CDP being removed. This preference may need to be updated or removed if it was specifically for CDP support.

    // https://fxdx.dev/deprecating-cdp-support-in-firefox-embracing-the-future-with-webdriver-bidi/.
    addPreference("remote.active-protocols", 3);

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Improve error message with documentation link

    The error message for Firefox CDP removal should include more context about
    WebDriver BiDi as the recommended alternative, including a link to
    documentation.

    javascript/node/selenium-webdriver/lib/webdriver.js [1247]

    -throw new Error('CDP support for Firefox is removed. Please switch to WebDriver BiDi.')
    +throw new Error('CDP support for Firefox is removed. Please switch to WebDriver BiDi - see https://www.selenium.dev/documentation/webdriver/bidirectional/ for migration guide.')
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves the error message by adding a helpful documentation link for users to learn about WebDriver BiDi migration, making the transition smoother for developers. However, it's a minor enhancement to an already clear error message.

    5

    @pujagani pujagani merged commit 29e858f into SeleniumHQ:trunk Feb 3, 2025
    27 of 34 checks passed
    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.

    1 participant