Home Original page

[dotnet] [bidi] Null guard for event handlers by nvborisenko · Pull Request #16967 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Refactor to avoid massive duplication

The PR adds many nearly identical private handler methods, causing significant
code duplication. Refactor this by using a single generic private method to
handle context filtering for all events, which would be more maintainable.

Examples:

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [309-467] 
    private async Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler)
    {
        if (Equals(e.Context))
        {
            await handler(e).ConfigureAwait(false);
        }
    }

    private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler)
    {

 ... (clipped 149 lines)
dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextNetworkModule.cs [183-261] 
    private async Task HandleBeforeRequestSentAsync(BeforeRequestSentEventArgs e, Func<BeforeRequestSentEventArgs, Task> handler)
    {
        if (context.Equals(e.Context))
        {
            await handler(e).ConfigureAwait(false);
        }
    }

    private void HandleBeforeRequestSent(BeforeRequestSentEventArgs e, Action<BeforeRequestSentEventArgs> handler)
    {

 ... (clipped 69 lines)

Solution Walkthrough:

Before:

// In dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs
public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...)
{
    if (handler is null) throw new ArgumentNullException(nameof(handler));
    return BiDi.BrowsingContext.OnNavigationStartedAsync(
        e => HandleNavigationStarted(e, handler), ...);
}

private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler)
{
    if (Equals(e.Context)) { handler(e); }
}

// ... dozens of similar handler methods for different events ...
private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler)
{
    if (Equals(e.Context)) { handler(e); }
}

After:

// In a new helper class or within each module
private static Action<T> CreateContextFilteredHandler<T>(
    T e, Action<T> handler, BrowsingContext context) where T : IContextEventArgs
{
    return (args) =>
    {
        if (context.Equals(args.Context))
        {
            handler(args);
        }
    };
}

// In dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs
public Task<Subscription> OnNavigationStartedAsync(Action<NavigationInfo> handler, ...)
{
    if (handler is null) throw new ArgumentNullException(nameof(handler));
    return BiDi.BrowsingContext.OnNavigationStartedAsync(
        CreateContextFilteredHandler(handler, this), ...);
}
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies massive code duplication introduced across multiple files, which contradicts the PR's goal of improving maintainability and is a significant design flaw.

High
Possible issue
Restore special handling for subscription options

Restore the special handling for OnEntryAddedAsync subscription options, which
was removed during refactoring, to avoid unintended context scoping due to a
known spec issue.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContextLogModule.cs [28-44]

 public Task<Subscription> OnEntryAddedAsync(Func<Log.LogEntry, Task> handler, ContextSubscriptionOptions? options = null)
 {
     if (handler is null) throw new ArgumentNullException(nameof(handler));
 
     return logModule.OnEntryAddedAsync(
         e => HandleEntryAddedAsync(e, handler),
-        ContextSubscriptionOptions.WithContext(options, context));
+        new SubscriptionOptions() { Timeout = options?.Timeout }); // special case, don't scope to context, awaiting https://github.com/w3c/webdriver-bidi/issues/1032
 }
 
 public Task<Subscription> OnEntryAddedAsync(Action<Log.LogEntry> handler, ContextSubscriptionOptions? options = null)
 {
     if (handler is null) throw new ArgumentNullException(nameof(handler));
 
     return logModule.OnEntryAddedAsync(
         e => HandleEntryAdded(e, handler),
-        ContextSubscriptionOptions.WithContext(options, context));
+        new SubscriptionOptions() { Timeout = options?.Timeout }); // special case, don't scope to context, awaiting https://github.com/w3c/webdriver-bidi/issues/1032
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the refactoring removed special-case logic for OnEntryAddedAsync that was intentionally added to work around a spec issue, which could be a significant regression.

Medium
General
Guard against null type info

Add a null check for the jsonTypeInfo parameter in SubscribeAsync to prevent
potential exceptions.

dotnet/src/webdriver/BiDi/Broker.cs [165]

+if (jsonTypeInfo is null)
+    throw new ArgumentNullException(nameof(jsonTypeInfo), "Event JSON type info cannot be null.");
 _eventTypesMap[eventName] = jsonTypeInfo;
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out a missing null check for the jsonTypeInfo parameter, which improves the method's robustness by preventing potential null reference exceptions.

Medium
Learned
best practice
Centralize repeated handler logic

Replace the many per-event HandleXxx methods with 1–2 generic helpers that apply
the context filter and invoke the provided handler to keep behavior
single-sourced.

dotnet/src/webdriver/BiDi/BrowsingContext/BrowsingContext.cs [309-339]

-private async Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler)
+private Task InvokeIfThisContextAsync<T>(T e, Func<T, Task> handler, Func<T, BrowsingContext> getContext)
 {
-    if (Equals(e.Context))
-    {
-        await handler(e).ConfigureAwait(false);
-    }
+    return Equals(getContext(e)) ? handler(e) : Task.CompletedTask;
 }
 
-private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler)
+private void InvokeIfThisContext<T>(T e, Action<T> handler, Func<T, BrowsingContext> getContext)
 {
-    if (Equals(e.Context))
+    if (Equals(getContext(e)))
     {
         handler(e);
     }
 }
 
-private async Task HandleFragmentNavigatedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler)
-{
-    if (Equals(e.Context))
-    {
-        await handler(e).ConfigureAwait(false);
-    }
-}
+private Task HandleNavigationStartedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) =>
+    InvokeIfThisContextAsync(e, handler, x => x.Context);
 
-private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler)
-{
-    if (Equals(e.Context))
-    {
-        handler(e);
-    }
-}
+private void HandleNavigationStarted(NavigationInfo e, Action<NavigationInfo> handler) =>
+    InvokeIfThisContext(e, handler, x => x.Context);
 
+private Task HandleFragmentNavigatedAsync(NavigationInfo e, Func<NavigationInfo, Task> handler) =>
+    InvokeIfThisContextAsync(e, handler, x => x.Context);
+
+private void HandleFragmentNavigated(NavigationInfo e, Action<NavigationInfo> handler) =>
+    InvokeIfThisContext(e, handler, x => x.Context);
+
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Reduce duplication by centralizing shared behavior (use helpers for repeated context-filter + invoke logic instead of many near-identical methods).

Low
  • More