Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 23, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This improves the story for trimming/AOT using Selenium, specifically addressing non-JSON related warnings.

Motivation and Context

In furtherance of #14480

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

  • Enhanced the DevToolsDomains class to support AOT compatibility by introducing a DomainType struct and adding DynamicallyAccessedMembers attributes.
  • Improved the JsonEnumMemberConverter to handle AOT scenarios with conditional compilation and dynamic access attributes.
  • Added a new file TrimmingAttributes.cs containing custom attributes to manage dynamic and unreferenced code for AOT compatibility.
  • Updated the WebDriver.csproj file to include a conditional property for AOT compatibility, preparing for future .NET versions.

Changes walkthrough 📝

Relevant files
Enhancement
DevToolsDomains.cs
Enhance DevToolsDomains for AOT compatibility and trimming

dotnet/src/webdriver/DevTools/DevToolsDomains.cs

  • Introduced DomainType struct to handle trimming.
  • Added DynamicallyAccessedMembers attribute for trimming support.
  • Modified dictionary to use DomainType instead of Type.
  • +19/-8   
    JsonEnumMemberConverter.cs
    Improve JsonEnumMemberConverter for AOT compatibility       

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs

  • Added DynamicallyAccessedMembers attribute for enum converter.
  • Used conditional compilation for .NET version-specific code.
  • +9/-3     
    TrimmingAttributes.cs
    Introduce custom trimming attributes for AOT support         

    dotnet/src/webdriver/Internal/TrimmingAttributes.cs

  • Added custom attributes for trimming support.
  • Implemented attributes for dynamic code and unreferenced code
    handling.
  • +420/-0 
    Configuration changes
    WebDriver.csproj
    Prepare WebDriver project for AOT compatibility                   

    dotnet/src/webdriver/WebDriver.csproj

    • Added conditional AOT compatibility property for future use.
    +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @CLAassistant
    Copy link

    CLAassistant commented Oct 23, 2024

    CLA assistant check
    Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
    1 out of 2 committers have signed the CLA.

    ✅ RenderMichael
    ❌ Michael Render


    Michael Render seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
    You have signed the CLA already but the status is still pending? Let us recheck it.

    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    14480 - Partially compliant

    Fully compliant requirements:

    • Address non-JSON related warnings for trimming/AOT

    Not compliant requirements:

    • Make Selenium library in .NET fully compatible with Native AOT Deployment
    • Improve AOT compatibility for Selenium Manager, W3C WebDriver, CDP, W3C BiDi
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Impact
    The introduction of the DomainType struct and the use of reflection might have a slight performance impact. Verify if this approach is the most efficient for AOT compatibility.

    Conditional Compilation
    The use of conditional compilation for different .NET versions may lead to maintenance challenges. Ensure that all code paths are properly tested.

    Code Duplication
    The file contains multiple attribute definitions that might already exist in newer .NET versions. Consider using compiler directives to avoid duplication in future .NET versions.

    Commented Code
    There is commented-out code for AOT compatibility. Ensure this is addressed in future updates and doesn't remain as dead code.

    { 85, new DomainType(typeof(V85.V85Domains)) },
    };

    /// <summary>Workaround for trimming.</summary>
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This is the kind of workaround necessary when using a Type as a generic argument, which requires trimming annotations. Hope this is acceptable.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Oct 23, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a readonly dictionary to improve thread safety and prevent accidental modifications

    Consider using a readonly dictionary for SupportedDevToolsVersions to improve thread
    safety and prevent accidental modifications.

    dotnet/src/webdriver/DevTools/DevToolsDomains.cs [37-43]

    -private static readonly Dictionary<int, DomainType> SupportedDevToolsVersions = new Dictionary<int, DomainType>()
    +private static readonly IReadOnlyDictionary<int, DomainType> SupportedDevToolsVersions = new Dictionary<int, DomainType>()
     {
         { 127, new DomainType(typeof(V127.V127Domains)) },
         { 129, new DomainType(typeof(V129.V129Domains) )},
         { 128, new DomainType(typeof(V128.V128Domains) )},
         { 85, new DomainType(typeof(V85.V85Domains)) },
     };
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Changing the dictionary to IReadOnlyDictionary enhances thread safety and prevents accidental modifications, which is a significant improvement for maintaining the integrity of the data structure.

    8
    Enhancement
    Use pattern matching for more concise and readable attribute retrieval

    Consider using pattern matching with is keyword for more concise and readable code
    when checking for EnumMemberAttribute.

    dotnet/src/webdriver/DevTools/Json/JsonEnumMemberConverter.cs [29-31]

     var attr = enumMember.GetCustomAttributes(typeof(EnumMemberAttribute), false)
    -  .Cast<EnumMemberAttribute>()
    -  .FirstOrDefault();
    +  .FirstOrDefault() as EnumMemberAttribute;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using pattern matching with 'is' for attribute retrieval simplifies the code, making it more readable and concise without altering functionality, which is a beneficial enhancement.

    7
    Maintainability
    Improve clarity of commented-out code by providing a clear TODO comment

    Consider removing the commented-out PropertyGroup for AOT compatibility if it's not
    needed, or add a TODO comment explaining why it's commented out if it's intended for
    future use.

    dotnet/src/webdriver/WebDriver.csproj [45-48]

    -<!--TODO when AOT is ready https://github.com/SeleniumHQ/selenium/issues/14480-->
    -<!--<PropertyGroup Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net8.0'))">
    -  <IsAotCompatible>true</IsAotCompatible>
    -</PropertyGroup>-->
    +<!-- TODO: Enable AOT compatibility when ready. See https://github.com/SeleniumHQ/selenium/issues/14480 -->
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a clear TODO comment enhances maintainability by providing context for the commented-out code, helping future developers understand its purpose and intended use.

    6

    💡 Need additional feedback ? start a PR chat

    @RenderMichael RenderMichael deleted the trimming-improvement branch October 23, 2024 05:55
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants