[java][BiDi] emulation: allow passing null to GeolocationOverride

[Delta456]

User description

🔗 Related Issues

💥 What does this PR do?

Allows passing null to setGeolocationOverride's coord parameter as per the W3C spec - https://w3c.github.io/webdriver-bidi/#command-emulation-setGeolocationOverride

Uses assertThat instead of assert

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Bug fix (backwards compatible)

---

PR Type

Bug fix, Tests

---

Description

• Allow passing null to GeolocationCoordinates parameter per W3C spec

• Replace legacy assert statements with assertThat for better test clarity

• Add test case for resetting geolocation override with null coordinates

---

Diagram Walkthrough

flowchart LR
  A["SetGeolocationOverrideParameters"] -->|"accepts null coordinates"| B["map.put coordinates null"]
  A -->|"accepts coordinates object"| C["map.put coordinates.toMap"]
  D["Test Suite"] -->|"replaces assert with"| E["assertThat fluent API"]
  D -->|"adds new test"| F["canResetGeolocationOverrideWithNullCoordinates"]

Loading

File Walkthrough

	Relevant files
Bug fix	SetGeolocationOverrideParameters.java Allow null coordinates in geolocation override                      java/src/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideParameters.java Modified constructor to accept null GeolocationCoordinates parameter Changed from throwing IllegalArgumentException to storing null in map Refactored logic to use if-else for null vs non-null coordinate handling Aligns implementation with W3C WebDriver BiDi specification +3/-2
Tests	SetGeolocationOverrideTest.java Modernize assertions and add null coordinates test              java/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java Added import for assertThat from AssertJ library Replaced all legacy assert statements with assertThat fluent assertions Updated assertions in canSetGeolocationOverrideWithCoordinatesInContext test Updated assertions in canSetGeolocationOverrideWithMultipleUserContexts test Updated assertions in canSetGeolocationOverrideWithError test Added new test canResetGeolocationOverrideWithNullCoordinates to verify null coordinate handling +54/-16

---

[qodo-code-review]

PR Compliance Guide 🔍

(Compliance updated until commit cff7bdc)

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
🟡	🎫 #5678 🔴 Investigate and address repeated "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04 with Selenium 3.9.0 and Chrome/ChromeDriver versions specified. Provide a fix or guidance to prevent connection failures after the first ChromeDriver instance. Validate behavior across multiple instantiations and ensure stability.
🟡	🎫 #1234 🔴 Ensure click() triggers JavaScript in link href in Firefox 42 with Selenium 2.48.x as it did in 2.47.1. Provide regression fix or tests validating the JS href behavior.
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 Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No auditing: The new behavior allowing null coordinates and updating the internal map has no accompanying audit/logging of this critical emulation change, but this may be handled elsewhere in the system. Referred Code public SetGeolocationOverrideParameters(GeolocationCoordinates coordinates) { if (coordinates == null) { map.put("coordinates", null); } else { map.put("coordinates", coordinates.toMap()); } Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit 7b89898

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡	🎫 #5678 🔴 Investigate and resolve repeated "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04 with Chrome/ChromeDriver versions specified. Ensure ChromeDriver initialization works reliably beyond the first instance without console connection errors. Provide fix or guidance specific to Selenium 3.9.0 and Chrome 65/ChromeDriver 2.35 on Linux 4.10.0.
🟡	🎫 #1234 🔴 Ensure Selenium 2.48 triggers JavaScript in link href on click() in Firefox 42, matching behavior of 2.47.1. Provide regression fix or compatibility handling for Firefox-specific click behavior.
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 Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
	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 Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	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: The new handling for null coordinates changes behavior but does not add any logging to create an audit trail of geolocation override actions. Referred Code public SetGeolocationOverrideParameters(GeolocationCoordinates coordinates) { if (coordinates == null) { map.put("coordinates", null); } else { map.put("coordinates", coordinates.toMap()); } Learn more about managing compliance generic rules or creating your own custom rules

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Improve API with a factory method Improve API usability by introducing a static factory method reset() in SetGeolocationOverrideParameters to avoid the need for casting null when resetting the geolocation override. java/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java [227-229] emul.setGeolocationOverride( - new SetGeolocationOverrideParameters((GeolocationCoordinates) null) - .contexts(List.of(contextId))); + SetGeolocationOverrideParameters.reset().contexts(List.of(contextId))); Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies an API usability issue due to constructor overloading ambiguity and proposes a standard design pattern (static factory method) to resolve it, which improves code readability and usability.	Low
Learned best practice	✅ Close context in finally Suggestion Impact: The commit added an explicit context.close() after the assertions, ensuring the context is closed. While not in a try-finally block, it addresses the resource cleanup intent. code diff: + + context.close(); Wrap context creation/usage in try-finally and close the BrowsingContext in finally to avoid leaks if assertions throw. java/test/org/openqa/selenium/bidi/emulation/SetGeolocationOverrideTest.java [201-235] BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle()); -String contextId = context.getId(); -... -// Error because there's no real geolocation available -assertThat(r2.containsKey("error")).isTrue(); +try { + String contextId = context.getId(); + String url = appServer.whereIsSecure("blank.html"); + context.navigate(url, ReadinessState.COMPLETE); + driver.switchTo().window(context.getId()); + String origin = + (String) ((JavascriptExecutor) driver).executeScript("return window.location.origin;"); + Emulation emul = new Emulation(driver); + GeolocationCoordinates coords = new GeolocationCoordinates(37.7749, -122.4194); + emul.setGeolocationOverride( + new SetGeolocationOverrideParameters(coords).contexts(List.of(contextId))); + Object firstResult = getBrowserGeolocation(driver, null, origin); + Map<String, Object> r1 = ((Map<String, Object>) firstResult); + assertThat(r1.containsKey("error")).isFalse(); + double latitude1 = ((Number) r1.get("latitude")).doubleValue(); + double longitude1 = ((Number) r1.get("longitude")).doubleValue(); + assertThat(abs(latitude1 - coords.getLatitude())).isLessThan(0.0001); + assertThat(abs(longitude1 - coords.getLongitude())).isLessThan(0.0001); + emul.setGeolocationOverride( + new SetGeolocationOverrideParameters((GeolocationCoordinates) null) + .contexts(List.of(contextId))); + Object secondResult = getBrowserGeolocation(driver, null, origin); + Map<String, Object> r2 = ((Map<String, Object>) secondResult); + assertThat(r2.containsKey("error")).isTrue(); +} finally { + context.close(); +} [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Ensure resources are cleaned up using try-finally or equivalent to close/tear down contexts and user contexts, even on exceptions.	Low
Update

[navin772]

LGTM!