[dotnet] [bidi] Align SetDownloadBehavior command

[nvborisenko]

Simplified polymorphic command parameters.

💥 What does this PR do?

This pull request refactors and simplifies the API for setting browser download behavior in the BiDi WebDriver implementation. The main change is consolidating multiple specific methods into a single, more flexible method that accepts a DownloadBehavior parameter. This also involves updating related interfaces, records, and tests to use the new approach.

API Refactoring and Simplification:

• Consolidated the three separate methods (SetDownloadBehaviorAllowedAsync, SetDownloadBehaviorDeniedAsync, and the overload for allowed with no path) into a single method: SetDownloadBehaviorAsync, which takes a DownloadBehavior parameter. This provides a more flexible and unified API for setting download behavior. (dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs, dotnet/src/webdriver/BiDi/Browser/IBrowserModule.cs) [1] [2]
• Updated the DownloadBehavior record and its derived types (DownloadBehaviorAllowed, DownloadBehaviorDenied) to be public, and added a static Default property for clarity when resetting to default behavior. (dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs)

Test Updates:

• Refactored tests to use the new SetDownloadBehaviorAsync method, including tests for allowed, denied, and default behaviors. Also added assertions for both null and Default usage. (dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs)
• Added missing using directive for the OpenQA.Selenium.BiDi.Browser namespace in the test file for clarity and correctness. (dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs)

🔄 Types of changes

• Cleanup (formatting, renaming)

[qodo-code-review]

Review Summary by Qodo

Simplify SetDownloadBehavior API with unified polymorphic method

✨ Enhancement

Walkthroughs

Description

• Consolidate three separate download behavior methods into single unified method
• Accept flexible DownloadBehavior parameter instead of specific method overloads
• Make DownloadBehavior types public with static Default property
• Remove browser-specific test ignores and update tests to new API

File Changes

1. dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs ✨ Enhancement +2/-16

Consolidate download behavior methods into unified API

• Replace three separate methods (SetDownloadBehaviorAllowedAsync with/without path,
 SetDownloadBehaviorDeniedAsync) with single SetDownloadBehaviorAsync method
• New method accepts nullable DownloadBehavior parameter for flexible behavior specification
• Simplify parameter construction to use passed downloadBehavior directly

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

---

2. dotnet/src/webdriver/BiDi/Browser/IBrowserModule.cs ✨ Enhancement +1/-3

Update interface with unified method signature

• Remove three method signatures for separate download behavior operations
• Add single SetDownloadBehaviorAsync method signature accepting DownloadBehavior? parameter
• Align interface with simplified implementation

dotnet/src/webdriver/BiDi/Browser/IBrowserModule.cs

---

3. dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs ✨ Enhancement +6/-3

Make download behavior types public with Default property

• Change DownloadBehavior abstract record from internal to public
• Add static Default property returning null for clarity
• Make DownloadBehaviorAllowed and DownloadBehaviorDenied records public
• Enable external API consumers to construct behavior instances directly

dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs

---

View more (1)

4. dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs 🧪 Tests +11/-13

Update tests for new unified download behavior API

• Add missing using OpenQA.Selenium.BiDi.Browser directive
• Update three test methods to use new SetDownloadBehaviorAsync method
• Remove IgnoreBrowser attributes from all three download behavior tests
• Add test demonstrating both null and DownloadBehavior.Default usage patterns

dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (3)   📘 Rule violations (2)   📎 Requirement gaps (0)   🎨 UX Issues (0)

🐞\ ≡ Correctness (1) ☼ Reliability (1) ⚙ Maintainability (1)

📘\ ☼ Reliability (1) ⚙ Maintainability (1)

1. Missing deprecation wrappers 📘 ⚙

Description

Removed public download-behavior methods were not kept as [Obsolete] shims pointing to
SetDownloadBehaviorAsync(...), so users receive no migration guidance. This violates the
requirement to deprecate public functionality before removal.

Code

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs[R74-79]

+    public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorAsync(DownloadBehavior? downloadBehavior, SetDownloadBehaviorOptions? options = null, CancellationToken cancellationToken = default)
    {
-        var @params = new SetDownloadBehaviorParameters(new DownloadBehaviorAllowed(destinationFolder), options?.UserContexts);
-
-        return await ExecuteAsync(SetDownloadBehaviorCommand, @params, options, cancellationToken).ConfigureAwait(false);
-    }
-
-    public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorAllowedAsync(SetDownloadBehaviorOptions? options = null, CancellationToken cancellationToken = default)
-    {
-        var @params = new SetDownloadBehaviorParameters(null, options?.UserContexts);
-
-        return await ExecuteAsync(SetDownloadBehaviorCommand, @params, options, cancellationToken).ConfigureAwait(false);
-    }
-
-    public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorDeniedAsync(SetDownloadBehaviorOptions? options = null, CancellationToken cancellationToken = default)
-    {
-        var @params = new SetDownloadBehaviorParameters(new DownloadBehaviorDenied(), options?.UserContexts);
+        var @params = new SetDownloadBehaviorParameters(downloadBehavior, options?.UserContexts);

        return await ExecuteAsync(SetDownloadBehaviorCommand, @params, options, cancellationToken).ConfigureAwait(false);
    }

