[java][BiDi] implement browser.setDownloadBehavior

[Delta456]

User description

🔗 Related Issues

💥 What does this PR do?

Implements browser.setDownloadBehavior from the W3C spec

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

---

PR Type

Enhancement

---

Description

• Implements W3C BiDi browser.setDownloadBehavior command

• Supports allowed/denied download behaviors with destination folder

• Enables per-user-context download behavior configuration

• Includes comprehensive test coverage for all scenarios

---

Diagram Walkthrough

flowchart LR
  A["SetDownloadBehaviorParameters"] -->|"parameters"| B["Browser.setDownloadBehavior"]
  B -->|"sends command"| C["BiDi Command"]
  C -->|"browser.setDownloadBehavior"| D["W3C BiDi Spec"]
  E["Test Cases"] -->|"validates"| B

Loading

File Walkthrough

	Relevant files
Enhancement	SetDownloadBehaviorParameters.java New download behavior parameters class                                      java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java New parameter class for configuring download behavior settings Supports both String and Path for destination folder specification Validates parameter combinations (allowed=true requires destinationFolder) Provides fluent API for setting optional userContexts list +46/-0    Browser.java Add setDownloadBehavior method to Browser module                  java/src/org/openqa/selenium/bidi/module/Browser.java Added import for SetDownloadBehaviorParameters New setDownloadBehavior() method that sends BiDi command Method accepts SetDownloadBehaviorParameters and converts to map +5/-0
Tests	BrowserCommandsTest.java Add comprehensive tests for setDownloadBehavior                    java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java Updated import from AssertionsForClassTypes to Assertions Added three comprehensive test cases for download behavior scenarios Tests cover allowed downloads, denied downloads, and user-context-specific behavior Uses temporary filesystem and WebDriverWait for file verification Includes proper cleanup with finally blocks +169/-1

---

[qodo-merge-pro]

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
🟡	🎫 #5678 🔴 Investigate and resolve ChromeDriver "Error: ConnectFailure (Connection refused)" occurring on subsequent ChromeDriver instantiations on Ubuntu 16.04.4 with Chrome 65/ChromeDriver 2.35 and Selenium 3.9.0. Provide guidance or a fix so that only the first instance not failing is addressed and subsequent instances no longer show the error.
🟡	🎫 #1234 🔴 Ensure WebDriver 2.48 triggers JavaScript embedded in a link's href on click() in Firefox 42 (regression from 2.47.1). Provide fix or behavior parity so alerts are fired as expected when clicking such links.
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: 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: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Auditing: The new critical action 'browser.setDownloadBehavior' is executed without any explicit logging or audit trail in the added code. Referred Code public void setDownloadBehavior(SetDownloadBehaviorParameters parameters) { bidi.send(new Command<>("browser.setDownloadBehavior", parameters.toMap())); } 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: Input Validation: The parameters accept a List of user contexts without null/empty validation and throw IllegalArgumentException without contextual logging, which may hinder diagnosing edge cases. Referred Code public SetDownloadBehaviorParameters userContexts(List<String> userContexts) { map.put("userContexts", userContexts); return this; } 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: Parameter Checks: While basic logical validation is present, the code does not normalize or validate the 'destinationFolder' path beyond toAbsolutePath nor validate 'userContexts' contents, which may require additional safeguards depending on upstream inputs. Referred Code public SetDownloadBehaviorParameters(Boolean allowed, Path destinationFolder) { if (allowed == null) { map.put("downloadBehavior", null); } else if (allowed) { if (destinationFolder == null) { throw new IllegalArgumentException("destinationFolder is required when allowed is true"); } Map<String, String> behavior = new HashMap<>(); behavior.put("type", "allowed"); behavior.put("destinationFolder", destinationFolder.toAbsolutePath().toString()); map.put("downloadBehavior", behavior); } else { if (destinationFolder != null) { throw new IllegalArgumentException( "destinationFolder should not be provided when allowed is false"); } Map<String, String> behavior = new HashMap<>(); behavior.put("type", "denied"); map.put("downloadBehavior", behavior); } } ... (clipped 5 lines) Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

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

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Improve API design using static factories Refactor the SetDownloadBehaviorParameters class to use static factory methods (allow, deny, reset) instead of a constructor with complex boolean-based logic. This change will result in a more expressive and less error-prone API. Examples: java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java [12-36] public SetDownloadBehaviorParameters(Boolean allowed, String destinationFolder) { this(allowed, destinationFolder != null ? Paths.get(destinationFolder) : null); } public SetDownloadBehaviorParameters(Boolean allowed, Path destinationFolder) { if (allowed == null) { map.put("downloadBehavior", null); } else if (allowed) { if (destinationFolder == null) { throw new IllegalArgumentException("destinationFolder is required when allowed is true"); ... (clipped 15 lines) java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java [152] browser.setDownloadBehavior(new SetDownloadBehaviorParameters(false, (Path) null)); Solution Walkthrough: Before: public class SetDownloadBehaviorParameters { public SetDownloadBehaviorParameters(Boolean allowed, Path destinationFolder) { if (allowed == null) { // reset behavior } else if (allowed) { if (destinationFolder == null) { throw new IllegalArgumentException(...); } // set allowed behavior } else { if (destinationFolder != null) { throw new IllegalArgumentException(...); } // set denied behavior } } // ... } After: public class SetDownloadBehaviorParameters { private SetDownloadBehaviorParameters() { /* ... */ } public static SetDownloadBehaviorParameters allow(Path destinationFolder) { // ... logic to set allowed behavior } public static SetDownloadBehaviorParameters deny() { // ... logic to set denied behavior } public static SetDownloadBehaviorParameters reset() { // ... logic to reset behavior } // ... } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a design weakness in the new public API and proposes a well-established pattern (static factory methods) that significantly improves readability and usability.	Medium
Possible issue	✅ Clean up temporary test directories Suggestion Impact: The commit adds calls to TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile()) in finally blocks, cleaning up the temporary directories as suggested. code diff: } finally { browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null, (Path) null)); + TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile()); } } @@ -178,6 +179,7 @@ } } finally { browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null, (Path) null)); + TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile()); } } @@ -257,6 +259,7 @@ } } finally { browser.removeUserContext(userContext); + TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile()); Add cleanup logic to the finally block to delete the temporary directory created during the test. This prevents resource leaks on the file system. java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java [113-142] Path tmpDir = TemporaryFilesystem.getDefaultTmpFS().createTempDir("downloads", "test").toPath(); try { browser.setDownloadBehavior(new SetDownloadBehaviorParameters(true, tmpDir)); BrowsingContext context = new BrowsingContext(driver, driver.getWindowHandle()); String url = appServer.whereIs("downloads/download.html"); context.navigate(url, ReadinessState.COMPLETE); driver.findElement(By.id("file-1")).click(); new WebDriverWait(driver, Duration.ofSeconds(5)) .until( d -> { try { return Files.list(tmpDir) .anyMatch(path -> path.getFileName().toString().equals("file_1.txt")); } catch (Exception e) { return false; } }); List<String> fileNames = Files.list(tmpDir) .map(path -> path.getFileName().toString()) .collect(Collectors.toList()); assertThat(fileNames).contains("file_1.txt"); } finally { browser.setDownloadBehavior(new SetDownloadBehaviorParameters(null, (Path) null)); + TemporaryFilesystem.getDefaultTmpFS().deleteTempDir(tmpDir.toFile()); } [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a resource leak where temporary directories created for tests are not cleaned up, which is important for test suite hygiene, especially on CI servers.	Medium
Learned best practice	Add clear Javadoc documentation Add Javadoc for the class and methods to document parameter semantics, especially how null/true/false affect behavior and user contexts. java/src/org/openqa/selenium/bidi/browser/SetDownloadBehaviorParameters.java [9-46] +/** + * Parameters for the BiDi command browser.setDownloadBehavior. + * <p>allowed == null clears any existing behavior. + * <p>allowed == true requires a non-null destinationFolder, which specifies the download directory. + * <p>allowed == false denies downloads and destinationFolder must be null. + * <p>Optional user contexts can scope the behavior to specific contexts. + */ public class SetDownloadBehaviorParameters { private final Map<String, Object> map = new HashMap<>(); - + + /** + * Creates parameters using a string path for the destination folder. + * @param allowed null to clear, true to allow (requires destinationFolder), false to deny (must be null) + * @param destinationFolder destination folder when allowed is true; must be null when allowed is false + */ public SetDownloadBehaviorParameters(Boolean allowed, String destinationFolder) { this(allowed, destinationFolder != null ? Paths.get(destinationFolder) : null); } - + + /** + * Creates parameters using a Path for the destination folder. + * @param allowed null to clear, true to allow (requires destinationFolder), false to deny (must be null) + * @param destinationFolder destination folder when allowed is true; must be null when allowed is false + */ public SetDownloadBehaviorParameters(Boolean allowed, Path destinationFolder) { if (allowed == null) { map.put("downloadBehavior", null); } else if (allowed) { - ... + if (destinationFolder == null) { + throw new IllegalArgumentException("destinationFolder is required when allowed is true"); + } + Map<String, String> behavior = new HashMap<>(); + behavior.put("type", "allowed"); + behavior.put("destinationFolder", destinationFolder.toAbsolutePath().toString()); + map.put("downloadBehavior", behavior); } else { - ... + if (destinationFolder != null) { + throw new IllegalArgumentException( + "destinationFolder should not be provided when allowed is false"); + } + Map<String, String> behavior = new HashMap<>(); + behavior.put("type", "denied"); + map.put("downloadBehavior", behavior); } } - + + /** + * Restrict the behavior to specific user contexts. If omitted, applies globally. + * @param userContexts list of user context ids + * @return this instance for chaining + */ public SetDownloadBehaviorParameters userContexts(List<String> userContexts) { map.put("userContexts", userContexts); return this; } - + + /** Returns the underlying command parameters as a map. */ public Map<String, Object> toMap() { return map; } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Enforce accurate and consistent documentation and naming to match actual behavior and APIs.	Low
Update