search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.77k stars 8.02k forks source link

[bidi] [java] Ensure the listeners returns an id #14215

Closed pujagani closed 2 days ago

pujagani commented 2 days ago

User description

This is the base for implementing the high-level BiDi API

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

Checklist

PR Type

Enhancement

Description

Changes walkthrough 📝

Relevant files
Enhancement
BiDi.java
Modify listener methods to return IDs and add removal method
java/src/org/openqa/selenium/bidi/BiDi.java
  • Changed addListener methods to return a long ID.
  • Added removeListener method to remove listeners by ID.
    		•  +8/-4     
    Connection.java
    Enhance listener management with ID-based tracking             
    java/src/org/openqa/selenium/bidi/Connection.java
  • Changed addListener method to return a long ID.
  • Modified internal structure to store listeners with IDs.
  • Added removeListener method to remove listeners by ID.
    		•  +24/-5   
    LogInspector.java
    Update LogInspector to use ID-based listener management   
    java/src/org/openqa/selenium/bidi/module/LogInspector.java
  • Changed listener methods to return a long ID.
  • Updated internal methods to handle ID-based listener management.
    		•  +7/-7     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-pro[bot] commented 2 days ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 3
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Return Type Change:
    The method addListener in BiDi.java and Connection.java has been changed to return a long instead of void. This change affects the method signature and any existing code that uses these methods. Ensure that this change is documented and that all usages of these methods in the codebase are updated to handle the new return type.
    Data Structure Change:
    In Connection.java, the storage structure for event callbacks has been changed from a List to a Map. This change could affect how callbacks are managed and invoked. Review the logic to ensure that callbacks are correctly managed and that there are no memory leaks or errors in callback invocation.
    codiumai-pr-agent-pro[bot] commented 2 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    ✅ Enhance the removeListener method to clean up empty maps to prevent memory leaks ___
    Suggestion Impact:The commit implemented the suggestion to clean up empty maps from the eventCallbacks map after removing an id to prevent memory leaks. code diff: ```diff + eventCallbacks.forEach( + (k, v) -> { + v.remove(id); + if (v.isEmpty()) { + eventCallbacks.remove(k); + } + }); ```
    ___ **When removing a listener, ensure that empty maps are cleaned up from the eventCallbacks
    map to prevent memory leaks. This can be done by checking if the map is empty after
    removing an id and, if so, removing the map from eventCallbacks.** [java/src/org/openqa/selenium/bidi/Connection.java [219]](https://github.com/SeleniumHQ/selenium/pull/14215/files#diff-fa4b58e0bb2f950d2559090a4e1f36eb7126c32eeee7d611844d93c0b785670bR219-R219) ```diff -eventCallbacks.forEach((k, v) -> v.remove(id)); +eventCallbacks.forEach((k, v) -> { + v.remove(id); + if (v.isEmpty()) { + eventCallbacks.remove(k); + } +}); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: Cleaning up empty maps after removing a listener is essential to prevent memory leaks. This suggestion correctly identifies and addresses a potential issue in the new code, making it highly valuable.
    		10
    Add exception handling around the event subscription command to ensure robustness ___ **Ensure that the addListener method in the BiDi class properly handles exceptions that
    might occur during the subscription process. This can be achieved by adding a try-catch
    block around the send method call, which sends the subscription command to the server.
    This will prevent the method from failing silently if an error occurs during the command
    transmission.** [java/src/org/openqa/selenium/bidi/BiDi.java [58-60]](https://github.com/SeleniumHQ/selenium/pull/14215/files#diff-f0ed6d655b8c0ff47563631639d01d8f7a5f8e83a40db48456a2e9137e8ab0e6R58-R60) ```diff -send( - new Command<>( - "session.subscribe", Map.of("events", Collections.singletonList(event.getMethod())))); +try { + send( + new Command<>( + "session.subscribe", Map.of("events", Collections.singletonList(event.getMethod())))); +} catch (Exception e) { + // Handle exception appropriately + throw new RuntimeException("Failed to subscribe to event", e); +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding exception handling around the event subscription command is a good practice to prevent silent failures and improve robustness. This suggestion correctly addresses a potential issue in the new code.
    		8
    Possible bug
    Improve the event handler registration logic to support multiple handlers for the same event ___ **Modify the addListener method to ensure that the event handler is added to an existing map
    if the event is already registered. This avoids overwriting existing handlers for the same
    event. Use Map.computeIfPresent to add the handler to the existing map instead of creating
    a new one every time.** [java/src/org/openqa/selenium/bidi/Connection.java [192-198]](https://github.com/SeleniumHQ/selenium/pull/14215/files#diff-fa4b58e0bb2f950d2559090a4e1f36eb7126c32eeee7d611844d93c0b785670bR192-R198) ```diff -eventCallbacks.computeIfAbsent( - event, - key -> { - HashMap> map = new HashMap<>(); - map.put(id, handler); - return map; - }); +eventCallbacks.compute(event, (key, existingMap) -> { + if (existingMap == null) { + existingMap = new HashMap<>(); + } + existingMap.put(id, handler); + return existingMap; +}); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: This suggestion ensures that multiple handlers can be registered for the same event without overwriting existing handlers, which is crucial for the correct functioning of the event system. The improved code is accurate and enhances the existing functionality.
    		9
    Possible issue
    Ensure that addLogEntryAddedListener method handles the case where no browsing context IDs are provided ___ **Refactor the addLogEntryAddedListener method to throw an exception when no browsing
    context IDs are provided, instead of silently failing or doing nothing. This ensures that
    the method's caller is aware that the operation requires valid context IDs.** [java/src/org/openqa/selenium/bidi/module/LogInspector.java [167-170]](https://github.com/SeleniumHQ/selenium/pull/14215/files#diff-a268bb41209662ce7f375cf43e34b721a2264a9e6b34ba729bab38a516a67d8eR167-R170) ```diff if (browsingContextIds.isEmpty()) { - return this.bidi.addListener(Log.entryAdded(), consumer); -} else { - return this.bidi.addListener(browsingContextIds, Log.entryAdded(), consumer); + throw new IllegalArgumentException("Browsing context IDs cannot be empty."); } +return this.bidi.addListener(browsingContextIds, Log.entryAdded(), consumer); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Throwing an exception when no browsing context IDs are provided ensures that the caller is aware of the requirement, improving the method's robustness. However, this change might affect existing code that relies on the current behavior, so it should be carefully considered.
    		7