| General |
✅ Remove inconsistent and unnecessary page reload
Suggestion Impact:The commit removed the page reload (context.navigate) after calling setScreenOrientationOverride in the test, matching the suggestion to eliminate the unnecessary reload.
code diff:
@@ -64,9 +67,6 @@
emulation.setScreenOrientationOverride(
new SetScreenOrientationOverrideParameters(landscapeOrientation)
.contexts(List.of(contextId)));
-
- // Reload the page to apply the orientation change
- context.navigate(url, ReadinessState.COMPLETE);
Map<String, Object> currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
@@ -124,9 +124,6 @@
new SetScreenOrientationOverrideParameters(landscapeOrientation)
.userContexts(List.of(userContext)));
- // Reload the page to apply the orientation override
- context.navigate(url, ReadinessState.COMPLETE);
-
Map<String, Object> currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
assertThat(currentOrientation.get("angle")).isEqualTo(0);
Remove the inconsistent and unnecessary page reload after the first
setScreenOrientationOverride call to make the test accurately reflect the
expected dynamic behavior.
java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [68-93]
-// Reload the page to apply the orientation change
-context.navigate(url, ReadinessState.COMPLETE);
-
Map<String, Object> currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
assertThat(currentOrientation.get("angle")).isEqualTo(0);
// Set portrait-secondary orientation
ScreenOrientation portraitOrientation =
new ScreenOrientation(
ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY);
emulation.setScreenOrientationOverride(
new SetScreenOrientationOverrideParameters(portraitOrientation)
.contexts(List.of(contextId)));
currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary");
assertThat(currentOrientation.get("angle")).isEqualTo(180);
// Clear the override
emulation.setScreenOrientationOverride(
new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId)));
currentOrientation = getScreenOrientation(contextId);
assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type"));
assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle"));
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies an inconsistent and likely unnecessary page reload in the test, improving test correctness and consistency.
| Medium
|
✅ Use idiomatic null checks for consistency
Suggestion Impact:The commit removed manual null checks and assigned fields using Require.nonNull(), aligning with the suggestion.
code diff:
+import org.openqa.selenium.internal.Require;
public class ScreenOrientation {
private final ScreenOrientationNatural natural;
private final ScreenOrientationType type;
public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) {
- if (natural == null) {
- throw new IllegalArgumentException("Natural orientation cannot be null");
- }
- if (type == null) {
- throw new IllegalArgumentException("Orientation type cannot be null");
- }
- this.natural = natural;
- this.type = type;
+ this.natural = Require.nonNull("natural", natural);
+ this.type = Require.nonNull("type", type);
In the ScreenOrientation constructor, replace manual null checks with the
idiomatic Require.nonNull() for consistency with the rest of the codebase.
java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [27-36]
-public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) {
- if (natural == null) {
- throw new IllegalArgumentException("Natural orientation cannot be null");
+import org.openqa.selenium.internal.Require;
+
+public class ScreenOrientation {
+ // ...
+ public ScreenOrientation(ScreenOrientationNatural natural, ScreenOrientationType type) {
+ this.natural = Require.nonNull("Natural orientation", natural);
+ this.type = Require.nonNull("Orientation type", type);
}
- if (type == null) {
- throw new IllegalArgumentException("Orientation type cannot be null");
- }
- this.natural = natural;
- this.type = type;
+ // ...
}
Suggestion importance[1-10]: 4
__
Why: The suggestion improves code consistency by proposing the use of the idiomatic Require.nonNull() helper, aligning the new class with existing codebase conventions.
| Low
|
| Possible issue |
✅ Fetch screen orientation properties atomically
Suggestion Impact:The commit replaced two separate executeScript calls with a single atomic call returning both type and angle, and adjusted the return map accordingly.
code diff:
- String type = (String) executor.executeScript("return screen.orientation.type;");
- Number angle = (Number) executor.executeScript("return screen.orientation.angle;");
+ Map<String, Object> orientation =
+ (Map<String, Object>)
+ executor.executeScript(
+ "return { type: screen.orientation.type, angle: screen.orientation.angle };");
- return Map.of("type", type, "angle", angle.intValue());
+ return Map.of(
+ "type", orientation.get("type"), "angle", ((Number) orientation.get("angle")).intValue());
Refactor the getScreenOrientation method to fetch screen orientation type and
angle in a single, atomic JavaScript execution to prevent potential race
conditions.
java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [36-44]
private Map<String, Object> getScreenOrientation(String context) {
driver.switchTo().window(context);
JavascriptExecutor executor = (JavascriptExecutor) driver;
- String type = (String) executor.executeScript("return screen.orientation.type;");
- Number angle = (Number) executor.executeScript("return screen.orientation.angle;");
+ Map<String, Object> orientation =
+ (Map<String, Object>)
+ executor.executeScript(
+ "return { type: screen.orientation.type, angle: screen.orientation.angle };");
- return Map.of("type", type, "angle", angle.intValue());
+ return Map.of("type", orientation.get("type"), "angle", ((Number) orientation.get("angle")).intValue());
}
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies a potential race condition in a test helper method and proposes a more robust, atomic operation which also slightly improves performance.
| Low
|
Learned
best practice |
Ensure context cleanup with finally
Ensure the created BrowsingContext is closed even if navigation or assertions
fail by wrapping its lifecycle in try/finally.
java/test/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideTest.java [49-93]
BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle());
-String contextId = context.getId();
+try {
+ String contextId = context.getId();
-// Navigate to a page first to ensure screen.orientation is available
-String url = appServer.whereIs("formPage.html");
-context.navigate(url, ReadinessState.COMPLETE);
+ // Navigate to a page first to ensure screen.orientation is available
+ String url = appServer.whereIs("formPage.html");
+ context.navigate(url, ReadinessState.COMPLETE);
-Map<String, Object> initialOrientation = getScreenOrientation(contextId);
+ Map<String, Object> initialOrientation = getScreenOrientation(contextId);
-Emulation emulation = new Emulation(driver);
+ Emulation emulation = new Emulation(driver);
-// Set landscape-primary orientation
-ScreenOrientation landscapeOrientation =
- new ScreenOrientation(
- ScreenOrientationNatural.LANDSCAPE, ScreenOrientationType.LANDSCAPE_PRIMARY);
-emulation.setScreenOrientationOverride(
- new SetScreenOrientationOverrideParameters(landscapeOrientation)
- .contexts(List.of(contextId)));
+ // Set landscape-primary orientation
+ ScreenOrientation landscapeOrientation =
+ new ScreenOrientation(
+ ScreenOrientationNatural.LANDSCAPE, ScreenOrientationType.LANDSCAPE_PRIMARY);
+ emulation.setScreenOrientationOverride(
+ new SetScreenOrientationOverrideParameters(landscapeOrientation)
+ .contexts(List.of(contextId)));
-// Reload the page to apply the orientation change
-context.navigate(url, ReadinessState.COMPLETE);
+ // Reload the page to apply the orientation change
+ context.navigate(url, ReadinessState.COMPLETE);
-Map<String, Object> currentOrientation = getScreenOrientation(contextId);
-assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
-assertThat(currentOrientation.get("angle")).isEqualTo(0);
+ Map<String, Object> currentOrientation = getScreenOrientation(contextId);
+ assertThat(currentOrientation.get("type")).isEqualTo("landscape-primary");
+ assertThat(currentOrientation.get("angle")).isEqualTo(0);
-// Set portrait-secondary orientation
-ScreenOrientation portraitOrientation =
- new ScreenOrientation(
- ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY);
-emulation.setScreenOrientationOverride(
- new SetScreenOrientationOverrideParameters(portraitOrientation)
- .contexts(List.of(contextId)));
+ // Set portrait-secondary orientation
+ ScreenOrientation portraitOrientation =
+ new ScreenOrientation(
+ ScreenOrientationNatural.PORTRAIT, ScreenOrientationType.PORTRAIT_SECONDARY);
+ emulation.setScreenOrientationOverride(
+ new SetScreenOrientationOverrideParameters(portraitOrientation)
+ .contexts(List.of(contextId)));
-currentOrientation = getScreenOrientation(contextId);
-assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary");
-assertThat(currentOrientation.get("angle")).isEqualTo(180);
+ currentOrientation = getScreenOrientation(contextId);
+ assertThat(currentOrientation.get("type")).isEqualTo("portrait-secondary");
+ assertThat(currentOrientation.get("angle")).isEqualTo(180);
-// Clear the override
-emulation.setScreenOrientationOverride(
- new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId)));
+ // Clear the override
+ emulation.setScreenOrientationOverride(
+ new SetScreenOrientationOverrideParameters(null).contexts(List.of(contextId)));
-currentOrientation = getScreenOrientation(contextId);
-assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type"));
-assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle"));
+ currentOrientation = getScreenOrientation(contextId);
+ assertThat(currentOrientation.get("type")).isEqualTo(initialOrientation.get("type"));
+ assertThat(currentOrientation.get("angle")).isEqualTo(initialOrientation.get("angle"));
+} finally {
+ context.close();
+}
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Always wrap creation of external contexts/resources in try/finally and ensure explicit cleanup to prevent leaks on failure.
| Low
|
| Possible issue |
Omit null field from payload
Modify the SetScreenOrientationOverrideParameters constructor to omit the
screenOrientation key from the payload when it is null, instead of explicitly
setting it to null.
java/src/org/openqa/selenium/bidi/emulation/SetScreenOrientationOverrideParameters.java [22-28]
public SetScreenOrientationOverrideParameters(ScreenOrientation screenOrientation) {
- if (screenOrientation == null) {
- map.put("screenOrientation", null);
- } else {
+ if (screenOrientation != null) {
map.put("screenOrientation", screenOrientation.toMap());
}
+ // If null, omit the key to clear the override
}
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that omitting a key is often better than sending an explicit null in JSON-RPC protocols, improving robustness and adherence to the BiDi specification.
| Medium
|
Serialize using enum values
In ScreenOrientation.toMap(), use a dedicated getValue() method for serializing
the natural and type enums instead of relying on toString().
java/src/org/openqa/selenium/bidi/emulation/ScreenOrientation.java [46-51]
public Map<String, Object> toMap() {
Map<String, Object> map = new HashMap<>();
- map.put("natural", natural.toString());
- map.put("type", type.toString());
+ map.put("natural", natural.getValue());
+ map.put("type", type.getValue());
return map;
}
Suggestion importance[1-10]: 5
__
Why: The suggestion improves code robustness by decoupling serialization logic from the toString() method, which is a good practice even though the current implementation works correctly.
| Low
|
| General |
Add explicit enum value getter
In the ScreenOrientationNatural enum, add an explicit getValue() accessor for
the protocol value and change toString() to return the enum's name for better
debugging.
java/src/org/openqa/selenium/bidi/emulation/ScreenOrientationNatural.java [20-34]
public enum ScreenOrientationNatural {
PORTRAIT("portrait"),
LANDSCAPE("landscape");
private final String value;
ScreenOrientationNatural(String value) {
this.value = value;
}
+ public String getValue() {
+ return value;
+ }
+
@Override
public String toString() {
- return value;
+ return name();
}
}
Suggestion importance[1-10]: 5
__
Why: This is a good code quality suggestion that improves maintainability by separating the enum's serialization value from its string representation, preventing potential future bugs.
| Low
|
|
| |
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
