Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet][bidi] Add OnNavigationCommitted event #15253

Merged

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 8, 2025

User description

Motivation and Context

Support NavigationCommitted event: https://w3c.github.io/webdriver-bidi/#event-browsingContext-navigationCommitted

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement


Description

  • Added OnNavigationCommitted event to support navigation committed handling.

  • Introduced overloads for OnNavigationCommitted with Action and Func handlers.

  • Updated BrowsingContext and BrowsingContextModule to include new event subscriptions.


Changes walkthrough 📝

Relevant files
Enhancement
BrowsingContext.cs
Add OnNavigationCommittedAsync methods in BrowsingContext

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

  • Added OnNavigationCommittedAsync method with Action and Func
    overloads.
  • Introduced event subscription logic for navigation committed events.
  • Adjusted existing methods for consistency in event handling.
  • +12/-2   
    BrowsingContextModule.cs
    Add OnNavigationCommittedAsync methods in BrowsingContextModule

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

  • Added OnNavigationCommittedAsync method with Action and Func
    overloads.
  • Implemented subscription to browsingContext.navigationCommitted event.
  • +10/-0   

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    qodo-merge-pro bot commented Feb 8, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling

    The new OnNavigationCommittedAsync methods should include error handling or validation for the handler parameter to ensure it's not null

    public async Task<Subscription> OnNavigationCommittedAsync(Func<NavigationInfo, Task> handler, BrowsingContextsSubscriptionOptions? options = null)
    {
        return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options).ConfigureAwait(false);
    }
    
    public async Task<Subscription> OnNavigationCommittedAsync(Action<NavigationInfo> handler, BrowsingContextsSubscriptionOptions? options = null)
    {
        return await Broker.SubscribeAsync("browsingContext.navigationCommitted", handler, options).ConfigureAwait(false);
    }

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 8, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove redundant empty code block

    Remove the empty block between OnNavigationCommittedAsync methods as it serves
    no purpose and could cause compilation issues.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContext.cs [202-210]

     public Task<Subscription> OnNavigationCommittedAsync(Action<NavigationInfo> handler, SubscriptionOptions? options = null)
     {
         return BiDi.BrowsingContext.OnNavigationCommittedAsync(handler, new BrowsingContextsSubscriptionOptions(options) { Contexts = [this] });
     }
     
    -{
    -}
    -
     public Task<Subscription> OnNavigationCommittedAsync(Func<NavigationInfo, Task> handler, SubscriptionOptions? options = null)
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The empty block between the two OnNavigationCommittedAsync methods is a critical issue that would cause compilation errors and break the code functionality. Removing it is essential for the code to work properly.

    High
    Learned
    best practice
    Add null validation checks for required callback parameters to prevent potential null reference exceptions

    Add null validation check for the handler parameter in the new
    OnNavigationCommittedAsync methods since it's a required callback parameter. Use
    ArgumentNullException.ThrowIfNull() at the start of each method.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContext.cs [202-205]

     public Task<Subscription> OnNavigationCommittedAsync(Action<NavigationInfo> handler, SubscriptionOptions? options = null)
     {
    +    ArgumentNullException.ThrowIfNull(handler, nameof(handler));
         return BiDi.BrowsingContext.OnNavigationCommittedAsync(handler, new BrowsingContextsSubscriptionOptions(options) { Contexts = [this] });
     }
    • Apply this suggestion
    Low

    @nvborisenko nvborisenko merged commit 9f53b33 into SeleniumHQ:trunk Feb 8, 2025
    9 of 10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-bidi-navigation-committed branch February 8, 2025 13:33
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant