Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Oct 5, 2025

User description

https://w3c.github.io/webdriver-bidi/#event-browsingContext-downloadWillBegin
https://w3c.github.io/webdriver-bidi/#event-browsingContext-downloadEnd

💥 What does this PR do?

Fix downloadWillBegin event signature, implement downloadEnd event. Adding tests.

🔄 Types of changes

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

PR Type

Enhancement


Description

  • Fix downloadWillBegin event signature to use proper event args

  • Implement new downloadEnd event with polymorphic event args

  • Add comprehensive tests for both download events

  • Create JSON converters for proper serialization


Diagram Walkthrough

flowchart LR
  A["BrowsingContext"] --> B["OnDownloadWillBeginAsync"]
  A --> C["OnDownloadEndAsync"]
  B --> D["DownloadWillBeginEventArgs"]
  C --> E["DownloadEndEventArgs"]
  E --> F["DownloadCanceledEventArgs"]
  E --> G["DownloadCompleteEventArgs"]
  H["JSON Converter"] --> E
  I["Tests"] --> B
  I --> C
Loading

File Walkthrough

Relevant files
Enhancement
BrowsingContext.cs
Update download event method signatures                                   

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs

  • Fix OnDownloadWillBeginAsync method signatures to use
    DownloadWillBeginEventArgs
  • Add new OnDownloadEndAsync methods for both sync and async handlers
+12/-2   
BrowsingContextModule.cs
Update module download event subscriptions                             

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextModule.cs

  • Fix OnDownloadWillBeginAsync method signatures to use proper event
    args
  • Add new OnDownloadEndAsync methods subscribing to
    browsingContext.downloadEnd
+12/-2   
DownloadEndEventArgs.cs
Create download end event args classes                                     

dotnet/src/webdriver/BiDi/BrowsingContext/DownloadEndEventArgs.cs

  • Create abstract base class DownloadEndEventArgs
  • Implement DownloadCanceledEventArgs and DownloadCompleteEventArgs
    derived types
  • Include navigation info and timestamp properties
+35/-0   
DownloadWillBeginEventArgs.cs
Create download will begin event args                                       

dotnet/src/webdriver/BiDi/BrowsingContext/DownloadWillBeginEventArgs.cs

  • Create DownloadWillBeginEventArgs record with suggested filename
  • Implement IBaseNavigationInfo interface for navigation context
+25/-0   
DownloadEndEventArgsConverter.cs
Create polymorphic download end converter                               

dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/DownloadEndEventArgsConverter.cs

  • Implement custom JSON converter for polymorphic DownloadEndEventArgs
  • Handle canceled and complete status discriminators
  • Support deserialization to appropriate derived types
+45/-0   
Configuration changes
Broker.cs
Register download end event converter                                       

dotnet/src/webdriver/BiDi/Communication/Broker.cs

  • Register DownloadEndEventArgsConverter for polymorphic JSON
    serialization
+1/-0     
BiDiJsonSerializerContext.cs
Register download event types for serialization                   

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs

  • Add JSON serializable attributes for all download event args types
  • Include both base and derived download event classes
+4/-0     
Tests
BrowsingContextEventsTest.cs
Add download events integration tests                                       

dotnet/test/common/BiDi/BrowsingContext/BrowsingContextEventsTest.cs

  • Add test for downloadWillBegin event with file download simulation
  • Add test for downloadEnd event verifying completion and filepath
  • Validate event args properties and types
+65/-0   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Oct 5, 2025
Copy link
Contributor

qodo-merge-pro bot commented Oct 5, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unimplemented serialization

Description: The custom JSON converter throws NotImplementedException on serialization for
DownloadEndEventArgs.Write, which could cause a runtime crash if the type is ever
serialized (e.g., logged or echoed back); consider implementing or explicitly preventing
serialization paths.
DownloadEndEventArgsConverter.cs [41-44]

Referred Code
public override void Write(Utf8JsonWriter writer, DownloadEndEventArgs value, JsonSerializerOptions options)
{
    throw new NotImplementedException();
}
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
No custom compliance provided

Follow the guide to enable custom compliance check.

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

Copy link
Contributor

qodo-merge-pro bot commented Oct 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Throw exception on unknown discriminator

In the Read method, throw a JsonException for unknown status discriminators
instead of returning null to prevent silent failures and improve error handling.

dotnet/src/webdriver/BiDi/Communication/Json/Converters/Polymorphic/DownloadEndEventArgsConverter.cs [31-39]

 public override DownloadEndEventArgs? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
 {
-    return reader.GetDiscriminator("status") switch
+    var discriminator = reader.GetDiscriminator("status");
+    return discriminator switch
     {
         "canceled" => JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<DownloadCanceledEventArgs>()),
         "complete" => JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<DownloadCompleteEventArgs>()),
-        _ => null,
+        _ => throw new JsonException($"Unknown 'status' discriminator '{discriminator}' for DownloadEndEventArgs."),
     };
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that returning null for an unknown discriminator can hide issues. Throwing an exception improves robustness by making failures explicit, which is a good practice for deserialization logic.

Medium
  • Update

@nvborisenko nvborisenko merged commit 3baf4bc into SeleniumHQ:trunk Oct 5, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-download-events branch October 5, 2025 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants