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

[pujagani]

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

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement

---

Description

• Modified addListener methods in BiDi, Connection, and LogInspector classes to return a long ID, enabling better listener management.
• Added removeListener method in BiDi and Connection classes to allow removal of listeners by their ID.
• Updated internal data structures in Connection to store listeners with their corresponding IDs.

---

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

[qodo-code-review]

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.

[qodo-code-review]

PR Code Suggestions ✨

Category	Suggestion	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: + 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] -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] -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] -eventCallbacks.computeIfAbsent( - event, - key -&gt; { - HashMap<long, consumer<?="">&gt; map = new HashMap&lt;&gt;(); - map.put(id, handler); - return map; - }); +eventCallbacks.compute(event, (key, existingMap) -&gt; { + if (existingMap == null) { + existingMap = new HashMap&lt;&gt;(); + } + 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] 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