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
Error Handling The implementation of addDomMutationHandler in RemoteScript.java lacks proper error handling for potential exceptions that might occur during the execution of the script or while fetching elements. Consider adding more robust error handling and logging mechanisms to ensure the stability and maintainability of the code.
Synchronization Concern The use of synchronized block in addDomMutationHandler method might lead to performance issues if the block is accessed by multiple threads simultaneously. Evaluate if this synchronization is necessary or if there could be a more efficient way to handle concurrency.
Handle potential NoSuchElementException for better error handling
Consider handling the potential NoSuchElementException when retrieving elements by CSS selector. This exception could be thrown if no elements match the given selector, which would cause the current implementation to fail. To handle this, you could check if the list elements is not only non-empty but also non-null.
elements = this.driver.findElements(By.cssSelector(String.format("*[data-__webdriver_id='%s']", id)));
+if (elements != null && !elements.isEmpty()) {+ DomMutation event = new DomMutation(elements.get(0), String.valueOf(values.get("name")), String.valueOf(values.get("value")), String.valueOf(values.get("oldValue")));+ consumer.accept(event);+}
Apply this suggestion
Suggestion importance[1-10]: 8
Why: This suggestion improves error handling by ensuring that the code does not fail if no elements match the given CSS selector. It addresses a potential bug and enhances the robustness of the code.
8
Enhancement
Improve error handling for file reading operations
Consider adding error handling for the IOException that could occur when reading the script file. Instead of throwing a generic IllegalStateException, it might be beneficial to log the error or retry reading the file.
try (InputStream stream = RemoteScript.class.getResourceAsStream("/org/openqa/selenium/remote/bidi-mutation-listener.js")) {
if (stream == null) {
throw new IllegalStateException("Unable to find helper script");
}
scriptValue = new String(stream.readAllBytes(), UTF_8);
} catch (IOException e) {
- throw new IllegalStateException("Unable to read helper script");+ // Log the error or handle it accordingly+ logger.error("Error reading helper script", e);+ throw new RuntimeException("Error processing the helper script", e);
}
Apply this suggestion
Suggestion importance[1-10]: 7
Why: This suggestion enhances error handling by logging the IOException and providing more specific error information. It improves maintainability and debugging but is not critical for functionality.
7
Performance
Improve thread handling by removing unnecessary synchronization
Replace the use of synchronized block with a more concurrent-friendly approach such as using ConcurrentHashMap or similar thread-safe collections. This change would improve the performance by reducing the blocking time of threads, especially in a multi-threaded environment.
-synchronized (this) {- elements = this.driver.findElements(By.cssSelector(String.format("*[data-__webdriver_id='%s']", id)));-}+elements = this.driver.findElements(By.cssSelector(String.format("*[data-__webdriver_id='%s']", id)));
Apply this suggestion
Suggestion importance[1-10]: 5
Why: While removing the synchronized block could improve performance, it might introduce concurrency issues. The suggestion is contextually correct but should be carefully evaluated for thread safety.
5
Best practice
Ensure resources are closed properly to avoid leaks
To avoid potential resource leaks, ensure that the InputStream is closed properly. Although the try-with-resources statement is used, explicitly checking and closing the stream in a finally block would be more robust, especially if modifications are made to the try block in the future.
-try (InputStream stream = RemoteScript.class.getResourceAsStream("/org/openqa/selenium/remote/bidi-mutation-listener.js")) {+InputStream stream = null;+try {+ stream = RemoteScript.class.getResourceAsStream("/org/openqa/selenium/remote/bidi-mutation-listener.js");
if (stream == null) {
throw new IllegalStateException("Unable to find helper script");
}
scriptValue = new String(stream.readAllBytes(), UTF_8);
} catch (IOException e) {
throw new IllegalStateException("Unable to read helper script");
+} finally {+ if (stream != null) {+ stream.close();+ }
}
Apply this suggestion
Suggestion importance[1-10]: 3
Why: The current try-with-resources statement already ensures that the InputStream is closed properly. The suggestion is redundant and does not provide significant improvement.
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
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
Related to #13992.
Motivation and Context
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
DomMutation
class to encapsulate DOM mutation details.RemoteScript
.Script
interface to include DOM mutation handler methods.Changes walkthrough 📝
Script.java
Update `onMessage` method to return listener ID
java/src/org/openqa/selenium/bidi/module/Script.java
onMessage
method to return a long value.DomMutation.java
Add `DomMutation` class for DOM mutation events
java/src/org/openqa/selenium/remote/DomMutation.java
DomMutation
to represent DOM mutation events.old value.
RemoteScript.java
Implement DOM mutation handler in `RemoteScript`
java/src/org/openqa/selenium/remote/RemoteScript.java
addDomMutationHandler
method to handle DOM mutation events.removeDomMutationHandler
method to remove DOM mutation handlers.Script.java
Extend `Script` interface with DOM mutation handler methods
java/src/org/openqa/selenium/remote/Script.java
WebScriptTest.java
Add tests for DOM mutation handlers
java/test/org/openqa/selenium/WebScriptTest.java
BUILD.bazel
Update Bazel build for DOM mutation support
java/src/org/openqa/selenium/remote/BUILD.bazel
bidi-mutation-listener
script to resources.bidi-mutation-listener
.BUILD.bazel
Export `bidi-mutation-listener.js` in Bazel build
javascript/bidi-support/BUILD.bazel
bidi-mutation-listener.js
to exports.