[py][bidi]: add downloadEnd event for browsing context

[navin772]

User description

🔗 Related Issues

💥 What does this PR do?

Adds support for the downloadEnd event in bidi browsing context module - https://w3c.github.io/webdriver-bidi/#event-browsingContext-downloadEnd

🔧 Implementation Notes

💡 Additional Considerations

1. This event works in Chrome 141 (currently in canary), so the test will be added later.
2. I have also updated DownloadWillBeginParams's from_json method to be better as done for other Download classes.

🔄 Types of changes

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

---

PR Type

Enhancement

---

Description

• Add downloadEnd event support for BiDi browsing context

• Implement DownloadCanceledParams and DownloadCompleteParams classes

• Refactor DownloadWillBeginParams.from_json method for consistency

• Register new event in BrowsingContext EVENT_CONFIGS

---

Diagram Walkthrough

flowchart LR
  A["NavigationInfo base class"] --> B["DownloadCanceledParams"]
  A --> C["DownloadCompleteParams"]
  B --> D["DownloadEndParams"]
  C --> D
  D --> E["DownloadEnd event class"]
  E --> F["BrowsingContext EVENT_CONFIGS"]

Loading

File Walkthrough

Relevant files

Enhancement

browsing_context.py Add downloadEnd event classes and BiDi integration              py/selenium/webdriver/common/bidi/browsing_context.py Add DownloadCanceledParams and DownloadCompleteParams classes Implement DownloadEndParams wrapper class with status-based factory Create DownloadEnd event class for BiDi integration Refactor DownloadWillBeginParams.from_json to use NavigationInfo.from_json +101/-29

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1234 - Partially compliant Compliant requirements: None Non-compliant requirements: Investigate regression where clicking a link with JavaScript in href no longer triggers in 2.48.x on Firefox 42. Provide fix or workaround ensuring click() triggers JS in href. Verify behavior specifically on Firefox. Requires further human verification: None 5678 - Partially compliant Compliant requirements: None Non-compliant requirements: Diagnose intermittent "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances. Identify OS/driver/browser compatibility or lifecycle issues causing failures after the first instance. Provide reliable solution or guidance. Validate on Ubuntu 16.04.4 with Chrome 65 and ChromeDriver 2.35. Requires further human verification: None

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Validation Consistency Consider validating allowed keys for DownloadEnd payloads to ensure no unexpected fields slip through compared to spec; current parsing only checks 'status' branch and minimal type checks. def from_json(cls, json: dict) -> "DownloadEndParams": status = json.get("status") if status == "canceled": return cls(DownloadCanceledParams.from_json(json)) elif status == "complete": return cls(DownloadCompleteParams.from_json(json)) else: raise ValueError("status must be either 'canceled' or 'complete'") Type Annotation Import 'Union' is used in DownloadEndParams but ensure it's imported in this module; otherwise this could fail type checking or runtime if annotations are evaluated. self, download_params: Union[DownloadCanceledParams, DownloadCompleteParams], ): self.download_params = download_params Event Registration Confirm EVENT_CONFIGS key naming aligns with external consumers; introducing 'download_end' must match any public API or docs to avoid breaking subscriptions. EVENT_CONFIGS = { "context_created": EventConfig("context_created", "browsingContext.contextCreated", ContextCreated), "context_destroyed": EventConfig("context_destroyed", "browsingContext.contextDestroyed", ContextDestroyed), "dom_content_loaded": EventConfig("dom_content_loaded", "browsingContext.domContentLoaded", DomContentLoaded), "download_end": EventConfig("download_end", "browsingContext.downloadEnd", DownloadEnd), "download_will_begin": EventConfig( "download_will_begin", "browsingContext.downloadWillBegin", DownloadWillBegin ),

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Validate spec fields and types The new DownloadEnd parsing assumes only "status" plus basic NavigationInfo fields, but the BiDi spec may include additional required fields (e.g., reason for cancel, download id, error details) or different type constraints; rejecting anything except 'canceled'/'complete' without passthrough could make it brittle across browser versions. Cross-check against the latest WebDriver BiDi downloadEnd schema and ensure all mandated fields are mapped and surfaced (not silently dropped), and that type validation (including timestamp range/type and optional filepath semantics) strictly follows the spec. Examples: py/selenium/webdriver/common/bidi/browsing_context.py [354-380] class DownloadCanceledParams(NavigationInfo): def __init__( self, context: str, navigation: Optional[str], timestamp: int, url: str, status: str = "canceled", ): super().__init__(context, navigation, timestamp, url) ... (clipped 17 lines) py/selenium/webdriver/common/bidi/browsing_context.py [428-436] @classmethod def from_json(cls, json: dict) -> "DownloadEndParams": status = json.get("status") if status == "canceled": return cls(DownloadCanceledParams.from_json(json)) elif status == "complete": return cls(DownloadCompleteParams.from_json(json)) else: raise ValueError("status must be either 'canceled' or 'complete'") Solution Walkthrough: Before: class DownloadCanceledParams(NavigationInfo): # ... # Missing `reason` field. class DownloadEndParams: @classmethod def from_json(cls, json: dict): status = json.get("status") if status == "canceled": return cls(DownloadCanceledParams.from_json(json)) elif status == "complete": return cls(DownloadCompleteParams.from_json(json)) else: raise ValueError("status must be 'canceled' or 'complete'") After: class DownloadCanceledParams(NavigationInfo): def __init__(self, ..., reason: str): super().__init__(...) self.reason = reason # ... from_json also parses `reason` class DownloadErrorParams(NavigationInfo): # ... New class to handle 'error' status with a 'reason' field. class DownloadEndParams: @classmethod def from_json(cls, json: dict): status = json.get("status") if status == "canceled": return cls(DownloadCanceledParams.from_json(json)) elif status == "complete": return cls(DownloadCompleteParams.from_json(json)) elif status == "error": return cls(DownloadErrorParams.from_json(json)) else: raise ValueError("Unsupported download status from spec") Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical flaw where the implementation diverges from the WebDriver BiDi spec by omitting the error status and the required reason field for canceled downloads, which would lead to runtime errors.	High
Possible issue	Guard against non-dict inputs Validate that json is a dictionary before accessing keys to avoid runtime errors when malformed payloads are received. Add a defensive check and raise a clear error if the input is not a dict. py/selenium/webdriver/common/bidi/browsing_context.py [419-436] class DownloadEndParams: """Parameters for the downloadEnd event.""" def __init__( self, download_params: Union[DownloadCanceledParams, DownloadCompleteParams], ): self.download_params = download_params @classmethod def from_json(cls, json: dict) -> "DownloadEndParams": + if not isinstance(json, dict): + raise ValueError("json must be a dictionary") status = json.get("status") if status == "canceled": return cls(DownloadCanceledParams.from_json(json)) elif status == "complete": return cls(DownloadCompleteParams.from_json(json)) else: raise ValueError("status must be either 'canceled' or 'complete'") Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly adds a check to ensure the json input is a dictionary, which improves the robustness of the from_json method against malformed data.	Low
General	Validate status type strictly Ensure status is a string before comparing to avoid truthy non-string values passing basic presence checks. Add a type check to prevent unexpected inputs from being accepted. py/selenium/webdriver/common/bidi/browsing_context.py [354-380] class DownloadCanceledParams(NavigationInfo): def __init__( self, context: str, navigation: Optional[str], timestamp: int, url: str, status: str = "canceled", ): super().__init__(context, navigation, timestamp, url) self.status = status @classmethod def from_json(cls, json: dict) -> "DownloadCanceledParams": nav_info = NavigationInfo.from_json(json) status = json.get("status") - if status is None or status != "canceled": + if not isinstance(status, str) or status != "canceled": raise ValueError("status is required and must be 'canceled'") return cls( context=nav_info.context, navigation=nav_info.navigation, timestamp=nav_info.timestamp, url=nav_info.url, status=status, ) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion improves the validation logic by adding a strict type check for the status field, making the parsing more robust and preventing non-string values from being processed.	Low
	Normalize empty filepath values Normalize the optional filepath field by converting empty strings to None. This avoids treating empty paths as valid file locations and prevents downstream file handling errors. py/selenium/webdriver/common/bidi/browsing_context.py [383-416] class DownloadCompleteParams(NavigationInfo): def __init__( self, context: str, navigation: Optional[str], timestamp: int, url: str, status: str = "complete", filepath: Optional[str] = None, ): super().__init__(context, navigation, timestamp, url) self.status = status self.filepath = filepath @classmethod def from_json(cls, json: dict) -> "DownloadCompleteParams": nav_info = NavigationInfo.from_json(json) status = json.get("status") if status is None or status != "complete": raise ValueError("status is required and must be 'complete'") filepath = json.get("filepath") if filepath is not None and not isinstance(filepath, str): raise ValueError("filepath must be a string if provided") + # Normalize empty string to None + if isinstance(filepath, str) and filepath.strip() == "": + filepath = None return cls( context=nav_info.context, navigation=nav_info.navigation, timestamp=nav_info.timestamp, url=nav_info.url, status=status, filepath=filepath, ) Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion to normalize an empty string filepath to None is a good practice for data consistency, preventing potential downstream issues with empty but non-null path values.	Low
