8000 [py][bidi]: add note for `enable_webextensions = False` by navin772 · Pull Request #15981 · SeleniumHQ/selenium · GitHub
[go: up one dir, main page]
Skip to content

[py][bidi]: add note for enable_webextensions = False #15981

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 30, 2025
Merged

[py][bidi]: add note for enable_webextensions = False #15981

merged 3 commits into from
Jun 30, 2025

Conversation

navin772
Copy link
Member
@navin772 navin772 commented Jun 30, 2025 •
edited by qodo-merge-pro bot
Loading

User description

🔗 Related Issues

Addresses the concerns mentioned in - #15794 (comment)

💥 What does this PR do?

Adds a relevant note when enable_webextensions is set to False.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Documentation

Description

  • Add clarification note for enable_webextensions = False behavior

  • Document that disabling removes manually added flags

  • Fix indentation in existing documentation

Changes diagram

flowchart LR
  A["enable_webextensions = False"] --> B["Removes flags automatically"]
  B --> C["Even manually added flags"]
  C --> D["Updated documentation"]
Loading

Changes walkthrough 📝

8000
Relevant files
Documentation
options.py
Enhanced webextensions documentation with removal behavior

py/selenium/webdriver/chromium/options.py

  • Added note explaining flag removal behavior when webextensions
    disabled
  • Clarified that manually added flags are also removed
  • Fixed indentation alignment in existing documentation
    		• +4/-2     
    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 requested a review from shbenzer June 30, 2025 07:23
    @selenium-ci selenium-ci added the C-py Python Bindings label Jun 30, 2025
    @navin772 navin772 requested a review from cgoldberg June 30, 2025 07:23
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link href on click() for Firefox
    • Ensure compatibility with Firefox 42.0
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4
    • Resolve connection refused errors for subsequent ChromeDriver instances
    • Ensure compatibility with Chrome 65.0.3325.181 and ChromeDriver 2.35

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

    Documentation Clarity

    The added note about flag removal behavior when webextensions is disabled should be verified for accuracy and completeness regarding the actual implementation behavior.

     
    - When disabled, this removes BOTH flags listed above, even if they were manually added via add_argument()
  before enabling webextensions.

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor
    qodo-merge-pro bot commented Jun 30, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Verify flag removal implementation matches documentation

    The note mentions removing flags when disabled, but the actual implementation
    logic for flag removal is not shown in the diff. Verify that the implementation
    actually removes these flags when value is False to match the documentation.

    py/selenium/webdriver/chromium/options.py [156-157]

    +- When disabled, this removes BOTH flags listed above, even if they were manually added via add_argument()
+  before enabling webextensions.
 
-
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that the added documentation about flag removal needs a corresponding implementation, which is not visible in the diff, and rightly asks for verification.

    		Medium
    • Update

    cgoldberg
    cgoldberg approved these changes Jun 30, 2025
    View reviewed changes
    Copy link
    Contributor
    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @navin772
    Copy link
    Member Author

    The failing test is unrelated and fails on trunk also.

    @navin772 navin772 merged commit 954ffec into SeleniumHQ:trunk Jun 30, 2025
    16 of 18 checks passed
    @navin772 navin772 deleted the py-add-note-webex branch June 30, 2025 16:58
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @cgoldberg cgoldberg cgoldberg approved these changes

    @shbenzer shbenzer shbenzer approved these changes

    Assignees
    No one assigned
    Labels
    C-py Python Bindings Review effort 1/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    4 participants
    @navin772 @cgoldberg @shbenzer @selenium-ci
    0