Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Dec 15, 2025

User description

💥 What does this PR do?

This PR introduces a new class named LocalWebDriver that is a common base class that all local WebDriver classes inherit from. This allows us to implement common code across all local WebDriver instances without repeating code in every class.

🔧 Implementation Notes

You will get a TypeError if you try to instantiate LocalWebDriver directly. I was going to implement it as an Abstract Base Class (using abc.ABC), but it doesn't provide any abstract methods and also inherits from RemoteWebDriver... so I felt it was better to just leave it as a concrete class, but make it impossible to instantiate.

💡 Additional Considerations

This shouldn't affect the public API... it is just an internal class hierarchy change for easier maintenance and to remove duplicated code.


PR Type

Enhancement


Description

  • Introduces LocalWebDriver base class for all local WebDriver implementations

  • Consolidates common code (quit, download methods) into single base class

  • Removes duplicate code across Chrome, Firefox, IE, Safari, WebKitGTK, WPEWebKit drivers

  • Prevents direct instantiation of LocalWebDriver via __new__ override


Diagram Walkthrough

flowchart LR A["RemoteWebDriver"] -->|inherits| B["LocalWebDriver"] B -->|inherited by| C["ChromiumDriver"] B -->|inherited by| D["Firefox WebDriver"] B -->|inherited by| E["IE WebDriver"] B -->|inherited by| F["Safari WebDriver"] B -->|inherited by| G["WebKitGTK WebDriver"] B -->|inherited by| H["WPEWebKit WebDriver"] B -->|provides| I["quit, download methods"] B -->|prevents| J["Direct instantiation"] 
Loading

File Walkthrough

Relevant files
Enhancement
webdriver.py
Create LocalWebDriver base class with common functionality

py/selenium/webdriver/common/webdriver.py

  • New file creating LocalWebDriver base class inheriting from
    RemoteWebDriver
  • Implements __new__ to prevent direct instantiation of LocalWebDriver
  • Sets _is_remote = False in __init__ for all local drivers
  • Consolidates quit() method with service cleanup logic
  • Consolidates download_file(), get_downloadable_files(),
    delete_downloadable_files() as NotImplementedError stubs
+54/-0   
webdriver.py
Refactor ChromiumDriver to use LocalWebDriver base class 

py/selenium/webdriver/chromium/webdriver.py

  • Changed ChromiumDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed quit() method (now in LocalWebDriver)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
+2/-26   
webdriver.py
Refactor Firefox WebDriver to use LocalWebDriver base class

py/selenium/webdriver/firefox/webdriver.py

  • Changed WebDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed quit() method (now in LocalWebDriver)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
+4/-26   
webdriver.py
Refactor IE WebDriver to use LocalWebDriver base class     

py/selenium/webdriver/ie/webdriver.py

  • Changed WebDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed quit() method (now in LocalWebDriver)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
+2/-23   
webdriver.py
Refactor Safari WebDriver to use LocalWebDriver base class

py/selenium/webdriver/safari/webdriver.py

  • Changed WebDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
  • Kept Safari-specific quit() method with http_client.BadStatusLine
    exception handling
+2/-13   
webdriver.py
Refactor WebKitGTK WebDriver to use LocalWebDriver base class

py/selenium/webdriver/webkitgtk/webdriver.py

  • Changed WebDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed quit() method (now in LocalWebDriver)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Removed unused http.client import
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
+2/-22   
webdriver.py
Refactor WPEWebKit WebDriver to use LocalWebDriver base class

py/selenium/webdriver/wpewebkit/webdriver.py

  • Changed WebDriver to inherit from LocalWebDriver instead of
    RemoteWebDriver
  • Removed _is_remote = False assignment (now in LocalWebDriver.__init__)
  • Removed quit() method (now in LocalWebDriver)
  • Removed download_file(), get_downloadable_files(),
    delete_downloadable_files() methods (now in LocalWebDriver)
  • Removed unused http.client import
  • Updated import to use LocalWebDriver from
    selenium.webdriver.common.webdriver
+2/-22   

@selenium-ci selenium-ci added the C-py Python Bindings label Dec 15, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Swallowed quit errors: LocalWebDriver.quit() catches Exception and silently suppresses it without logging or
actionable context, making failures harder to diagnose.

Referred Code
def quit(self) -> None: """Closes the browser and shuts down the driver executable.""" try: super().quit() except Exception: # We don't care about the message because something probably has gone wrong pass

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 15, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix incorrect __new__ method signature
Suggestion Impact:The commit addressed the core issue by removing *args/**kwargs from the object.__new__ call (changing it to object.__new__(cls)), which prevents the TypeError the suggestion warned about. However, it did not switch to super().__new__(cls) as suggested.

code diff:

 def __new__(cls, *args, **kwargs): if cls is LocalWebDriver: raise TypeError(f"Only children of '{cls.__name__}' may be instantiated") - return object.__new__(cls, *args, **kwargs) + return object.__new__(cls)

**In LocalWebDriver.new, replace the incorrect call to object.new(cls,
*args, kwargs) with super().new(cls) to prevent a TypeError during
instantiation.

py/selenium/webdriver/common/webdriver.py [29-32]

 def __new__(cls, *args, **kwargs): if cls is LocalWebDriver: raise TypeError(f"Only children of '{cls.__name__}' may be instantiated") - return object.__new__(cls, *args, **kwargs) + return super().__new__(cls)

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug in the __new__ method that would cause a TypeError when instantiating any subclass of LocalWebDriver, making the core refactoring of the PR non-functional.

High
Learned
best practice
Make resource cleanup exception-safe

Make the cleanup in finally robust by handling cases where service is
missing/partially initialized and by not letting service.stop() raise during
cleanup.

py/selenium/webdriver/common/webdriver.py [34-42]

 def quit(self) -> None: """Closes the browser and shuts down the driver executable.""" try: super().quit() except Exception: # We don't care about the message because something probably has gone wrong pass finally: - self.service.stop() + service = getattr(self, "service", None) + if service is not None: + try: + service.stop() + except Exception: + pass
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Always perform explicit, exception-safe cleanup of external resources in finally to prevent leaks and secondary failures during cleanup.

Low
  • Update
@titusfortner
Copy link
Member

How difficult would it be not to inherit from RemoteWebDriver any more? Ruby, JS & .NET all moved away from that hierarchy. Not sure it is worth it here, but just curious.

@cgoldberg
Copy link
Member Author

How difficult would it be not to inherit from RemoteWebDriver any more?

That was actually my original thought... I wanted a BaseWebDriver and then RemoteWebDriver and LocalWebDriver as children of that. This was just easier for now.

It's not a huge amount of work, but there is a decent amount of reshuffling/copying/renaming that has to be done. I'll get to it eventually :)

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

Labels

3 participants