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] Get tree from browsing context as root #14495

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Sep 13, 2024

User description

Description

New method

var tree = await context.GetTreeAsync();

which is equivalent to:

var tree = await bidi.GetBrowsingContextTreeAsync(new() { Root = context });

Motivation and Context

One more method forwarding options implicitly depending on current context.

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

  • Introduced a new method GetTreeAsync in the BrowsingContext class to retrieve the browsing context tree with the current context set as the root by default.
  • Updated intercept and preload script methods to use specific options classes (BrowsingContextAddInterceptOptions and BrowsingContextAddPreloadScriptOptions) to ensure context is set correctly.
  • Added new options classes (BrowsingContextGetTreeOptions, BrowsingContextAddInterceptOptions, BrowsingContextAddPreloadScriptOptions) with internal properties to encapsulate context-specific configurations.

Changes walkthrough 📝

Relevant files
Enhancement
BrowsingContext.cs
Add `GetTreeAsync` method to BrowsingContext class             

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

  • Added a new method GetTreeAsync to retrieve the browsing context tree.
  • The method sets the current context as the root by default.
  • +9/-0     
    BrowsingContextNetworkModule.cs
    Update intercept methods to use specific options class     

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

  • Updated method signatures to use BrowsingContextAddInterceptOptions.
  • Ensured context is set in intercept options.
  • +4/-4     
    BrowsingContextScriptModule.cs
    Update preload script method to use specific options class

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

  • Updated AddPreloadScriptAsync method to use
    BrowsingContextAddPreloadScriptOptions.
  • Ensured context is set in preload script options.
  • +1/-1     
    GetTreeCommand.cs
    Introduce BrowsingContextGetTreeOptions for tree command 

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

  • Added BrowsingContextGetTreeOptions class inheriting from
    GetTreeOptions.
  • Overridden Root property to be internal.
  • +9/-0     
    AddInterceptCommand.cs
    Introduce BrowsingContextAddInterceptOptions for intercept command

    dotnet/src/webdriver/BiDi/Modules/Network/AddInterceptCommand.cs

  • Added BrowsingContextAddInterceptOptions class inheriting from
    AddInterceptOptions.
  • Overridden Contexts property to be internal.
  • +9/-0     
    AddPreloadScriptCommand.cs
    Introduce BrowsingContextAddPreloadScriptOptions for preload script
    command

    dotnet/src/webdriver/BiDi/Modules/Script/AddPreloadScriptCommand.cs

  • Added BrowsingContextAddPreloadScriptOptions class inheriting from
    AddPreloadScriptOptions.
  • Overridden Contexts property to be internal.
  • +9/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Null Reference
    The GetTreeAsync method uses the null-coalescing operator to create a new BrowsingContextGetTreeOptions if options is null, but then immediately accesses options.Root without checking for null. This could potentially lead to a null reference exception if BrowsingContextGetTreeOptions is implemented to allow Root to be null.

    Inconsistent Naming
    The method parameters use BrowsingContextAddInterceptOptions, but the local variable is still named interceptOptions. Consider renaming the local variable to match the new type name for consistency.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add null checks to prevent potential null reference exceptions

    Consider adding null checks for _bidi and _bidi.BrowsingContextModule to prevent
    potential null reference exceptions.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContext.cs [103-110]

     public Task<IReadOnlyList<BrowsingContextInfo>> GetTreeAsync(BrowsingContextGetTreeOptions? options = null)
     {
         options ??= new();
     
         options.Root = this;
     
    +    if (_bidi?.BrowsingContextModule == null)
    +    {
    +        throw new InvalidOperationException("BrowsingContextModule is not initialized.");
    +    }
    +
         return _bidi.BrowsingContextModule.GetTreeAsync(options);
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding null checks for _bidi and _bidi.BrowsingContextModule is a crucial improvement to prevent potential null reference exceptions, which could lead to runtime errors. This suggestion enhances the robustness of the code.

    9
    Best practice
    Improve encapsulation by making the property setter private and providing an internal method for setting the value

    Consider making the Root property in BrowsingContextGetTreeOptions private set
    instead of internal to improve encapsulation.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/GetTreeCommand.cs [22-29]

     public record BrowsingContextGetTreeOptions : GetTreeOptions
     {
    -    internal new BrowsingContext? Root
    +    public new BrowsingContext? Root
         {
             get => base.Root;
    -        set => base.Root = value;
    +        private set => base.Root = value;
         }
    +
    +    internal void SetRoot(BrowsingContext? root) => Root = root;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Making the Root property setter private and providing an internal method for setting the value improves encapsulation, which is a best practice for maintaining control over how properties are modified.

    8
    Enhancement
    Use null-coalescing assignment operator for simplification and to prevent overwriting existing values

    Consider using the null-coalescing assignment operator ??= for
    interceptOptions.Contexts to simplify the code and ensure it's not overwritten if
    already set.

    dotnet/src/webdriver/BiDi/Modules/BrowsingContext/BrowsingContextNetworkModule.cs [9-13]

     public async Task<Intercept> InterceptRequestAsync(Func<BeforeRequestSentEventArgs, Task> handler, BrowsingContextAddInterceptOptions? interceptOptions = null, SubscriptionOptions? options = null)
     {
         interceptOptions ??= new();
     
    -    interceptOptions.Contexts = [context];
    +    interceptOptions.Contexts ??= [context];
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to use the null-coalescing assignment operator simplifies the code and ensures that existing values are not overwritten, improving code readability and maintainability.

    7

    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