Skip to content

[java][BiDi] implement browsingContext.historyUpdated #15901

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 7 commits into
base: trunk
Choose a base branch
from

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Jun 16, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Implements https://w3c.github.io/webdriver-bidi/#event-browsingContext-historyUpdated

🔧 Implementation Notes

💡 Additional Considerations

Tests ain't added because they don't work any browser at this stage.

🔄 Types of changes

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

PR Type

Enhancement


Description

• Implement browsingContext.historyUpdated BiDi event
• Add event listener support in BrowsingContextInspector
• Create HistoryUpdated class with JSON serialization


Changes walkthrough 📝

Relevant files
Enhancement
HistoryUpdated.java
Add HistoryUpdated BiDi event class                                           

java/src/org/openqa/selenium/bidi/browsingcontext/HistoryUpdated.java

• Create new HistoryUpdated class for BiDi event
• Implement JSON
deserialization with fromJson method
• Add getter methods for
browsingContextId, timestamp, and url
• Include private toJson method
for serialization

+93/-0   
BrowsingContextInspector.java
Add historyUpdated event support to inspector                       

java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java

• Add import for HistoryUpdated class
• Create historyUpdated event
with JSON mapper
• Implement onHistoryUpdated listener method
• Add
historyUpdated to close method cleanup

+20/-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-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 16, 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 "ConnectFailure (Connection refused)" error 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's href attribute when using click() method
    • Ensure JavaScript alerts are triggered properly in Firefox 42.0
    • Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

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

    Missing Validation

    The fromJson method doesn't validate required fields. All three fields (browsingContextId, timestamp, url) could be null/zero after parsing, which may cause issues when the object is used.

    public static HistoryUpdated fromJson(JsonInput input) {
      String browsingContextId = null;
      int timestamp = 0;
      String url = null;
    
      input.beginObject();
      while (input.hasNext()) {
        switch (input.nextName()) {
          case "context":
            browsingContextId = input.read(String.class);
            break;
    
          case "timestamp":
            timestamp = input.read(int.class);
            break;
    
          case "url":
            url = input.read(String.class);
            break;
    
          default:
            input.skipValue();
            break;
        }
      }
    
      input.endObject();
    
      return new HistoryUpdated(browsingContextId, timestamp, url);
    }
    Inconsistent Naming

    The fromJson method reads "context" from JSON but stores it as browsingContextId, while toJson outputs "browsingContextId". This inconsistency could cause serialization/deserialization issues.

          case "context":
            browsingContextId = input.read(String.class);
            break;
    
          case "timestamp":
            timestamp = input.read(int.class);
            break;
    
          case "url":
            url = input.read(String.class);
            break;
    
          default:
            input.skipValue();
            break;
        }
      }
    
      input.endObject();
    
      return new HistoryUpdated(browsingContextId, timestamp, url);
    }
    
    public String getBrowsingContextId() {
      return browsingContextId;
    }
    
    public int getTimestamp() {
      return timestamp;
    }
    
    public String getUrl() {
      return url;
    }
    
    private Map<String, Object> toJson() {
      Map<String, Object> toReturn = new TreeMap<>();
    
      toReturn.put("browsingContextId", this.getBrowsingContextId());

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 16, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inconsistent JSON field names

    The toJson method uses inconsistent field names compared to the fromJson method.
    The deserialization expects "context" but serialization uses
    "browsingContextId", which could cause issues.

    java/src/org/openqa/selenium/bidi/browsingcontext/HistoryUpdated.java [84-92]

     private Map<String, Object> toJson() {
       Map<String, Object> toReturn = new TreeMap<>();
     
    -  toReturn.put("browsingContextId", this.getBrowsingContextId());
    +  toReturn.put("context", this.getBrowsingContextId());
       toReturn.put("timestamp", this.getTimestamp());
       toReturn.put("url", this.getUrl());
     
       return unmodifiableMap(toReturn);
     }
    • Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion identifies a critical bug. The fromJson method expects the field "context", while the toJson method serializes it as "browsingContextId". This inconsistency would break any serialization-deserialization round trip. The proposed fix correctly aligns the field names.

    High
    Add null validation for required fields

    Add null validation for required fields before creating the HistoryUpdated
    instance. The constructor should validate that browsingContextId and url are not
    null since they are essential for the event.

    java/src/org/openqa/selenium/bidi/browsingcontext/HistoryUpdated.java [41-70]

     public static HistoryUpdated fromJson(JsonInput input) {
       String browsingContextId = null;
       int timestamp = 0;
       String url = null;
    -  ...
    +
    +  input.beginObject();
    +  while (input.hasNext()) {
    +    switch (input.nextName()) {
    +      case "context":
    +        browsingContextId = input.read(String.class);
    +        break;
    +
    +      case "timestamp":
    +        timestamp = input.read(int.class);
    +        break;
    +
    +      case "url":
    +        url = input.read(String.class);
    +        break;
    +
    +      default:
    +        input.skipValue();
    +        break;
    +    }
    +  }
    +
    +  input.endObject();
    +
    +  if (browsingContextId == null) {
    +    throw new IllegalArgumentException("browsingContextId cannot be null");
    +  }
    +  if (url == null) {
    +    throw new IllegalArgumentException("url cannot be null");
    +  }
    +
       return new HistoryUpdated(browsingContextId, timestamp, url);
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that browsingContextId and url could be null if they are missing from the JSON input. Adding null checks improves the robustness of the fromJson method by ensuring the created HistoryUpdated object is always in a valid state.

    Medium
    • Update

    @Delta456 Delta456 requested a review from pujagani June 18, 2025 07:24
    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Will tests come in a separate PR?

    @Delta456
    Copy link
    Member Author

    Will tests come in a separate PR?

    Yes because no browser supports this function so far.

    @nvborisenko
    Copy link
    Member

    Or leave it as draft like #15533?

    @Delta456
    Copy link
    Member Author

    Or leave it as draft like #15533?

    Events in Java binding lack tests. With time, I am planning to make a PR with all those tests

    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-java Java Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants