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

[dotnet] Add CDP deprecation warning for Firefox #14759

Merged
merged 5 commits into from
Nov 20, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Nov 15, 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

Add a warning about Firefox CDP deprecation.

Motivation and Context

Allow users up to 2 versions to switch to BiDi and then remove the CDP support for Firefox.

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


Description

  • Added an Obsolete attribute to the GetDevToolsSession method in FirefoxDriver.cs to warn about the deprecation of CDP support for Firefox.
  • Introduced a logger in RemoteWebDriver.cs to log warnings when CDP is used with Firefox, encouraging the switch to WebDriver BiDi.

Changes walkthrough 📝

Relevant files
Documentation
FirefoxDriver.cs
Add deprecation warning for Firefox CDP support                   

dotnet/src/webdriver/Firefox/FirefoxDriver.cs

  • Added an Obsolete attribute to the GetDevToolsSession method.
  • Warns about the deprecation of CDP support for Firefox.
  • +1/-0     
    Enhancement
    RemoteWebDriver.cs
    Implement logging for Firefox CDP deprecation warning       

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs

  • Introduced logging for deprecation warnings.
  • Added a logger to the RemoteWebDriver class.
  • Logs a warning if CDP is used with Firefox.
  • +11/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 15, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 25d1f6e)

    Here are some key observations to aid the review process:

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

    Code Duplication
    The deprecation warning message is duplicated between FirefoxDriver.cs and RemoteWebDriver.cs. Consider extracting it to a constant or resource file to maintain consistency and ease future updates.

    Logging Check
    The logging level check may be redundant since most logging frameworks handle this internally. Consider removing the explicit IsEnabled check unless there's significant performance overhead in constructing the log message.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 15, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 25d1f6e
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add defensive programming checks to prevent null reference exceptions

    Add null check for Capabilities to prevent potential NullReferenceException.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [431]

    -if (this.Capabilities.GetCapability(CapabilityType.BrowserName) == "firefox")
    +if (this.Capabilities?.GetCapability(CapabilityType.BrowserName) == "firefox")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a null check is important for preventing potential runtime exceptions, as Capabilities could be null in certain scenarios, making this a valuable defensive programming practice.

    7
    Best practice
    Replace magic strings with named constants to improve maintainability

    Use string constant for "firefox" to avoid magic strings and potential typos.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [431]

    -if (this.Capabilities.GetCapability(CapabilityType.BrowserName) == "firefox")
    +private const string FirefoxBrowserName = "firefox";
    +...
    +if (this.Capabilities.GetCapability(CapabilityType.BrowserName) == FirefoxBrowserName)
    Suggestion importance[1-10]: 5

    Why: Using a constant for the browser name would improve code maintainability and reduce the risk of typos, especially since "firefox" appears in multiple places in the codebase.

    5
    Performance
    Cache frequently accessed values to improve performance and readability

    Cache the browser name value to avoid repeated capability lookups in the
    GetDevToolsSession method.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [431]

    -if (this.Capabilities.GetCapability(CapabilityType.BrowserName) == "firefox")
    +string browserName = this.Capabilities.GetCapability(CapabilityType.BrowserName) as string;
    +if (browserName == "firefox")
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Caching the browser name would slightly improve performance by avoiding repeated capability lookups, though the impact is minimal since this method is not called frequently.

    4

    💡 Need additional feedback ? start a PR chat


    Previous suggestions

    ✅ Suggestions up to commit 0fc56db
    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Extract duplicated string literal into a shared constant to improve maintainability

    Consider extracting the Firefox CDP deprecation warning message into a constant
    since it's duplicated in FirefoxDriver.cs. This improves maintainability and
    consistency.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [434]

    -_logger.Warn("CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.");
    +private const string FirefoxCdpDeprecationMessage = "CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.";
    +// ...
    +_logger.Warn(FirefoxCdpDeprecationMessage);
    Suggestion importance[1-10]: 7

    Why: The suggestion identifies a valid maintainability issue where the same deprecation message is duplicated across files. Extracting it to a constant would make future updates easier and ensure consistency.

    7
    Best practice
    ✅ Fix inconsistent code indentation to improve readability
    Suggestion Impact:The indentation of the logger check block was corrected, aligning the opening brace with the if statement, improving readability.

    code diff:

                     if (_logger.IsEnabled(LogEventLevel.Warn))
    -                 {
    -                _logger.Warn("CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.");
    +                {
    +                    _logger.Warn("CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.");

    The indentation of the logger check block is inconsistent. Align the opening brace
    with the if statement.

    dotnet/src/webdriver/Remote/RemoteWebDriver.cs [432-435]

     if (_logger.IsEnabled(LogEventLevel.Warn))
    -             {
    -            _logger.Warn("CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.");
    -            }
    +{
    +    _logger.Warn("CDP support for Firefox is deprecated and will be removed in future versions. Please switch to WebDriver BiDi.");
    +}
    Suggestion importance[1-10]: 4

    Why: The suggestion correctly identifies an indentation inconsistency in the code that affects readability, though it's a relatively minor issue that doesn't affect functionality.

    4

    @pujagani pujagani marked this pull request as draft November 15, 2024 09:13
    Co-authored-by: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com>
    @pujagani pujagani marked this pull request as ready for review November 18, 2024 04:34
    Copy link
    Contributor

    Persistent review updated to latest commit 25d1f6e

    @pujagani pujagani merged commit 8f725b3 into SeleniumHQ:trunk Nov 20, 2024
    10 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.

    2 participants