You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
✅ Enhance the removeListener method to clean up empty maps to prevent memory leaksSuggestion Impact:The commit implemented the suggestion to clean up empty maps from the eventCallbacks map after removing an id to prevent memory leaks.
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.
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.
-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.
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.
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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
addListener
methods inBiDi
,Connection
, andLogInspector
classes to return a long ID, enabling better listener management.removeListener
method inBiDi
andConnection
classes to allow removal of listeners by their ID.Connection
to store listeners with their corresponding IDs.Changes walkthrough 📝
BiDi.java
Modify listener methods to return IDs and add removal method
java/src/org/openqa/selenium/bidi/BiDi.java
addListener
methods to return a long ID.removeListener
method to remove listeners by ID.Connection.java
Enhance listener management with ID-based tracking
java/src/org/openqa/selenium/bidi/Connection.java
addListener
method to return a long ID.removeListener
method to remove listeners by ID.LogInspector.java
Update LogInspector to use ID-based listener management
java/src/org/openqa/selenium/bidi/module/LogInspector.java