Update

[shbenzer]

LGTM - once you’ve got a test or 2 for this I think you’re good to go

[navin772]

Current stable browsers are not supporting this event, so can't test it :(

[shbenzer]

Hm, that’s an interesting case then. We might want to put this pr on hold… what do y’all think @cgoldberg @diemol ?

[cgoldberg]

Current stable browsers are not supporting this event
what do y’all think

hmm... that's a tough call. If we release it now, it's not fully tested and not usable by most people... which might cause confusion. If we wait for Chrome/Edge/Firefox to all support it in stable versions, it's safer but we are late to the party and might be holding back useful functionality for a subset of users.

I think either way has a valid argument, so I'm not sure :)

Whatever we decide, we should make it a project policy for timing the release of new BiDi functionality. Have other bindings hit this issue?

[navin772]

Have other bindings hit this issue?

Dotnet adds some new events without tests (like #15916), there are some events in Java which don't have tests - onDownloadWillBegin, onNavigationFailed which I am planning to add.

[diemol]

I think we can add this, and perhaps write in the equivalent of JavaDocs for Python that this is supported on X version of the browser. Otherwise this PR might become stale, like many others we have.

[cgoldberg]

Dotnet adds some new events without tests

I more meant the issue of releasing features that all browsers don't yet support in stable versions.

I like the idea from @diemol of just adding a note in docstrings that it requires the browser to support this. My only concern is how nicely this fails when run on a browser that doesn't support it. Do we give some cryptic error? Other than that, this seems fine to ship.

[shbenzer]

My biggest concern would be that we don’t know if this code truly works (though I’m sure it does, @navin772 is awesome) until at least one browser supports it - I am uncomfortable w/ the precedent of merging untested code.

[navin772]

equivalent of JavaDocs for Python that this is supported on X version of the browser.

Chrome 140 supports this (next stable version) and there is no info regarding edge and firefox, they are currently failing the WPT tests. What should the docstring include in this case?

My biggest concern would be that we don’t know if this code truly works

@shbenzer This is a valid concern, I just tested this code against Chrome 140 which is currently in beta and it works as expected. We recently started testing python tests against chrome beta in CI-RBE so this test should pass there too.

But the issue will be that we need to selectively skip the chrome stable test in CI (so CI stays green) for which we don't have a xfail marker as of now. Should we add it? Is this a better approach or just wait for 140 to release?

[cgoldberg]

What should the docstring include in this case?

I think we should add something vague in a module-level docstring about BiDi functionality only working in supported browsers. I don't think we should get into detail there about which browsers and versions currently work with each feature, or do this for every method. We don't want to constantly update the individual docstrings and keep an internal version of BiDi feature support. Maybe all of the BiDi modules should have a similar docstring.

I think more important is the behavior and error message when someone tries to use some BiDi functionality on a browser that doesn't support it. It would be nice to fail with an error like "sorry, your browser doesn't support this yet" instead of some cryptic exception (I don't know what currently happens).

[navin772]

We get a pretty good error message from the browser which we pass on to the user's stdout. For example, firefox throws this error:

selenium.common.exceptions.WebDriverException: Message: invalid argument: browsingContext.downloadEnd is not a valid event name

But chrome isn't throwing any error and would fail when no event is received.

Should we modify this behaviour?

[shbenzer]

We get a pretty good error message from the browser which we pass on to the user's stdout. For example, firefox throws this error:

selenium.common.exceptions.WebDriverException: Message: invalid argument: browsingContext.downloadEnd is not a valid event name

But chrome isn't throwing any error and would fail when no event is received.

Should we modify this behaviour?

We could append to it w/ some clarifying details

[nvborisenko]

My 50 cents: Selenium should not wait until all vendors implement BiDi spec. We can implement the specification in advance. But the question is are we sure we implement it correctly? My personal plan in .NET area is easy: implement method/event, add small subset of tests which works at least with one browser, we are happy - release to end users.

Mentioning something in DocStrings is not necessary, because browser can be updated at some time with supporting of new method/event, thus DoCStrings become invalid. Note: Users might easily request to use beta/dev/preview browsers.

[cgoldberg]

Mentioning something in DocStrings is not necessary, because browser can be updated at some time with supporting of new method/event, thus DoCStrings become invalid

That's why I recommended not mentioning specific browsers or versions. I think a note in doctrings that says "some of these classes/methods may not be supported yet in all browsers" is useful until BiDi is fully supported... Users will see it in the API documentation.

[nvborisenko]

I think a note in doctrings that says "some of these classes/methods may not be supported yet in all browsers" is useful until BiDi is fully supported...

It is good to mention it globally, not for particular method/event.

[cgoldberg]

It is good to mention it globally, not for particular method/event

In Python we have module docstrings, which is where I suggested it go.

[navin772]

In Python we have module docstrings, which is where I suggested it go.

@cgoldberg where should I put the docstring? Should it be at the top of the bidi/browsing_context.py file? The above docstring you mentioned is applicable for all modules, so is there a better place to put it so it applies to all modules?

[cgoldberg]

where should I put the docstring?

I just looked at the API docs and noticed module level docstrings show up in the Table of Contents next to each module name, so we probably can't put a big block of text. I guess we should hold off on adding this until we figure out a good place. That shouldn't hold up this PR.