Skip to content

Conversation

@asolntsev
Copy link
Contributor

@asolntsev asolntsev commented Oct 28, 2025

User description

💥 What does this PR do?

This PR the existing method BrowsingContext.setViewport(width, height) so that it allows 'null' arguments now.

Calling BrowsingContext.setViewport(null, null) reverts the viewport size to the initial size (in other words, disables "mobile emulation" mode and enables "desktop" mode).

🔧 Implementation Notes

According to BiDi spec, we have to call method "setViewport" with "viewport" parameter equal to "null".
That's why I had to replace new APIs Map.of and Map.copyOf (which does not allow null values) by good old APIs (new HashMap()) that allow null values.

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests


Description

  • Mark all 3 arguments of BiDi method BrowsingContext.setViewport() as nullable (null means reverting viewport to initial size)

  • Replace Map.copyOf() with HashMap to support null values in Command parameters

  • Add toString() method to BrowsingContextInfo for better debugging

  • Refactor viewport tests with helper methods and improved assertions

  • Update test assertions to use modern AssertJ API patterns


Diagram Walkthrough

flowchart LR
  A["BrowsingContext.setViewport()"] -->|"with viewport=null"| B["BrowsingContext.setViewport(null, null)"]
  C["Command class"] -->|"support null values"| D["HashMap instead of Map.copyOf()"]
  E["Test refactoring"] -->|"helper methods"| F["getViewportSize() & getDevicePixelRatio()"]
Loading

File Walkthrough

Relevant files
Enhancement
Command.java
Support null values in Command parameters                               

java/src/org/openqa/selenium/bidi/Command.java

  • Replace Map.copyOf() with unmodifiableMap(new HashMap<>()) to allow
    null parameter values
  • Add import for Collections.unmodifiableMap and HashMap
  • Maintains immutability while supporting null values required by BiDi
    spec
+5/-1     
BrowsingContext.java
Make setViewport(width, height) BiDi method arguments nullable                                                   

java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContext.java

  • Make arguments of resetViewport() method nullable
  • Uses HashMap to construct parameters allowing null values per BiDi
    specification
  • Reverts viewport to initial "desktop mode" size
+8/-0     
BrowsingContextInfo.java
Add toString() method for debugging                                           

java/src/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInfo.java

  • Add toString() override method for better object representation
  • Returns formatted string with browsing context id and url
+5/-0     
Tests
BrowsingContextTest.java
Refactor viewport tests and modernize assertions                 

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java

  • Add helper methods getViewportSize() and getDevicePixelRatio() to
    reduce code duplication
  • Add test for setViewport(null, null) functionality verifying viewport returns
    to initial size
  • Update assertions to use modern AssertJ API (Assertions instead of
    AssertionsForClassTypes)
  • Replace assertThatExceptionOfType() with assertThatThrownBy() and add
    message pattern matching
  • Simplify assertions using hasSize(), isNotEmpty(), and isEqualTo(new
    Dimension()) patterns
  • Add CONTEXT_NOT_FOUND pattern constant for exception message
    validation
+44/-35 

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Oct 28, 2025
@asolntsev asolntsev changed the title add BiDi method `BrowsingContext.resetViewport" add BiDi method BrowsingContext.resetViewport Oct 28, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 28, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
No audit logs: New actions like resetViewport() and command dispatches add critical behavior without any
explicit logging of action, user, or outcome in the changed code.

Referred Code
public void resetViewport() {
  Map<String, Object> params = new HashMap<>();
  params.put(CONTEXT, id);
  params.put("viewport", null);
  params.put("devicePixelRatio", null);
  this.bidi.send(new Command<>("browsingContext.setViewport", params));
}
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null handling: The constructor now wraps params in an unmodifiable HashMap allowing null values, but
added code does not show explicit validation or handling for null keys/values beyond
construction.

Referred Code
this.params =
    unmodifiableMap(new HashMap<String, Object>(Require.nonNull("Command parameters", params)));
this.mapper = Require.nonNull("Mapper for result", mapper);
Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
External input params: resetViewport() and viewport setters send parameters (including nulls) directly to BiDi
without visible sanitization or authorization checks in the diff, which may rely on
external validation.

Referred Code
public void resetViewport() {
  Map<String, Object> params = new HashMap<>();
  params.put(CONTEXT, id);
  params.put("viewport", null);
  params.put("devicePixelRatio", null);
  this.bidi.send(new Command<>("browsingContext.setViewport", params));
}
  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 28, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Guard against blank method names

Add a null-check for the method string to ensure it's not blank or empty,
preventing invalid BiDi command creation.

java/src/org/openqa/selenium/bidi/Command.java [54-55]

-this.params =
-    unmodifiableMap(new HashMap<String, Object>(Require.nonNull("Command parameters", params)));
+this.method = Require.nonNull("Method name", method);
+Require.nonNull("Command parameters", params);
+if (method.isBlank()) {
+  throw new IllegalArgumentException("Method name must not be blank");
+}
+this.params = unmodifiableMap(new HashMap<>(params));
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and states early with precise checks to prevent NPEs and logic errors.

Low
Replace wildcard AWT import

Replace the wildcard AWT import with the specific Dimension import to keep test
dependencies explicit and avoid unintended class loads.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java [26]

-import java.awt.*;
+import java.awt.Dimension;
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Enforce deterministic and safe resource handling in tests by avoiding wildcard AWT imports which can mask missing headless constraints and bloat classpath.

Low
General
Improve type safety in test helper

Change the return type of the getDevicePixelRatio helper method from Number to
double and call .doubleValue() on the result to handle different numeric types
returned by executeScript more robustly.

java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextTest.java [580-582]

-private Number getDevicePixelRatio() {
-  return (Number) ((JavascriptExecutor) driver).executeScript("return window.devicePixelRatio");
+private double getDevicePixelRatio() {
+  return ((Number) ((JavascriptExecutor) driver).executeScript("return window.devicePixelRatio"))
+      .doubleValue();
 }
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that executeScript can return different numeric types and proposes a more robust implementation for the getDevicePixelRatio helper method by standardizing the return type to double, which improves type safety and code quality.

Low
  • Update

Implementation detail: according to BiDi spec (https://www.w3.org/TR/webdriver-bidi/#command-browsingContext-setViewport), we have to call method "setViewport" with "viewport" parameter equal to "null".
@asolntsev asolntsev force-pushed the feature/bidi-reset-viewport branch from 1263bc6 to 328267e Compare October 28, 2025 21:58
@nvborisenko
Copy link
Member

There is no resetViewport in the spec (kind of low-level).

@asolntsev
Copy link
Contributor Author

I know :)

resetViewport calls BiDi's method setViewport(viewport: null, browserPixelRation: null). Which looks quite strange for me.

If we want Java api to exactly reflect BiDi API, then we should add method setViewport with "null" parameters? This would not look good at all.

Look, currently we have two methods:

public void setViewport(double width, double height) { ... }
public void setViewport(double width, double height, double devicePixelRatio) { ... }

Or should we add method setViewport() without parameters?
For me, resetViewport looks much better. And it exactly reflects the intention.

@nvborisenko
Copy link
Member

If setViewport(null) works, then it is good.

Believe me, in .NET I introduced so many syntax sugar (like aliases), and now I am removing it. Low-level API should represent available API withoout addition magic (Selenium dreams it might auto-generated). But it is as is, I propose to not introduce "aliases".

@asolntsev
Copy link
Contributor Author

@nvborisenko Point taken.
I replaced method resetViewport by setViewport(null, null, null).

@navin772 navin772 changed the title add BiDi method BrowsingContext.resetViewport [java][bidi]: add BiDi method BrowsingContext.resetViewport Oct 29, 2025
@asolntsev asolntsev force-pushed the feature/bidi-reset-viewport branch from 432f797 to ae3e084 Compare October 29, 2025 12:09
@asolntsev asolntsev changed the title [java][bidi]: add BiDi method BrowsingContext.resetViewport [java][bidi]: add BiDi method BrowsingContext.setViewport(null, null) to reset the mobile emulation mode Oct 29, 2025
@asolntsev asolntsev self-assigned this Oct 29, 2025
@asolntsev asolntsev requested a review from nvborisenko October 29, 2025 12:16
@cgoldberg
Copy link
Member

I replaced method resetViewport by setViewport(null, null, null)

Please update the PR description to reflect that.
(Also, the AI bot added a bunch of stuff to the description that still mentions resetViewport)

@asolntsev
Copy link
Contributor Author

Please update the PR description to reflect that.
(Also, the AI bot added a bunch of stuff to the description that still mentions resetViewport)

done.

@asolntsev asolntsev force-pushed the feature/bidi-reset-viewport branch from 920dd54 to 97af1c4 Compare October 29, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants