Skip to content

[py][bidi]: Implement BiDi browsing_context module #15631

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Apr 16, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the browsing context module for the python bindings - https://w3c.github.io/webdriver-bidi/#module-browsingContext

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

PR Type

Enhancement, Tests


Description

  • Implements BiDi browsingContext module for Python bindings

  • Adds comprehensive tests for BiDi browsing context functionality

  • Integrates new module into Remote WebDriver API

  • Supports context creation, navigation, screenshot, print, viewport, and event handling


Changes walkthrough 📝

Relevant files
Enhancement
browsing_context.py
Add BiDi browsingContext module implementation                     

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

  • Implements BiDi browsingContext module for Python
  • Provides context management, navigation, screenshot, print, viewport,
    and event handling
  • Defines related data classes and event parameter structures
  • Includes detailed docstrings for all methods
  • +732/-0 
    webdriver.py
    Integrate browsingContext module into Remote WebDriver     

    py/selenium/webdriver/remote/webdriver.py

  • Imports and integrates new BrowsingContext module
  • Adds browsing_context property to Remote WebDriver
  • Enables access to BiDi browsing context commands via driver API
  • Updates initialization to support new module
  • +25/-0   
    Tests
    bidi_browsing_context_tests.py
    Add tests for BiDi browsingContext module                               

    py/test/selenium/webdriver/common/bidi_browsing_context_tests.py

  • Adds comprehensive tests for BiDi browsing context functionality
  • Covers context creation, navigation, screenshot, print, viewport, and
    history traversal
  • Tests user prompt handling and event-driven features
  • Validates integration with Selenium test pages
  • +417/-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 16, 2025
    @navin772
    Copy link
    Member Author

    I will update this PR to use the session class once this PR gets merged - #15615

    @navin772 navin772 marked this pull request as ready for review April 19, 2025 07:32
    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 32bit on 64bit machine
    • Ensure JavaScript alerts work properly when clicking links with JavaScript in href attribute
    • Fix regression between Selenium 2.47.1 and 2.48.0/2.48.2

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver
    • Address issue where subsequent ChromeDriver instantiations fail after first successful one
    • Fix for Ubuntu 16.04.4 with Linux 4.10.0, Selenium 3.9.0, Chrome 65.0.3325.181, ChromeDriver 2.35

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

    Error Handling

    The module lacks comprehensive error handling for BiDi protocol errors. Many methods don't handle potential exceptions from the BiDi protocol or validate input parameters before sending commands.

    def activate(self, context: str) -> None:
        """Activates and focuses the given top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID to activate.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {"context": context}
        self.conn.execute(command_builder("browsingContext.activate", params))
    
    def capture_screenshot(
        self,
        context: str,
        origin: str = "viewport",
        format: Optional[Dict] = None,
        clip: Optional[Dict] = None,
    ) -> str:
        """Captures an image of the given navigable, and returns it as a Base64-encoded string.
    
        Parameters:
        -----------
            context: The browsing context ID to capture.
            origin: The origin of the screenshot, either "viewport" or "document".
            format: The format of the screenshot.
            clip: The clip rectangle of the screenshot.
    
        Returns:
        -------
            str: The Base64-encoded screenshot.
        """
        params = {"context": context, "origin": origin}
        if format is not None:
            params["format"] = format
        if clip is not None:
            params["clip"] = clip
    
        result = self.conn.execute(command_builder("browsingContext.captureScreenshot", params))
        return result["data"]
    
    def close(self, context: str, prompt_unload: bool = False) -> None:
        """Closes a top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID to close.
            prompt_unload: Whether to prompt to unload.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {"context": context, "promptUnload": prompt_unload}
        self.conn.execute(command_builder("browsingContext.close", params))
    
    def create(
        self,
        type: str,
        reference_context: Optional[str] = None,
        background: bool = False,
        user_context: Optional[str] = None,
    ) -> str:
        """Creates a new navigable, either in a new tab or in a new window, and returns its navigable id.
    
        Parameters:
        -----------
            type: The type of the new navigable, either "tab" or "window".
            reference_context: The reference browsing context ID.
            background: Whether to create the new navigable in the background.
            user_context: The user context ID.
    
        Returns:
        -------
            str: The browsing context ID of the created navigable.
        """
        params = {"type": type}
        if reference_context is not None:
            params["referenceContext"] = reference_context
        if background is not None:
            params["background"] = background
        if user_context is not None:
            params["userContext"] = user_context
    
        result = self.conn.execute(command_builder("browsingContext.create", params))
        return result["context"]
    
    def get_tree(
        self,
        max_depth: Optional[int] = None,
        root: Optional[str] = None,
    ) -> List[BrowsingContextInfo]:
        """Returns a tree of all descendent navigables including the given parent itself, or all top-level contexts when no parent is provided.
    
        Parameters:
        -----------
            max_depth: The maximum depth of the tree.
            root: The root browsing context ID.
    
        Returns:
        -------
            List[BrowsingContextInfo]: A list of browsing context information.
        """
        params = {}
        if max_depth is not None:
            params["maxDepth"] = max_depth
        if root is not None:
            params["root"] = root
    
        result = self.conn.execute(command_builder("browsingContext.getTree", params))
        return [BrowsingContextInfo.from_json(context) for context in result["contexts"]]
    
    def handle_user_prompt(
        self,
        context: str,
        accept: Optional[bool] = None,
        user_text: Optional[str] = None,
    ) -> None:
        """Allows closing an open prompt.
    
        Parameters:
        -----------
            context: The browsing context ID.
            accept: Whether to accept the prompt.
            user_text: The text to enter in the prompt.
        """
        params = {"context": context}
        if accept is not None:
            params["accept"] = accept
        if user_text is not None:
            params["userText"] = user_text
    
        self.conn.execute(command_builder("browsingContext.handleUserPrompt", params))
    
    def locate_nodes(
        self,
        context: str,
        locator: Dict,
        max_node_count: Optional[int] = None,
        serialization_options: Optional[Dict] = None,
        start_nodes: Optional[List[Dict]] = None,
    ) -> List[Dict]:
        """Returns a list of all nodes matching the specified locator.
    
        Parameters:
        -----------
            context: The browsing context ID.
            locator: The locator to use.
            max_node_count: The maximum number of nodes to return.
            serialization_options: The serialization options.
            start_nodes: The start nodes.
    
        Returns:
        -------
            List[Dict]: A list of nodes.
        """
        params = {"context": context, "locator": locator}
        if max_node_count is not None:
            params["maxNodeCount"] = max_node_count
        if serialization_options is not None:
            params["serializationOptions"] = serialization_options
        if start_nodes is not None:
            params["startNodes"] = start_nodes
    
        result = self.conn.execute(command_builder("browsingContext.locateNodes", params))
        return result["nodes"]
    
    def navigate(
        self,
        context: str,
        url: str,
        wait: Optional[str] = None,
    ) -> Dict:
        """Navigates a navigable to the given URL.
    
        Parameters:
        -----------
            context: The browsing context ID.
            url: The URL to navigate to.
            wait: The readiness state to wait for.
    
        Returns:
        -------
            Dict: A dictionary containing the navigation result.
        """
        params = {"context": context, "url": url}
        if wait is not None:
            params["wait"] = wait
    
        result = self.conn.execute(command_builder("browsingContext.navigate", params))
        return result
    
    def print(
        self,
        context: str,
        background: bool = False,
        margin: Optional[Dict] = None,
        orientation: str = "portrait",
        page: Optional[Dict] = None,
        page_ranges: Optional[List[Union[int, str]]] = None,
        scale: float = 1.0,
        shrink_to_fit: bool = True,
    ) -> str:
        """Creates a paginated representation of a document, and returns it as a PDF document represented as a Base64-encoded string.
    
        Parameters:
        -----------
            context: The browsing context ID.
            background: Whether to include the background.
            margin: The margin parameters.
            orientation: The orientation, either "portrait" or "landscape".
            page: The page parameters.
            page_ranges: The page ranges.
            scale: The scale.
            shrink_to_fit: Whether to shrink to fit.
    
        Returns:
        -------
            str: The Base64-encoded PDF document.
        """
        params = {
            "context": context,
            "background": background,
            "orientation": orientation,
            "scale": scale,
            "shrinkToFit": shrink_to_fit,
        }
        if margin is not None:
            params["margin"] = margin
        if page is not None:
            params["page"] = page
        if page_ranges is not None:
            params["pageRanges"] = page_ranges
    
        result = self.conn.execute(command_builder("browsingContext.print", params))
        return result["data"]
    
    def reload(
        self,
        context: str,
        ignore_cache: Optional[bool] = None,
        wait: Optional[str] = None,
    ) -> Dict:
        """Reloads a navigable.
    
        Parameters:
        -----------
            context: The browsing context ID.
            ignore_cache: Whether to ignore the cache.
            wait: The readiness state to wait for.
    
        Returns:
        -------
            Dict: A dictionary containing the navigation result.
        """
        params = {"context": context}
        if ignore_cache is not None:
            params["ignoreCache"] = ignore_cache
        if wait is not None:
            params["wait"] = wait
    
        result = self.conn.execute(command_builder("browsingContext.reload", params))
        return result
    
    def set_viewport(
        self,
        context: Optional[str] = None,
        viewport: Optional[Dict] = None,
        device_pixel_ratio: Optional[float] = None,
        user_contexts: Optional[List[str]] = None,
    ) -> None:
        """Modifies specific viewport characteristics on the given top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID.
            viewport: The viewport parameters.
            device_pixel_ratio: The device pixel ratio.
            user_contexts: The user context IDs.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {}
        if context is not None:
            params["context"] = context
        if viewport is not None:
            params["viewport"] = viewport
        if device_pixel_ratio is not None:
            params["devicePixelRatio"] = device_pixel_ratio
        if user_contexts is not None:
            params["userContexts"] = user_contexts
    
        self.conn.execute(command_builder("browsingContext.setViewport", params))
    
    def traverse_history(self, context: str, delta: int) -> Dict:
        """Traverses the history of a given navigable by a delta.
    
        Parameters:
        -----------
            context: The browsing context ID.
            delta: The delta to traverse by.
    
        Returns:
        -------
            Dict: A dictionary containing the traverse history result.
        """
        params = {"context": context, "delta": delta}
        result = self.conn.execute(command_builder("browsingContext.traverseHistory", params))
        return result
    Incomplete Documentation

    While most methods have docstrings, they lack examples of possible exceptions that could be raised and don't document the format of complex parameters like viewport, clip, and margin objects.

    def activate(self, context: str) -> None:
        """Activates and focuses the given top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID to activate.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {"context": context}
        self.conn.execute(command_builder("browsingContext.activate", params))
    
    def capture_screenshot(
        self,
        context: str,
        origin: str = "viewport",
        format: Optional[Dict] = None,
        clip: Optional[Dict] = None,
    ) -> str:
        """Captures an image of the given navigable, and returns it as a Base64-encoded string.
    
        Parameters:
        -----------
            context: The browsing context ID to capture.
            origin: The origin of the screenshot, either "viewport" or "document".
            format: The format of the screenshot.
            clip: The clip rectangle of the screenshot.
    
        Returns:
        -------
            str: The Base64-encoded screenshot.
        """
        params = {"context": context, "origin": origin}
        if format is not None:
            params["format"] = format
        if clip is not None:
            params["clip"] = clip
    
        result = self.conn.execute(command_builder("browsingContext.captureScreenshot", params))
        return result["data"]
    
    def close(self, context: str, prompt_unload: bool = False) -> None:
        """Closes a top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID to close.
            prompt_unload: Whether to prompt to unload.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {"context": context, "promptUnload": prompt_unload}
        self.conn.execute(command_builder("browsingContext.close", params))
    
    def create(
        self,
        type: str,
        reference_context: Optional[str] = None,
        background: bool = False,
        user_context: Optional[str] = None,
    ) -> str:
        """Creates a new navigable, either in a new tab or in a new window, and returns its navigable id.
    
        Parameters:
        -----------
            type: The type of the new navigable, either "tab" or "window".
            reference_context: The reference browsing context ID.
            background: Whether to create the new navigable in the background.
            user_context: The user context ID.
    
        Returns:
        -------
            str: The browsing context ID of the created navigable.
        """
        params = {"type": type}
        if reference_context is not None:
            params["referenceContext"] = reference_context
        if background is not None:
            params["background"] = background
        if user_context is not None:
            params["userContext"] = user_context
    
        result = self.conn.execute(command_builder("browsingContext.create", params))
        return result["context"]
    
    def get_tree(
        self,
        max_depth: Optional[int] = None,
        root: Optional[str] = None,
    ) -> List[BrowsingContextInfo]:
        """Returns a tree of all descendent navigables including the given parent itself, or all top-level contexts when no parent is provided.
    
        Parameters:
        -----------
            max_depth: The maximum depth of the tree.
            root: The root browsing context ID.
    
        Returns:
        -------
            List[BrowsingContextInfo]: A list of browsing context information.
        """
        params = {}
        if max_depth is not None:
            params["maxDepth"] = max_depth
        if root is not None:
            params["root"] = root
    
        result = self.conn.execute(command_builder("browsingContext.getTree", params))
        return [BrowsingContextInfo.from_json(context) for context in result["contexts"]]
    
    def handle_user_prompt(
        self,
        context: str,
        accept: Optional[bool] = None,
        user_text: Optional[str] = None,
    ) -> None:
        """Allows closing an open prompt.
    
        Parameters:
        -----------
            context: The browsing context ID.
            accept: Whether to accept the prompt.
            user_text: The text to enter in the prompt.
        """
        params = {"context": context}
        if accept is not None:
            params["accept"] = accept
        if user_text is not None:
            params["userText"] = user_text
    
        self.conn.execute(command_builder("browsingContext.handleUserPrompt", params))
    
    def locate_nodes(
        self,
        context: str,
        locator: Dict,
        max_node_count: Optional[int] = None,
        serialization_options: Optional[Dict] = None,
        start_nodes: Optional[List[Dict]] = None,
    ) -> List[Dict]:
        """Returns a list of all nodes matching the specified locator.
    
        Parameters:
        -----------
            context: The browsing context ID.
            locator: The locator to use.
            max_node_count: The maximum number of nodes to return.
            serialization_options: The serialization options.
            start_nodes: The start nodes.
    
        Returns:
        -------
            List[Dict]: A list of nodes.
        """
        params = {"context": context, "locator": locator}
        if max_node_count is not None:
            params["maxNodeCount"] = max_node_count
        if serialization_options is not None:
            params["serializationOptions"] = serialization_options
        if start_nodes is not None:
            params["startNodes"] = start_nodes
    
        result = self.conn.execute(command_builder("browsingContext.locateNodes", params))
        return result["nodes"]
    
    def navigate(
        self,
        context: str,
        url: str,
        wait: Optional[str] = None,
    ) -> Dict:
        """Navigates a navigable to the given URL.
    
        Parameters:
        -----------
            context: The browsing context ID.
            url: The URL to navigate to.
            wait: The readiness state to wait for.
    
        Returns:
        -------
            Dict: A dictionary containing the navigation result.
        """
        params = {"context": context, "url": url}
        if wait is not None:
            params["wait"] = wait
    
        result = self.conn.execute(command_builder("browsingContext.navigate", params))
        return result
    
    def print(
        self,
        context: str,
        background: bool = False,
        margin: Optional[Dict] = None,
        orientation: str = "portrait",
        page: Optional[Dict] = None,
        page_ranges: Optional[List[Union[int, str]]] = None,
        scale: float = 1.0,
        shrink_to_fit: bool = True,
    ) -> str:
        """Creates a paginated representation of a document, and returns it as a PDF document represented as a Base64-encoded string.
    
        Parameters:
        -----------
            context: The browsing context ID.
            background: Whether to include the background.
            margin: The margin parameters.
            orientation: The orientation, either "portrait" or "landscape".
            page: The page parameters.
            page_ranges: The page ranges.
            scale: The scale.
            shrink_to_fit: Whether to shrink to fit.
    
        Returns:
        -------
            str: The Base64-encoded PDF document.
        """
        params = {
            "context": context,
            "background": background,
            "orientation": orientation,
            "scale": scale,
            "shrinkToFit": shrink_to_fit,
        }
        if margin is not None:
            params["margin"] = margin
        if page is not None:
            params["page"] = page
        if page_ranges is not None:
            params["pageRanges"] = page_ranges
    
        result = self.conn.execute(command_builder("browsingContext.print", params))
        return result["data"]
    
    def reload(
        self,
        context: str,
        ignore_cache: Optional[bool] = None,
        wait: Optional[str] = None,
    ) -> Dict:
        """Reloads a navigable.
    
        Parameters:
        -----------
            context: The browsing context ID.
            ignore_cache: Whether to ignore the cache.
            wait: The readiness state to wait for.
    
        Returns:
        -------
            Dict: A dictionary containing the navigation result.
        """
        params = {"context": context}
        if ignore_cache is not None:
            params["ignoreCache"] = ignore_cache
        if wait is not None:
            params["wait"] = wait
    
        result = self.conn.execute(command_builder("browsingContext.reload", params))
        return result
    
    def set_viewport(
        self,
        context: Optional[str] = None,
        viewport: Optional[Dict] = None,
        device_pixel_ratio: Optional[float] = None,
        user_contexts: Optional[List[str]] = None,
    ) -> None:
        """Modifies specific viewport characteristics on the given top-level traversable.
    
        Parameters:
        -----------
            context: The browsing context ID.
            viewport: The viewport parameters.
            device_pixel_ratio: The device pixel ratio.
            user_contexts: The user context IDs.
    
        Raises:
        ------
            Exception: If the browsing context is not a top-level traversable.
        """
        params = {}
        if context is not None:
            params["context"] = context
        if viewport is not None:
            params["viewport"] = viewport
        if device_pixel_ratio is not None:
            params["devicePixelRatio"] = device_pixel_ratio
        if user_contexts is not None:
            params["userContexts"] = user_contexts
    
        self.conn.execute(command_builder("browsingContext.setViewport", params))
    
    def traverse_history(self, context: str, delta: int) -> Dict:
        """Traverses the history of a given navigable by a delta.
    
        Parameters:
        -----------
            context: The browsing context ID.
            delta: The delta to traverse by.
    
        Returns:
        -------
            Dict: A dictionary containing the traverse history result.
        """
        params = {"context": context, "delta": delta}
        result = self.conn.execute(command_builder("browsingContext.traverseHistory", params))
        return result
    Event Handling

    The event handling implementation may have memory leak issues as it doesn't properly clean up all callbacks when removing event handlers, especially in error scenarios.

    def _on_event(self, event_name: str, callback: callable) -> int:
        """Set a callback function to subscribe to a browsing context event.
    
        Parameters:
        ----------
            event_name: The event to subscribe to.
            callback: The callback function to execute on event.
    
        Returns:
        -------
            int: callback id
        """
        event = BrowsingContextEvent(event_name)
    
        def _callback(event_data):
            if event_name == self.EVENTS["context_created"] or event_name == self.EVENTS["context_destroyed"]:
                info = BrowsingContextInfo.from_json(event_data.params)
                callback(info)
            elif event_name == self.EVENTS["download_will_begin"]:
                params = DownloadWillBeginParams.from_json(event_data.params)
                callback(params)
            elif event_name == self.EVENTS["user_prompt_opened"]:
                params = UserPromptOpenedParams.from_json(event_data.params)
                callback(params)
            elif event_name == self.EVENTS["user_prompt_closed"]:
                params = UserPromptClosedParams.from_json(event_data.params)
                callback(params)
            elif event_name == self.EVENTS["history_updated"]:
                params = HistoryUpdatedParams.from_json(event_data.params)
                callback(params)
            else:
                # For navigation events
                info = NavigationInfo.from_json(event_data.params)
                callback(info)
    
        callback_id = self.conn.add_callback(event, _callback)
    
        if event_name in self.callbacks:
            self.callbacks[event_name].append(callback_id)
        else:
            self.callbacks[event_name] = [callback_id]
    
        return callback_id
    
    def add_event_handler(self, event: str, callback: callable, contexts: Optional[List[str]] = None) -> int:
        """Add an event handler to the browsing context.
    
        Parameters:
        ----------
            event: The event to subscribe to.
            callback: The callback function to execute on event.
            contexts: The browsing context IDs to subscribe to.
    
        Returns:
        -------
            int: callback id
        """
        try:
            event_name = self.EVENTS[event]
        except KeyError:
            raise Exception(f"Event {event} not found")
    
        callback_id = self._on_event(event_name, callback)
    
        if event_name in self.subscriptions:
            self.subscriptions[event_name].append(callback_id)
        else:
            params = {"events": [event_name]}
            if contexts is not None:
                params["browsingContexts"] = contexts
            self.conn.execute(session_subscribe(**params))
            self.subscriptions[event_name] = [callback_id]
    
        return callback_id
    
    def remove_event_handler(self, event: str, callback_id: int) -> None:
        """Remove an event handler from the browsing context.
    
        Parameters:
        ----------
            event: The event to unsubscribe from.
            callback_id: The callback id to remove.
        """
        try:
            event_name = self.EVENTS[event]
        except KeyError:
            raise Exception(f"Event {event} not found")
    
        event = BrowsingContextEvent(event_name)
    
        self.conn.remove_callback(event, callback_id)
        self.subscriptions[event_name].remove(callback_id)
        if len(self.subscriptions[event_name]) == 0:
            params = {"events": [event_name]}
            self.conn.execute(session_unsubscribe(**params))
            del self.subscriptions[event_name]
    
    def clear_event_handlers(self) -> None:
        """Clear all event handlers from the browsing context."""
        for event_name in self.subscriptions:
            event = BrowsingContextEvent(event_name)
            for callback_id in self.subscriptions[event_name]:
                self.conn.remove_callback(event, callback_id)
            params = {"events": [event_name]}
            self.conn.execute(session_unsubscribe(**params))
        self.subscriptions = {}

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Reset callbacks dictionary

    The clear_event_handlers method doesn't reset the callbacks dictionary after
    clearing event handlers. This can lead to memory leaks and inconsistent state if
    new event handlers are added after clearing.

    py/selenium/webdriver/common/bidi/browsing_context.py [724-732]

     def clear_event_handlers(self) -> None:
         """Clear all event handlers from the browsing context."""
         for event_name in self.subscriptions:
             event = BrowsingContextEvent(event_name)
             for callback_id in self.subscriptions[event_name]:
                 self.conn.remove_callback(event, callback_id)
             params = {"events": [event_name]}
             self.conn.execute(session_unsubscribe(**params))
         self.subscriptions = {}
    +    self.callbacks = {}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This is a valid suggestion that fixes a potential memory leak. The clear_event_handlers method should reset both the subscriptions and callbacks dictionaries to maintain consistency and prevent memory leaks when handlers are cleared and new ones are added later.

    Medium
    Learned
    best practice
    Add validation for required JSON fields before object creation to prevent potential errors when processing incomplete data

    The from_json method doesn't validate that the required parameters exist in the
    JSON dictionary before accessing them. While get() safely returns None for
    missing keys, the class initialization expects valid values for required
    parameters like context, timestamp, and url. Add validation to ensure these
    required fields exist in the JSON before creating the object.

    py/selenium/webdriver/common/bidi/browsing_context.py [63-79]

     def from_json(cls, json: Dict) -> "NavigationInfo":
         """Creates a NavigationInfo instance from a dictionary.
     
         Parameters:
         -----------
             json: A dictionary containing the navigation information.
     
         Returns:
         -------
             NavigationInfo: A new instance of NavigationInfo.
    +    
    +    Raises:
    +    ------
    +        ValueError: If required fields are missing from the JSON.
         """
    +    if not json or "context" not in json or "timestamp" not in json or "url" not in json:
    +        raise ValueError("Required fields missing from NavigationInfo JSON")
    +        
         return cls(
             context=json.get("context"),
             navigation=json.get("navigation"),
             timestamp=json.get("timestamp"),
             url=json.get("url"),
         )
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • More

    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 4/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants