Skip to content

[py][bidi]: add timestamp to HistoryUpdatedParams class #15892

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

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Jun 12, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Adds the timestamp parameter to HistoryUpdatedParams class as per the updated bidi spec - https://w3c.github.io/webdriver-bidi/#event-browsingContext-historyUpdated

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

• Add timestamp parameter to HistoryUpdatedParams class
• Update constructor and JSON parsing method
• Add validation for timestamp parameter
• Align with updated WebDriver BiDi specification


Changes walkthrough 📝

Relevant files
Enhancement
browsing_context.py
Add timestamp parameter to HistoryUpdatedParams                   

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

• Added timestamp parameter to HistoryUpdatedParams constructor

Updated from_json method to parse and validate timestamp
• Added
validation ensuring timestamp is non-negative integer
• Updated class
to match WebDriver BiDi specification

+7/-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 Jun 12, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix ChromeDriver connection failure error that occurs after first instance
    • Resolve "Error: ConnectFailure (Connection refused)" for subsequent ChromeDriver instances
    • Ensure proper ChromeDriver instantiation on Ubuntu 16.04.4 with Chrome 65.0.3325.181

    1234 - Not compliant

    Non-compliant requirements:

    • Fix JavaScript execution in link href on click() method
    • Ensure JavaScript alerts are triggered when clicking links with JavaScript in href
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2
    • Fix compatibility with Firefox 42.0

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

    Missing Tests

    The addition of a new required parameter to the constructor and validation logic lacks corresponding unit tests to verify the timestamp validation behavior and proper instantiation.

    def __init__(
        self,
        context: str,
        timestamp: int,
        url: str,
    ):
        self.context = context
        self.timestamp = timestamp
        self.url = url
    
    @classmethod
    def from_json(cls, json: dict) -> "HistoryUpdatedParams":
        """Creates a HistoryUpdatedParams instance from a dictionary.
    
        Parameters:
        -----------
            json: A dictionary containing the history updated parameters.
    
        Returns:
        -------
            HistoryUpdatedParams: A new instance of HistoryUpdatedParams.
        """
        context = json.get("context")
        if context is None or not isinstance(context, str):
            raise ValueError("context is required and must be a string")
    
        timestamp = json.get("timestamp")
        if timestamp is None or not isinstance(timestamp, int) or timestamp < 0:
            raise ValueError("timestamp is required and must be a non-negative integer")
    
        url = json.get("url")
        if url is None or not isinstance(url, str):
            raise ValueError("url is required and must be a string")
    
        return cls(
            context=context,
            timestamp=timestamp,
            url=url,
        )

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 12, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add timestamp validation in constructor

    Add type validation for the timestamp parameter in the constructor to ensure
    consistency with the from_json method. This prevents invalid timestamp values
    from being passed directly to the constructor.

    py/selenium/webdriver/common/bidi/browsing_context.py [335-343]

     def __init__(
         self,
         context: str,
         timestamp: int,
         url: str,
     ):
    +    if not isinstance(timestamp, int) or timestamp < 0:
    +        raise ValueError("timestamp must be a non-negative integer")
         self.context = context
         self.timestamp = timestamp
         self.url = url
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that validation for the timestamp parameter is only performed in the from_json method. Adding the same validation to the __init__ constructor ensures that HistoryUpdatedParams objects are always created in a valid state, regardless of whether they are instantiated directly or through the factory method. This improves the robustness and consistency of the class.

    Medium
    • Update

    @navin772 navin772 requested review from cgoldberg and shbenzer June 12, 2025 13:34
    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 navin772 merged commit 1fe6d27 into SeleniumHQ:trunk Jun 12, 2025
    2 of 4 checks passed
    @navin772 navin772 deleted the add-timestamp-HistoryUpdatedParams branch June 12, 2025 18:15
    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 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants