Skip to content

[py][bidi]: Implement BiDi browser module #15616

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

Merged
merged 10 commits into from
Apr 11, 2025

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Apr 11, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements the BiDi browser module for the python bindings - https://w3c.github.io/webdriver-bidi/#module-browser.

These are the commands that have been added:

  1. create_user_context
  2. get_user_contexts
  3. remove_user_context
  4. get_client_windows

The PR also adds a new common.py file in the bidi/ directory that will contain the commonly used functions like command_builder to avoid repetition.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Implemented BiDi browser module for Python bindings.

  • Added commands for user context and client window management.

  • Refactored common BiDi command builder to avoid duplication.

  • Introduced comprehensive tests for BiDi browser functionality.


Changes walkthrough 📝

Relevant files
Enhancement
browser.py
Added BiDi browser module implementation                                 

py/selenium/webdriver/common/bidi/browser.py

  • Added BiDi browser module implementation.
  • Introduced user context and client window management commands.
  • Defined ClientWindowInfo and ClientWindowState classes.
  • +190/-0 
    common.py
    Added reusable BiDi command builder                                           

    py/selenium/webdriver/common/bidi/common.py

  • Added a reusable command_builder function for BiDi commands.
  • Simplified command construction for BiDi modules.
  • +38/-0   
    network.py
    Refactored BiDi network commands to use common builder     

    py/selenium/webdriver/common/bidi/network.py

  • Replaced inline command builders with the common command_builder.
  • Simplified network-related BiDi command execution.
  • +11/-33 
    webdriver.py
    Integrated BiDi browser module into WebDriver                       

    py/selenium/webdriver/remote/webdriver.py

  • Integrated BiDi browser module into WebDriver.
  • Added browser property for accessing BiDi browser commands.
  • +25/-0   
    Miscellaneous
    __init__.py
    Added init file for BiDi tests                                                     

    py/test/selenium/webdriver/common/bidi/init.py

    • Added initialization file for BiDi tests.
    +17/-0   
    Tests
    bidi_browser_tests.py
    Added tests for BiDi browser module                                           

    py/test/selenium/webdriver/common/bidi_browser_tests.py

  • Added tests for BiDi browser module functionality.
  • Covered user context and client window management.
  • Validated ClientWindowState constants.
  • +99/-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-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Apr 11, 2025
    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 issue where JavaScript in link's href is not triggered on click() in Firefox 42.0 with Selenium 2.48.0/2.48.2
    • Ensure click() properly executes JavaScript in href attributes
    • Maintain backward compatibility with previous working behavior in Selenium 2.47.1

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver multiple times
    • Address issue specific to Ubuntu 16.04.4 with Chrome 65.0.3325.181 and ChromeDriver 2.35
    • Ensure subsequent ChromeDriver instances work properly after first instance

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

    Error Handling

    The create_user_context and get_client_windows methods don't include error handling for potential BiDi protocol errors or connection issues. Consider adding try/except blocks to handle potential exceptions.

    def create_user_context(self) -> str:
        """Creates a new user context.
    
        Returns:
        -------
            str: The ID of the created user context.
        """
        result = self.conn.execute(command_builder("browser.createUserContext", {}))
        return result["userContext"]
    
    def get_user_contexts(self) -> List[str]:
        """Gets all user contexts.
    
        Returns:
        -------
            List[str]: A list of user context IDs.
        """
        result = self.conn.execute(command_builder("browser.getUserContexts", {}))
        return [context_info["userContext"] for context_info in result["userContexts"]]
    
    def remove_user_context(self, user_context_id: str) -> None:
        """Removes a user context.
    
        Parameters:
        -----------
            user_context_id: The ID of the user context to remove.
    
        Raises:
        ------
            Exception: If the user context ID is "default" or does not exist.
        """
        params = {"userContext": user_context_id}
        self.conn.execute(command_builder("browser.removeUserContext", params))
    
    def get_client_windows(self) -> List[ClientWindowInfo]:
        """Gets all client windows.
    
        Returns:
        -------
            List[ClientWindowInfo]: A list of client window information.
        """
        result = self.conn.execute(command_builder("browser.getClientWindows", {}))
        return [ClientWindowInfo.from_dict(window) for window in result["clientWindows"]]
    Command Builder Return

    The command_builder function uses yield to create a generator but the docstring indicates it returns a response. This may cause confusion as the actual response comes from the yield statement, not the function return.

    def command_builder(method: str, params: Dict = None) -> Dict:
        """Build a command iterator to send to the BiDi protocol.
    
        Parameters:
        -----------
            method: The method to execute.
            params: The parameters to pass to the method. Default is None.
    
        Returns:
        --------
            The response from the command execution.
        """
        if params is None:
            params = {}
    
        command = {"method": method, "params": params}
        cmd = yield command
        return cmd
    Test Assertions

    The test_get_client_windows has assertions that may fail on some browser configurations where window coordinates could be zero or negative. Consider making these assertions more flexible.

    def test_get_client_windows(driver):
        """Test getting client windows."""
        client_windows = driver.browser.get_client_windows()
    
        assert client_windows is not None
        assert len(client_windows) > 0
    
        window_info = client_windows[0]
        assert isinstance(window_info, ClientWindowInfo)
        assert window_info.get_client_window() is not None
        assert window_info.get_state() is not None
        assert isinstance(window_info.get_state(), str)
        assert window_info.get_width() > 0
        assert window_info.get_height() > 0
        assert isinstance(window_info.is_active(), bool)
        assert window_info.get_x() > 0
        assert window_info.get_y() > 0

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 11, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent removing default context
    Suggestion Impact:The suggestion was directly implemented in the commit. The code now checks if the user_context_id is 'default' and raises an exception if it is, preventing the removal of the default user context.

    code diff:

    +        if user_context_id == "default":
    +            raise Exception("Cannot remove the default user context")
    +

    Add validation to prevent removing the "default" user context. The docstring
    mentions that an exception will be raised if the user context ID is "default",
    but there's no actual validation in the code to prevent this operation.

    py/selenium/webdriver/common/bidi/browser.py [168-180]

     def remove_user_context(self, user_context_id: str) -> None:
         """Removes a user context.
     
         Parameters:
         -----------
             user_context_id: The ID of the user context to remove.
     
         Raises:
         ------
             Exception: If the user context ID is "default" or does not exist.
         """
    +    if user_context_id == "default":
    +        raise Exception("Cannot remove the default user context")
    +        
         params = {"userContext": user_context_id}
         self.conn.execute(command_builder("browser.removeUserContext", params))

    [Suggestion has been applied]

    Suggestion importance[1-10]: 9

    __

    Why: The suggestion adds critical validation to prevent removing the "default" user context, which aligns with the documented behavior in the docstring. Without this check, users could attempt to remove the default context, potentially causing system instability or unexpected behavior.

    High
    Validate required fields

    Add validation for required fields in the from_dict method. Currently, if any of
    the expected keys are missing in the input dictionary, the method will silently
    pass None values to the constructor, which could lead to unexpected behavior or
    errors later.

    py/selenium/webdriver/common/bidi/browser.py [117-137]

     @classmethod
     def from_dict(cls, data: Dict) -> "ClientWindowInfo":
         """Creates a ClientWindowInfo instance from a dictionary.
     
         Parameters:
         -----------
             data: A dictionary containing the client window information.
     
         Returns:
         -------
             ClientWindowInfo: A new instance of ClientWindowInfo.
    +        
    +    Raises:
    +    ------
    +        KeyError: If any required field is missing from the data dictionary.
         """
    +    required_fields = ["clientWindow", "state", "width", "height", "x", "y", "active"]
    +    for field in required_fields:
    +        if field not in data:
    +            raise KeyError(f"Required field '{field}' missing from client window data")
    +            
         return cls(
    -        client_window=data.get("clientWindow"),
    -        state=data.get("state"),
    -        width=data.get("width"),
    -        height=data.get("height"),
    -        x=data.get("x"),
    -        y=data.get("y"),
    -        active=data.get("active"),
    +        client_window=data["clientWindow"],
    +        state=data["state"],
    +        width=data["width"],
    +        height=data["height"],
    +        x=data["x"],
    +        y=data["y"],
    +        active=data["active"],
         )
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion adds important validation for required fields in the from_dict method, preventing silent failures when dictionary keys are missing. This would catch potential bugs early and provide clear error messages instead of allowing None values to propagate through the system.

    Medium
    • Update

    @navin772
    Copy link
    Member Author

    @shbenzer thank you for implementing the Network module in python, I have implemented the Browser module taking inspiration from it! Feel free to review this.

    Copy link
    Contributor

    @shbenzer shbenzer left a comment

    Choose a reason for hiding this comment

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

    This looks great!

    Could we add the browser.SetClientWindowState browser command too?

    @shbenzer shbenzer merged commit 963bf95 into SeleniumHQ:trunk Apr 11, 2025
    17 checks passed
    @shbenzer
    Copy link
    Contributor

    Great work @navin772!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 3/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants