Skip to content

[java][bidi] Remove subscription scope - #17842

Merged
pujagani merged 1 commit into
SeleniumHQ:trunkfrom
pujagani:remove-subscription-scope
Jul 29, 2026
Merged

[java][bidi] Remove subscription scope#17842
pujagani merged 1 commit into
SeleniumHQ:trunkfrom
pujagani:remove-subscription-scope

Conversation

@pujagani

Copy link
Copy Markdown
Contributor

💥 What does this PR do?

Removes per-context / per-user-context ("scoped") BiDi event subscription support, keeping only global subscriptions.

🔧 Implementation Notes

  • A delivered event message carries no subscription-id or scope-identifying metadata — just {method, params}.
  • The server deduplicates delivery at the connection level: two overlapping subscriptions to the same event+scope produce exactly one delivered message, not one per subscription.
  • The server does gate delivery by context correctly at subscribe/unsubscribe — unsubscribing one of two differently-scoped subscriptions stops only that context's events.
  • But scope is not a strict exact-context-id match: subscribing to a child browsing context can still surface parent/sibling frame events (test_subscribe_to_child_context) — real browsing-context-group/frame-tree bleed-through the current SubscriptionScope API neither models nor tests for.
  • This makes the part of having consumers be called per subscription scope not possible currently. It is better to stick to global subscription.
  • This is a clean up before generator code can be merged.

🤖 AI assistance

  • AI assisted (complete below)
    • Tool(s): Claude Code
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jul 29, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Remove scoped BiDi event subscriptions; keep global-only subscribe/unsubscribe

✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Remove per-context/user-context BiDi subscription scoping and related overloads.
• Keep only global event subscriptions to match actual delivered event semantics.
• Clarify public Module subscribe/unsubscribe API with Javadoc and simplify call chain.
Diagram

graph TD
  A["BiDi Module API"] --> B["Handle"] --> C["BiDi"] --> D{{"Browser BiDi endpoint"}}
  D --> E["Connection"] --> F["Client handler"]
  C --> E
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deprecate scoped subscriptions (keep API temporarily)
  • ➕ Avoids immediate breaking API removal for downstream users
  • ➕ Allows gradual migration to global subscriptions
  • ➖ Keeps a misleading API surface that suggests semantics the transport cannot guarantee
  • ➖ Delays cleanup needed for generator and future API consistency
2. Client-side filtering instead of transport scope
  • ➕ Preserves per-context/user-context intent without relying on server scope fidelity
  • ➕ Works even when server scope semantics bleed across frame trees/groups
  • ➖ Higher event volume delivered to clients
  • ➖ Requires richer event metadata and consistent context identifiers across all event types
3. Redesign scope model to match browsing-context groups/frame trees
  • ➕ Aligns API with actual remote-end delivery behavior
  • ➕ Could support future accurate scoping semantics
  • ➖ Significant design and test investment
  • ➖ Still constrained by what the protocol includes in delivered event payloads

Recommendation: Proceed with this PR’s approach: remove scoped subscriptions and standardize on global subscriptions. Given that delivered events carry no subscription-id/scope metadata and the existing scope semantics are leaky (not strict context-id matching), keeping the scoped API risks incorrect expectations. A deprecation period is a reasonable alternative, but since the removed type was @beta and no remaining references exist, the clean removal is the most coherent path before generator integration.

Files changed (3) +15 / -24

Enhancement (1) +15 / -7
Module.javaMake subscribe/unsubscribe public global-only API with Javadoc +15/-7

Make subscribe/unsubscribe public global-only API with Javadoc

• Removes protected scoped subscription helpers and promotes global subscribe/unsubscribe to a documented public API. This clarifies that subscriptions apply across all browsing contexts.

java/src/org/openqa/selenium/bidi/Module.java

Refactor (2) +0 / -17
BiDi.javaRemove scoped addListener overload and scope param building +0/-13

Remove scoped addListener overload and scope param building

• Deletes the addListener overload that accepted a SubscriptionScope and built subscribe parameters from it. This leaves a single listener registration path aligned with global subscription behavior.

java/src/org/openqa/selenium/bidi/BiDi.java

Handle.javaDrop scoped subscribe delegate method +0/-4

Drop scoped subscribe delegate method

• Removes the Handle.subscribe overload that accepted a SubscriptionScope, so Handle only supports global event subscriptions and unsubscription by id.

java/src/org/openqa/selenium/bidi/Handle.java

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (2) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 18 rules

Grey Divider


Action required

1. Scoped callbacks leak 🐞 Bug ≡ Correctness
Description
BiDi still allows context-scoped subscriptions via addListener(String/Set,...), but
Connection.handleEventResponse dispatches callbacks purely by event method and invokes all handlers
for that method regardless of subscription id or context. If two subscriptions to the same method
exist for different browsing contexts, both handlers will run for every received event of that
method, causing cross-context callback execution.
Code

java/src/org/openqa/selenium/bidi/BiDi.java[L120-130]

-  <X> String addListener(Event<X> event, Consumer<X> handler, SubscriptionScope scope) {
-    Require.nonNull("Event to listen for", event);
-    Require.nonNull("Handler to call", handler);
-    Require.nonNull("Subscription scope", scope);
-
-    Map<String, Object> params = new HashMap<>(scope.toMap());
-    params.put("events", List.of(event.getMethod()));
-    String subscriptionId = subscribe(params);
-    connection.addListener(subscriptionId, event, handler);
-    return subscriptionId;
-  }
Evidence
BiDi still creates scoped subscriptions using a contexts parameter, while Connection dispatches
events to callbacks solely by matching the event method and then iterating all registered
callbacks for that method, ignoring subscription id/scope—so multiple same-method scoped
subscriptions will receive each other’s events.

java/src/org/openqa/selenium/bidi/BiDi.java[95-116]
java/src/org/openqa/selenium/bidi/Connection.java[347-395]

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

### Issue description
`SubscriptionScope` support is removed, but BiDi still exposes context-scoped `addListener` overloads. Because `Connection.handleEventResponse` dispatches by event `method` only and does not filter callbacks by subscription id or context, multiple same-method subscriptions with different context scopes will cross-trigger handlers.

### Issue Context
- `BiDi.addListener(String/Set, ...)` still calls `session.subscribe` with a `contexts` parameter and stores the handler under the returned `subscriptionId`.
- `Connection.handleEventResponse` matches only on `method` and calls **all** handlers registered for that event method.

### Fix Focus Areas
- java/src/org/openqa/selenium/bidi/BiDi.java[95-116]
- java/src/org/openqa/selenium/bidi/Connection.java[347-395]

### Suggested fix direction
Choose one (consistent with PR goal of “global subscriptions only”):
1) **Remove/deprecate** the context-scoped `BiDi.addListener(String, ...)` and `BiDi.addListener(Set<String>, ...)` overloads (and update any internal callers to subscribe globally and filter in the handler), **or**
2) If keeping scoped subscriptions, extend the callback registry to retain enough scope metadata and **filter callbacks during dispatch** based on event payload context fields (so handlers only run for matching scopes).

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



Remediation recommended

2. SubscriptionScope API removed 📘 Rule violation ≡ Correctness
Description
This PR removes the public SubscriptionScope type and the `Module.subscribe(Event, Consumer,
SubscriptionScope)` overload, which is a backward-incompatible API/ABI change for external callers
and subclasses. The removed API is not preserved via a deprecation phase with guidance to an
alternative, increasing upgrade risk for downstream users.
Code

java/src/org/openqa/selenium/bidi/SubscriptionScope.java[L26-74]

-/**
- * Where a subscription applies: globally, or scoped to browsing contexts and/or user contexts. Part
- * of the transport layer, not generated — the remote end decides which combinations are valid.
- *
- * <p>This class is intentionally limited to the scope of a subscription, not the full set of
- * subscribe parameters.
- *
- * @see <a href="https://www.w3.org/TR/webdriver-bidi/#type-session-SubscriptionParameters">
- *     session.SubscriptionParameters</a>
- */
-@Beta
-public final class SubscriptionScope {
-
-  private Set<String> contexts = Set.of();
-  private Set<String> userContexts = Set.of();
-
-  /**
-   * Scopes the subscription to the given browsing contexts.
-   *
-   * @param contexts the browsing context ids to scope the subscription to
-   * @return this scope, for chaining
-   */
-  public SubscriptionScope contexts(Set<String> contexts) {
-    this.contexts = Set.copyOf(Require.nonNull("Browsing context ids", contexts));
-    return this;
-  }
-
-  /**
-   * Scopes the subscription to the given user contexts.
-   *
-   * @param userContexts the user context ids to scope the subscription to
-   * @return this scope, for chaining
-   */
-  public SubscriptionScope userContexts(Set<String> userContexts) {
-    this.userContexts = Set.copyOf(Require.nonNull("User context ids", userContexts));
-    return this;
-  }
-
-  Map<String, Object> toMap() {
-    Map<String, Object> params = new HashMap<>();
-    if (!contexts.isEmpty()) {
-      params.put("contexts", contexts);
-    }
-    if (!userContexts.isEmpty()) {
-      params.put("userContexts", userContexts);
-    }
-    return params;
-  }
-}
Evidence
Module in the PR branch exposes only subscribe(Event, Consumer) and unsubscribe(String) and no
longer provides a scoped-subscribe overload, indicating the scoped API surface was removed. No
remaining Java code references SubscriptionScope, indicating the type has been removed from the
Java BiDi API surface in this PR rather than being left as a deprecated shim for migration.

Rule 389266: Maintain backward-compatible public API and ABI
Rule 389271: Deprecate public APIs with guidance before removal
java/src/org/openqa/selenium/bidi/Module.java[44-67]
java/src/org/openqa/selenium/bidi/Handle.java[40-50]

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 BiDi subscription-scoping APIs were removed (`SubscriptionScope` and the `Module.subscribe(..., SubscriptionScope)` overload). This breaks backward compatibility and removes the chance for users to migrate via a deprecation period.

## Issue Context
- `Module` is a public, subclassed base type (`public abstract class Module`). Removing a `protected` overload breaks subclasses.
- The PR removes a public type (`SubscriptionScope`) rather than deprecating it with migration guidance.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[44-67]
- java/src/org/openqa/selenium/bidi/Handle.java[40-50]
- java/src/org/openqa/selenium/bidi/BiDi.java[86-117]
- java/src/org/openqa/selenium/bidi/SubscriptionScope.java[1-120]

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


3. No cross-binding scope comparison 📘 Rule violation ≡ Correctness
Description
Java BiDi subscription documentation now states subscriptions are global-only, but other language
bindings still model scoped subscription parameters (e.g., Ruby includes contexts and
userContexts). This indicates a user-visible behavioral/API divergence without nearby
documentation of cross-binding comparison or an explicit, documented divergence decision.
Code

java/src/org/openqa/selenium/bidi/Module.java[R48-58]

+  /**
+   * Subscribes to a BiDi event, globally across all browsing contexts.
+   *
+   * @param event the event to subscribe to
+   * @param handler invoked with the event's parameters each time it fires
+   * @param <X> the event's parameter type
+   * @return a subscription id that can be passed to {@link #unsubscribe(String)}
+   */
+  public final <X> String subscribe(Event<X> event, Consumer<X> handler) {
    return handle.subscribe(event, handler);
  }
Evidence
Module.subscribe(...) Javadoc in Java explicitly states subscriptions are global across all
browsing contexts. Ruby's BiDi protocol still defines subscribe parameters that include contexts
and userContexts, demonstrating another binding still models scoped subscriptions and motivating
the need for explicit cross-binding comparison/documentation.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
java/src/org/openqa/selenium/bidi/Module.java[48-58]
rb/lib/selenium/webdriver/bidi/protocol/session.rb[152-157]

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

## Issue description
The Java binding now documents global-only event subscriptions, while other bindings still expose scoped subscription parameters. The change is user-visible and should be cross-checked and either aligned or explicitly documented as an intentional divergence.

## Issue Context
Ruby's generated BiDi protocol includes `SubscribeParameters` with `contexts` and `userContexts`, suggesting scope support still exists outside Java.

## Fix Focus Areas
- java/src/org/openqa/selenium/bidi/Module.java[48-58]
- rb/lib/selenium/webdriver/bidi/protocol/session.rb[152-157]

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


Grey Divider

To customize comments, go to the Qodo configuration screen, or learn more in the docs.

Qodo Logo

Comment thread java/src/org/openqa/selenium/bidi/SubscriptionScope.java
Comment thread java/src/org/openqa/selenium/bidi/Module.java
Comment thread java/src/org/openqa/selenium/bidi/BiDi.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants