[java][BiDi] implement browsingContext.downloadEnd event

[Delta456]

User description

🔗 Related Issues

💥 What does this PR do?

Implements browsingContext.downloadEnd event according to spec https://w3c.github.io/webdriver-bidi/#event-browsingContext-downloadEnd for Java binding.

ONLY supported for Chrome atm.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

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

---

PR Type

Enhancement

---

Description

• Implement browsingContext.downloadEnd event for Java BiDi binding

• Add support for download completion and cancellation states

• Create new classes for handling download end scenarios

• Update test to verify download end event functionality

---

Diagram Walkthrough

flowchart LR
  A["DownloadEnded"] --> B["DownloadCompleted"]
  A --> C["DownloadCanceled"]
  D["BrowsingContextInspector"] --> E["onDownloadEnd listener"]
  E --> A

Loading

File Walkthrough

	Relevant files
Enhancement	DownloadCanceled.java Add DownloadCanceled class for canceled downloads                java/src/org/openqa/selenium/bidi/browsingcontext/DownloadCanceled.java New class extending NavigationInfo for canceled downloads Implements JSON deserialization with status validation Provides getter for download status +79/-0    DownloadCompleted.java Add DownloadCompleted class for successful downloads          java/src/org/openqa/selenium/bidi/browsingcontext/DownloadCompleted.java New class extending NavigationInfo for completed downloads Includes filepath property for download location Implements JSON parsing with status validation +95/-0    DownloadEnded.java Add DownloadEnded wrapper for download end events                java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java Main wrapper class for download end events Factory method creates appropriate subclass based on status Provides helper methods to check download state +93/-0    BrowsingContextInspector.java Add download end event support to inspector                            java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java Add downloadWillEndMapper for JSON deserialization Create downloadWillEndEvent for browsingContext.downloadEnd Add onDownloadEnd method for event subscription Update close method to clear new event listener +21/-6
Tests	BrowsingContextInspectorTest.java Update test for download end event                                              java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java Update test from downloadWillBegin to downloadEnd event Verify download completion status and context ID Test download end event functionality +8/-8

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: Non-compliant requirements: Investigate/resolve ChromeDriver connection failure issue. Requires further human verification: Reproduce connection failure across multiple ChromeDriver instantiations on Ubuntu 16.04 environment. Validate compatibility matrix (Chrome/ChromeDriver/Selenium versions) and networking/environmental causes. 1234 - Partially compliant Compliant requirements: Non-compliant requirements: Address click() not triggering javascript in href on Firefox. Requires further human verification: Re-run the provided test case on affected Firefox/WebDriver versions to confirm behavior.

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

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Null Handling If the incoming JSON lacks a 'status' field, an IllegalArgumentException is thrown. Confirm that the BiDi payload always includes 'status' per spec; otherwise consider a clearer error message or default. // Create the appropriate object based on status if ("canceled".equals(status)) { DownloadCanceled canceled = new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status); return new DownloadEnded(canceled); } else if ("complete".equals(status)) { DownloadCompleted completed = new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath); return new DownloadEnded(completed); } else { throw new IllegalArgumentException( "status must be either 'canceled' or 'complete', but got: " + status); } Field Optionality 'filepath' may be absent depending on browser implementation; ensure downstream consumers handle null gracefully and consider documenting nullability. case "filepath": filepath = input.read(String.class); break; default: input.skipValue(); break; } } input.endObject(); return new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath); } public String getStatus() { return status; } public String getFilepath() { return filepath; } Listener Lifecycle New downloadEnd listener is added and cleared, but ensure symmetry when multiple onDownloadEnd registrations occur and verify no leaks with navigationEventSet interactions. private final Event<DownloadEnded> downloadWillEndEvent = new Event<>("browsingContext.downloadEnd", downloadWillEndMapper); private final Event<UserPromptOpened> userPromptOpened = new Event<>( "browsingContext.userPromptOpened", params -> { try (StringReader reader = new StringReader(JSON.toJson(params)); JsonInput input = JSON.newInput(reader)) { return input.read(UserPromptOpened.class); } }); private final Event<HistoryUpdated> historyUpdated = new Event<>( "browsingContext.historyUpdated", params -> { try (StringReader reader = new StringReader(JSON.toJson(params)); JsonInput input = JSON.newInput(reader)) { return input.read(HistoryUpdated.class); } }); public BrowsingContextInspector(WebDriver driver) { this(new HashSet<>(), driver); } public BrowsingContextInspector(String browsingContextId, WebDriver driver) { this(Collections.singleton(Require.nonNull("Browsing context id", browsingContextId)), driver); } public BrowsingContextInspector(Set<String> browsingContextIds, WebDriver driver) { Require.nonNull("WebDriver", driver); Require.nonNull("Browsing context id list", browsingContextIds); if (!(driver instanceof HasBiDi)) { throw new IllegalArgumentException("WebDriver instance must support BiDi protocol"); } this.bidi = ((HasBiDi) driver).getBiDi(); this.browsingContextIds = browsingContextIds; } public void onBrowsingContextCreated(Consumer<BrowsingContextInfo> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(browsingContextCreated, consumer); } else { this.bidi.addListener(browsingContextIds, browsingContextCreated, consumer); } } public void onBrowsingContextDestroyed(Consumer<BrowsingContextInfo> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(browsingContextDestroyed, consumer); } else { this.bidi.addListener(browsingContextIds, browsingContextDestroyed, consumer); } } public void onNavigationStarted(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.navigationStarted", consumer); } public void onFragmentNavigated(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.fragmentNavigated", consumer); } public void onDomContentLoaded(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.domContentLoaded", consumer); } public void onBrowsingContextLoaded(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.load", consumer); } public void onDownloadWillBegin(Consumer<DownloadInfo> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(downloadWillBeginEvent, consumer); } else { this.bidi.addListener(browsingContextIds, downloadWillBeginEvent, consumer); } } public void onDownloadEnd(Consumer<DownloadEnded> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(downloadWillEndEvent, consumer); } else { this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer); } } public void onNavigationAborted(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.navigationAborted", consumer); } public void onNavigationFailed(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.navigationFailed", consumer); } public void onNavigationCommitted(Consumer<NavigationInfo> consumer) { addNavigationEventListener("browsingContext.navigationCommitted", consumer); } public void onUserPromptClosed(Consumer<UserPromptClosed> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(userPromptClosed, consumer); } else { this.bidi.addListener(browsingContextIds, userPromptClosed, consumer); } } public void onUserPromptOpened(Consumer<UserPromptOpened> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(userPromptOpened, consumer); } else { this.bidi.addListener(browsingContextIds, userPromptOpened, consumer); } } public void onHistoryUpdated(Consumer<HistoryUpdated> consumer) { if (browsingContextIds.isEmpty()) { this.bidi.addListener(historyUpdated, consumer); } else { this.bidi.addListener(browsingContextIds, historyUpdated, consumer); } } private void addNavigationEventListener(String name, Consumer<NavigationInfo> consumer) { Event<NavigationInfo> navigationEvent = new Event<>(name, navigationInfoMapper); navigationEventSet.add(navigationEvent); if (browsingContextIds.isEmpty()) { this.bidi.addListener(navigationEvent, consumer); } else { this.bidi.addListener(browsingContextIds, navigationEvent, consumer); } } @Override public void close() { this.bidi.clearListener(browsingContextCreated); this.bidi.clearListener(browsingContextDestroyed); this.bidi.clearListener(userPromptOpened); this.bidi.clearListener(userPromptClosed); this.bidi.clearListener(historyUpdated); this.bidi.clearListener(downloadWillBeginEvent); this.bidi.clearListener(downloadWillEndEvent);

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 8b155ab

Category	Suggestion	Impact
General	Support both filePath key casings In DownloadCompleted.fromJson, add support for both filePath and filepath JSON keys to correctly parse the downloaded file's path, prioritizing the camelCase version as per the specification. java/src/org/openqa/selenium/bidi/browsingcontext/DownloadCompleted.java [73-75] -case "filepath": +case "filePath": filepath = input.read(String.class); break; +case "filepath": + // Fallback for implementations using lowercase + if (filepath == null) { + filepath = input.read(String.class); + } else { + input.skipValue(); + } + break; + Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies that the WebDriver BiDi specification uses filePath (camelCase), while the code only handles filepath. This is a bug fix that ensures compatibility with spec-compliant browsers, preventing the file path from being missed during JSON parsing.	Medium
	✅ Fix inconsistent handler naming Suggestion Impact: The commit renames downloadWillEndMapper to downloadEndMapper and downloadWillEndEvent to downloadEndEvent, and updates all their usages including addListener and clearListener calls. code diff: - private final Function<Map<String, Object>, DownloadEnded> downloadWillEndMapper = + private final Function<Map<String, Object>, DownloadEnded> downloadEndMapper = params -> { try (StringReader reader = new StringReader(JSON.toJson(params)); JsonInput input = JSON.newInput(reader)) { @@ -94,8 +94,8 @@ private final Event<DownloadInfo> downloadWillBeginEvent = new Event<>("browsingContext.downloadWillBegin", downloadWillBeginMapper); - private final Event<DownloadEnded> downloadWillEndEvent = - new Event<>("browsingContext.downloadEnd", downloadWillEndMapper); + private final Event<DownloadEnded> downloadEndEvent = + new Event<>("browsingContext.downloadEnd", downloadEndMapper); private final Event<UserPromptOpened> userPromptOpened = new Event<>( @@ -179,9 +179,9 @@ public void onDownloadEnd(Consumer<DownloadEnded> consumer) { if (browsingContextIds.isEmpty()) { - this.bidi.addListener(downloadWillEndEvent, consumer); - } else { - this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer); + this.bidi.addListener(downloadEndEvent, consumer); + } else { + this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer); } } @@ -241,7 +241,7 @@ this.bidi.clearListener(userPromptClosed); this.bidi.clearListener(historyUpdated); this.bidi.clearListener(downloadWillBeginEvent); - this.bidi.clearListener(downloadWillEndEvent); + this.bidi.clearListener(downloadEndEvent); navigationEventSet.forEach(this.bidi::clearListener); In BrowsingContextInspector, rename the downloadWillEndMapper and downloadWillEndEvent fields to downloadEndMapper and downloadEndEvent to match the event name browsingContext.downloadEnd, and update their usages accordingly. java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [68-186] -private final Function<Map<String, Object>, DownloadEnded> downloadWillEndMapper = +private final Function<Map<String, Object>, DownloadEnded> downloadEndMapper = params -> { try (StringReader reader = new StringReader(JSON.toJson(params)); - JsonInput input = JSON.newInput(reader)) { + JsonInput input = JSON.newInput(reader)) { return input.read(DownloadEnded.class); } }; -... - -private final Event<DownloadEnded> downloadWillEndEvent = - new Event<>("browsingContext.downloadEnd", downloadWillEndMapper); - -... +private final Event<DownloadEnded> downloadEndEvent = + new Event<>("browsingContext.downloadEnd", downloadEndMapper); public void onDownloadEnd(Consumer<DownloadEnded> consumer) { if (browsingContextIds.isEmpty()) { - this.bidi.addListener(downloadWillEndEvent, consumer); + this.bidi.addListener(downloadEndEvent, consumer); } else { - this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer); + this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer); } } +@Override +public void close() { + this.bidi.clearListener(browsingContextCreated); + this.bidi.clearListener(browsingContextDestroyed); + this.bidi.clearListener(userPromptOpened); + this.bidi.clearListener(userPromptClosed); + this.bidi.clearListener(historyUpdated); + this.bidi.clearListener(downloadWillBeginEvent); + this.bidi.clearListener(downloadEndEvent); + + navigationEventSet.forEach(this.bidi::clearListener); +} + [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 4 __ Why: The suggestion improves code consistency by renaming internal variables like downloadWillEndMapper and downloadWillEndEvent to align with the event name browsingContext.downloadEnd. This enhances readability and maintainability, though it is a minor style improvement.	Low
Possible issue	Validate required fields early In DownloadEnded.fromJson, add validation to ensure required fields like status, context, timestamp, and url are not null after parsing the JSON, throwing an IllegalArgumentException if any are missing. java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java [30-79] public static DownloadEnded fromJson(JsonInput input) { String browsingContextId = null; String navigationId = null; - long timestamp = 0; + Long timestamp = null; String url = null; String status = null; String filepath = null; input.beginObject(); while (input.hasNext()) { switch (input.nextName()) { case "context": browsingContextId = input.read(String.class); break; case "navigation": navigationId = input.read(String.class); break; case "timestamp": timestamp = input.read(Long.class); break; case "url": url = input.read(String.class); break; case "status": status = input.read(String.class); break; case "filepath": filepath = input.read(String.class); break; default: input.skipValue(); break; } } input.endObject(); - // Create the appropriate object based on status + if (status == null) { + throw new IllegalArgumentException("Missing required field: status"); + } + if (browsingContextId == null) { + throw new IllegalArgumentException("Missing required field: context"); + } + if (timestamp == null) { + throw new IllegalArgumentException("Missing required field: timestamp"); + } + if (url == null) { + throw new IllegalArgumentException("Missing required field: url"); + } + if ("canceled".equals(status)) { DownloadCanceled canceled = new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status); return new DownloadEnded(canceled); } else if ("complete".equals(status)) { DownloadCompleted completed = new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath); return new DownloadEnded(completed); } else { throw new IllegalArgumentException( "status must be either 'canceled' or 'complete', but got: " + status); } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that missing fields in the JSON payload can lead to partially initialized objects and later runtime errors. Adding explicit checks improves robustness and provides clearer error messages, which is a good practice for error handling.	Medium
Update

---

Previous suggestions

✅ Suggestions up to commit 173391f

Category	Suggestion	Impact
High-level	Simplify the class hierarchy and deserialization Refactor the class hierarchy by making DownloadEnded an abstract base class extended by DownloadCompleted and DownloadCanceled. This change will remove the need for the wrapper class, eliminate redundant fromJson methods in the subclasses, and simplify the overall design. Examples: java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java [22-93] public class DownloadEnded { private final NavigationInfo downloadParams; public DownloadEnded(NavigationInfo downloadParams) { this.downloadParams = downloadParams; } public static DownloadEnded fromJson(JsonInput input) { String browsingContextId = null; ... (clipped 62 lines) java/src/org/openqa/selenium/bidi/browsingcontext/DownloadCompleted.java [39-86] public static DownloadCompleted fromJson(JsonInput input) { String browsingContextId = null; String navigationId = null; long timestamp = 0; String url = null; String status = "complete"; String filepath = null; input.beginObject(); while (input.hasNext()) { ... (clipped 38 lines) Solution Walkthrough: Before: // In DownloadEnded.java public class DownloadEnded { private final NavigationInfo downloadParams; // ... constructor ... public static DownloadEnded fromJson(JsonInput input) { // ... manually parse all fields ... if ("canceled".equals(status)) { DownloadCanceled canceled = new DownloadCanceled(...); return new DownloadEnded(canceled); } else if ("complete".equals(status)) { DownloadCompleted completed = new DownloadCompleted(...); return new DownloadEnded(completed); } // ... } public NavigationInfo getDownloadParams() { /* ... */ } } After: // In DownloadEnded.java public abstract class DownloadEnded extends NavigationInfo { // ... common fields and methods ... public static DownloadEnded fromJson(JsonInput input) { // ... manually parse all fields ... if ("canceled".equals(status)) { return new DownloadCanceled(...); // Returns subclass instance } else if ("complete".equals(status)) { return new DownloadCompleted(...); // Returns subclass instance } // ... } } // DownloadCompleted and DownloadCanceled would extend DownloadEnded // and their own fromJson methods would be removed. Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies dead code (fromJson in subclasses) and proposes a significant design improvement that simplifies the class structure, removes an unnecessary wrapper class, and creates a more direct polymorphic API.	Medium
General	✅ Refactor to remove duplicated JSON parsing Suggestion Impact: The method was refactored to read the JSON into a Map, extract status, and delegate to DownloadCanceled.fromJson or DownloadCompleted.fromJson using a new JsonInput, removing the previous manual field-by-field parsing and object construction. code diff: public static DownloadEnded fromJson(JsonInput input) { - String browsingContextId = null; - String navigationId = null; - long timestamp = 0; - String url = null; - String status = null; - String filepath = null; + Map<String, Object> jsonMap = input.read(Map.class); + String status = (String) jsonMap.get("status"); - input.beginObject(); - while (input.hasNext()) { - switch (input.nextName()) { - case "context": - browsingContextId = input.read(String.class); - break; - case "navigation": - navigationId = input.read(String.class); - break; - case "timestamp": - timestamp = input.read(Long.class); - break; - case "url": - url = input.read(String.class); - break; - case "status": - status = input.read(String.class); - break; - case "filepath": - filepath = input.read(String.class); - break; - default: - input.skipValue(); - break; + try (StringReader reader = new StringReader(new Json().toJson(jsonMap)); + JsonInput jsonInput = new Json().newInput(reader)) { + if ("canceled".equals(status)) { + return new DownloadEnded(DownloadCanceled.fromJson(jsonInput)); + } else if ("complete".equals(status)) { + return new DownloadEnded(DownloadCompleted.fromJson(jsonInput)); + } else { + throw new IllegalArgumentException( + "status must be either 'canceled' or 'complete', but got: " + status); } - } - input.endObject(); - - // Create the appropriate object based on status - if ("canceled".equals(status)) { - DownloadCanceled canceled = - new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status); - return new DownloadEnded(canceled); - } else if ("complete".equals(status)) { - DownloadCompleted completed = - new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath); - return new DownloadEnded(completed); - } else { - throw new IllegalArgumentException( - "status must be either 'canceled' or 'complete', but got: " + status); } Refactor the DownloadEnded.fromJson method to eliminate redundant JSON parsing logic by delegating deserialization to the DownloadCanceled.fromJson or DownloadCompleted.fromJson methods based on the status field. java/src/org/openqa/selenium/bidi/browsingcontext/DownloadEnded.java [30-79] public static DownloadEnded fromJson(JsonInput input) { - String browsingContextId = null; - String navigationId = null; - long timestamp = 0; - String url = null; - String status = null; - String filepath = null; + Map<String, Object> jsonMap = input.read(Map.class); + String status = (String) jsonMap.get("status"); - input.beginObject(); - while (input.hasNext()) { - switch (input.nextName()) { - case "context": - browsingContextId = input.read(String.class); - break; - case "navigation": - navigationId = input.read(String.class); - break; - case "timestamp": - timestamp = input.read(Long.class); - break; - case "url": - url = input.read(String.class); - break; - case "status": - status = input.read(String.class); - break; - case "filepath": - filepath = input.read(String.class); - break; - default: - input.skipValue(); - break; + try (StringReader reader = new StringReader(new Json().toJson(jsonMap)); + JsonInput jsonInput = new Json().newInput(reader)) { + if ("canceled".equals(status)) { + return new DownloadEnded(DownloadCanceled.fromJson(jsonInput)); + } else if ("complete".equals(status)) { + return new DownloadEnded(DownloadCompleted.fromJson(jsonInput)); + } else { + throw new IllegalArgumentException( + "status must be either 'canceled' or 'complete', but got: " + status); } - } - input.endObject(); - - // Create the appropriate object based on status - if ("canceled".equals(status)) { - DownloadCanceled canceled = - new DownloadCanceled(browsingContextId, navigationId, timestamp, url, status); - return new DownloadEnded(canceled); - } else if ("complete".equals(status)) { - DownloadCompleted completed = - new DownloadCompleted(browsingContextId, navigationId, timestamp, url, status, filepath); - return new DownloadEnded(completed); - } else { - throw new IllegalArgumentException( - "status must be either 'canceled' or 'complete', but got: " + status); } } [Suggestion processed] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies duplicated JSON parsing logic in DownloadEnded.fromJson and proposes a valid refactoring to delegate deserialization, which improves code structure and maintainability.	Low
Learned best practice	✅ Use accurate event naming Suggestion Impact: The commit renamed downloadWillEndEvent to downloadEndEvent, updated the mapper name to downloadEndMapper, and changed all usages in listener registration and clearing accordingly. code diff: - private final Function<Map<String, Object>, DownloadEnded> downloadWillEndMapper = + private final Function<Map<String, Object>, DownloadEnded> downloadEndMapper = params -> { try (StringReader reader = new StringReader(JSON.toJson(params)); JsonInput input = JSON.newInput(reader)) { @@ -94,8 +94,8 @@ private final Event<DownloadInfo> downloadWillBeginEvent = new Event<>("browsingContext.downloadWillBegin", downloadWillBeginMapper); - private final Event<DownloadEnded> downloadWillEndEvent = - new Event<>("browsingContext.downloadEnd", downloadWillEndMapper); + private final Event<DownloadEnded> downloadEndEvent = + new Event<>("browsingContext.downloadEnd", downloadEndMapper); private final Event<UserPromptOpened> userPromptOpened = new Event<>( @@ -179,9 +179,9 @@ public void onDownloadEnd(Consumer<DownloadEnded> consumer) { if (browsingContextIds.isEmpty()) { - this.bidi.addListener(downloadWillEndEvent, consumer); - } else { - this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer); + this.bidi.addListener(downloadEndEvent, consumer); + } else { + this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer); } } @@ -241,7 +241,7 @@ this.bidi.clearListener(userPromptClosed); this.bidi.clearListener(historyUpdated); this.bidi.clearListener(downloadWillBeginEvent); - this.bidi.clearListener(downloadWillEndEvent); + this.bidi.clearListener(downloadEndEvent); navigationEventSet.forEach(this.bidi::clearListener); Rename downloadWillEndEvent to reflect the actual event name (downloadEnd) for clarity and consistency with other events. This avoids confusion and aligns with event semantics. java/src/org/openqa/selenium/bidi/module/BrowsingContextInspector.java [97-247] -private final Event<DownloadEnded> downloadWillEndEvent = +private final Event<DownloadEnded> downloadEndEvent = new Event<>("browsingContext.downloadEnd", downloadWillEndMapper); ... public void onDownloadEnd(Consumer<DownloadEnded> consumer) { if (browsingContextIds.isEmpty()) { - this.bidi.addListener(downloadWillEndEvent, consumer); + this.bidi.addListener(downloadEndEvent, consumer); } else { - this.bidi.addListener(browsingContextIds, downloadWillEndEvent, consumer); + this.bidi.addListener(browsingContextIds, downloadEndEvent, consumer); } } ... public void close() { this.bidi.clearListener(browsingContextCreated); this.bidi.clearListener(browsingContextDestroyed); this.bidi.clearListener(userPromptOpened); this.bidi.clearListener(userPromptClosed); this.bidi.clearListener(historyUpdated); this.bidi.clearListener(downloadWillBeginEvent); - this.bidi.clearListener(downloadWillEndEvent); + this.bidi.clearListener(downloadEndEvent); navigationEventSet.forEach(this.bidi::clearListener); } Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Always close or dispose resources and clear listeners to prevent leaks; ensure new listeners are cleared on shutdown using consistent fields.	Low

[asolntsev]

suggested minor changes.

[asolntsev]

Should we use word "canceled" or "cancelled"?
Both are correct, while "canceled" American English, and "cancelled" is British English.

I've checked that in Selenium Java code,

• "cancelled" is used 17 times, including enum value Status.CANCELLED
• "canceled" is used 14 times

[cgoldberg]

I've actually had this same discussion about this word a few times in my career :)

Unlike "color/colour", "canceled" is pretty commonly used in both American/British English, but more common in American.

Across our entire codebase, the American spelling is the winner:

$ grep -r --ignore-case --exclude-dir=.git 'canceled\|canceling' | wc -l
169
$ grep -r --ignore-case --exclude-dir=.git 'cancelled\|cancelling' | wc -l
164

However, if we exclude 3rd party code we have vendored, British spelling has a slight edge:

$ grep -r --ignore-case --exclude-dir=.git --exclude-dir=common/devtools --exclude-dir=third_party 'canceled\|canceling' | wc -l
77
$ grep -r --ignore-case --exclude-dir=.git --exclude-dir=common/devtools --exclude-dir=third_party 'cancelled\|cancelling' | wc -l
88

So, I'd call it a draw.

[Delta456]

The spec uses canceled, which is a hardcore value for checking downloads.

[Delta456]

Ruby failure is related to my PR.