Evidence

PR Compliance ID 7 requires deprecating public functionality before removal with guidance to the
alternative. BrowserModule now only provides SetDownloadBehaviorAsync(...) and contains no
obsolete wrapper methods that preserve the old API surface and direct users to the replacement.

AGENTS.md
dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs[74-79]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Public methods appear to have been removed without a deprecation period. Add compatibility shims marked obsolete that forward to the new unified method.

## Issue Context
Users should have a clear migration path and not be forced into immediate breaking changes.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/Browser/IBrowserModule.cs[22-30]
- dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs[74-79]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Hardcoded download path 🐞 ☼

Description

BrowserTests.CanSetDownloadBehaviorAllowed uses a hardcoded destination folder ("/my/path") without
creating it, which is likely non-existent/unwritable on CI and Windows and can cause
browser.setDownloadBehavior to fail at runtime. Since the PR also removed the IgnoreBrowser skips,
this test will now run broadly and is likely to fail the test suite.

Code

dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[75]

+        var result = await bidi.Browser.SetDownloadBehaviorAsync(new DownloadBehaviorAllowed("/my/path"));

Evidence

The updated .NET test passes a fixed Unix-style path without ensuring it exists; other bindings’
tests in this repo create a real temporary directory and use it for allowed downloads, indicating
the destination folder is expected to be valid/writable.

dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[72-78]
java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[51-53]
java/test/org/openqa/selenium/bidi/browser/BrowserCommandsTest.java[129-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`CanSetDownloadBehaviorAllowed` uses `"/my/path"` as the download destination without ensuring it exists or is writable. This is not cross-platform and is likely to fail on CI/Windows.

### Issue Context
Other bindings’ tests in this repo create a temporary directory and pass that to the command.

### Fix Focus Areas
- dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[72-78]

### Suggested fix
- Create a temp directory (e.g., `Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString())`) and `Directory.CreateDirectory(...)`.
- Pass that directory into `new DownloadBehaviorAllowed(tempDir)`.
- Optionally delete the directory in a `try/finally` (or TearDown) to avoid leaving artifacts.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Download tests not gated 📘 ☼

Description

CanSetDownloadBehavior* tests are now unconditional integration tests, which can cause
environment-dependent CI failures if the feature/driver support varies. Integration tests should be
gated based on feature/driver availability.

Code

dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[R72-99]

    [Test]
-    [IgnoreBrowser(Infrastructure.Browser.Chrome, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Edge, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Firefox, "Not supported yet?")]
    public async Task CanSetDownloadBehaviorAllowed()
    {
-        var result = await bidi.Browser.SetDownloadBehaviorAllowedAsync("/my/path");
+        var result = await bidi.Browser.SetDownloadBehaviorAsync(new DownloadBehaviorAllowed("/my/path"));

        Assert.That(result, Is.Not.Null);
    }

    [Test]
-    [IgnoreBrowser(Infrastructure.Browser.Chrome, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Edge, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Firefox, "Not supported yet?")]
-    public async Task CanSetDownloadBehaviorAllowedDefault()
+    public async Task CanSetDownloadBehaviorDefault()
    {
-        var result = await bidi.Browser.SetDownloadBehaviorAllowedAsync();
+        var result = await bidi.Browser.SetDownloadBehaviorAsync(null);
+
+        // or

+        var result2 = await bidi.Browser.SetDownloadBehaviorAsync(DownloadBehavior.Default);
+        
        Assert.That(result, Is.Not.Null);
+        Assert.That(result2, Is.Not.Null);
    }

    [Test]
-    [IgnoreBrowser(Infrastructure.Browser.Chrome, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Edge, "Not supported yet?")]
-    [IgnoreBrowser(Infrastructure.Browser.Firefox, "Not supported yet?")]
    public async Task CanSetDownloadBehaviorDenied()
    {
-        var result = await bidi.Browser.SetDownloadBehaviorDeniedAsync();
+        var result = await bidi.Browser.SetDownloadBehaviorAsync(new DownloadBehaviorDenied());

        Assert.That(result, Is.Not.Null);
    }

Evidence

PR Compliance ID 14 requires integration tests depending on optional features/driver support to be
conditionally executed. The BiDi download-behavior tests run unconditionally ([Test] only) with no
gating/assumptions visible in the changed test code.

dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[72-99]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Integration tests for setting download behavior run unconditionally and may fail on environments/browsers/drivers where the capability is not supported.

## Issue Context
CI environments and driver capabilities can vary; integration tests should be skipped/ignored when the prerequisite feature is unavailable.

## Fix Focus Areas
- dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs[72-99]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Unsupported behavior subtypes 🐞 ⚙

Description

DownloadBehavior is now a public abstract record and is accepted by SetDownloadBehaviorAsync, but
JSON polymorphism only registers two derived types (allowed/denied). If a consumer derives a custom
DownloadBehavior and passes it in, serialization/command execution will fail at runtime.

Code

dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[R29-32]

+public abstract record DownloadBehavior
+{
+    public static DownloadBehavior? Default => null;
+}

Evidence

The public base type suggests extensibility, but only two derived types are registered for
polymorphic serialization, making any other subtype unsupported by design.

dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[26-36]
dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs[74-79]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DownloadBehavior` is public + abstract, but the serializer is only configured for `DownloadBehaviorAllowed` and `DownloadBehaviorDenied`. External derived types are not supported and will fail at runtime, making the API misleading.

### Issue Context
Polymorphic serialization is configured via `[JsonDerivedType]` attributes for only two types.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[26-36]

### Suggested fix options (pick one)
1) **Block external inheritance**: add a `private protected` (or `internal`) parameterless constructor on `DownloadBehavior` so only this assembly can derive from it.
2) **Constrain the API**: make `DownloadBehavior` `sealed` and expose only factory methods/instances for allowed/denied (requires reshaping the model).
3) If you intentionally want extensibility, **register additional derived types** (and add clear docs); otherwise option (1) is preferred.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Missing folder input validation 🐞 ≡

Description

DownloadBehaviorAllowed is now public but does not validate DestinationFolder (null/empty), allowing
construction of an invalid payload that will be rejected by the remote end. Other bindings in this
repo enforce these invariants client-side, so .NET callers will get later, less actionable failures.

Code

dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[34]

+public sealed record DownloadBehaviorAllowed(string DestinationFolder) : DownloadBehavior;

Evidence

The .NET type is a simple record with no guards; in contrast, Java and Python implementations in
this repo validate that allowed requires a destination folder and denied must not include one.

dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[34-36]
java/src/org/openqa/selenium/bidi/browser/DownloadBehavior.java[44-52]
py/selenium/webdriver/common/bidi/browser.py[265-275]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DownloadBehaviorAllowed` does not validate `DestinationFolder`, so callers can pass null/empty/whitespace and generate an invalid command payload.

### Issue Context
Other bindings in this repo validate required/forbidden parameters for allowed/denied download behavior.

### Fix Focus Areas
- dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs[34-36]

### Suggested fix
- Replace the positional record with an explicit constructor that throws early:
 - `ArgumentNullException.ThrowIfNull(destinationFolder)`
 - `ArgumentException.ThrowIfNullOrWhiteSpace(destinationFolder)` (or equivalent)
- (Optional) Consider normalizing to a full path (`Path.GetFullPath`) if that aligns with BiDi expectations.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[Copilot]

Pull request overview

Refactors the .NET BiDi browser.setDownloadBehavior API to use a single polymorphic DownloadBehavior parameter, aligning call sites and related types with the unified command shape.

Changes:

• Consolidated multiple SetDownloadBehavior*Async methods into SetDownloadBehaviorAsync(DownloadBehavior?) on the Browser module API.
• Made DownloadBehavior and its derived records public, adding DownloadBehavior.Default as a “reset to default” alias.
• Updated BiDi browser tests to use the consolidated method and new public types.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
dotnet/test/webdriver/BiDi/Browser/BrowserTests.cs	Updates tests to use SetDownloadBehaviorAsync and the new DownloadBehavior* records.
dotnet/src/webdriver/BiDi/Browser/SetDownloadBehavior.cs	Exposes DownloadBehavior records publicly and adds a Default alias for resetting behavior.
dotnet/src/webdriver/BiDi/Browser/IBrowserModule.cs	Replaces the previous method set with the consolidated SetDownloadBehaviorAsync API.
dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs	Implements the consolidated SetDownloadBehaviorAsync method and removes the specialized methods.