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] Add nullability to Chromium configuration types #15204

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 31, 2025

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.

Add nullability to Chromium configuration types

Motivation and Context

Contributes to #14640

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

  • Enabled nullable reference types in Chromium-related classes.

  • Simplified property declarations and removed backing fields.

  • Improved exception handling and validation logic.

  • Enhanced code readability and maintainability.


Changes walkthrough 📝

Relevant files
Enhancement
ChromiumAndroidOptions.cs
Refactored ChromiumAndroidOptions with nullable types       

dotnet/src/webdriver/Chromium/ChromiumAndroidOptions.cs

  • Enabled nullable reference types.
  • Simplified property declarations by removing backing fields.
  • Made AndroidProcess nullable.
  • +4/-13   
    ChromiumMobileEmulationDeviceSettings.cs
    Refactored ChromiumMobileEmulationDeviceSettings with nullable types

    dotnet/src/webdriver/Chromium/ChromiumMobileEmulationDeviceSettings.cs

  • Enabled nullable reference types.
  • Simplified property declarations by removing backing fields.
  • Made UserAgent nullable.
  • Set default value for EnableTouchEvents.
  • +9/-33   
    ChromiumNetworkConditions.cs
    Refactored ChromiumNetworkConditions with nullable types 

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs

  • Enabled nullable reference types.
  • Simplified property declarations by removing backing fields.
  • Improved exception handling for throughput validation.
  • Refactored dictionary parsing logic.
  • +18/-31 
    ChromiumPerformanceLoggingPreferences.cs
    Refactored ChromiumPerformanceLoggingPreferences with nullable types

    dotnet/src/webdriver/Chromium/ChromiumPerformanceLoggingPreferences.cs

  • Enabled nullable reference types.
  • Simplified property declarations by removing backing fields.
  • Improved exception handling for invalid arguments.
  • Refactored tracing categories logic.
  • +14/-33 

    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

    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 Message

    The error message for negative download throughput was fixed from "Downlod" to "Download", but the error handling for upload throughput is missing a similar error message.

    set
    {
        if (value < 0)
        {
    Constructor Issue

    The parameterized constructor is missing the initialization code block, which could lead to the UserAgent property not being set properly.

    public ChromiumMobileEmulationDeviceSettings(string? userAgent)
    {
        this.UserAgent = userAgent;
    }

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Standardize exception types for validation

    Use consistent exception types for throughput validation. Currently using
    WebDriverException for download but ArgumentOutOfRangeException for bidirectional
    throughput. Should standardize on ArgumentOutOfRangeException for all throughput
    validations.

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs [59]

    -throw new WebDriverException("Download throughput cannot be negative.");
    +throw new ArgumentOutOfRangeException(nameof(value), "Download throughput cannot be negative.");
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Important consistency improvement that aligns with .NET best practices by using ArgumentOutOfRangeException for parameter validation across the codebase.

    7
    Fix typo in error message

    Fix typo in error message for negative download throughput. The current message has
    a spelling error ("Downlod" instead of "Download").

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs [59]

    -throw new WebDriverException("Downlod throughput cannot be negative.");
    +throw new WebDriverException("Download throughput cannot be negative.");
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Fixes a minor spelling error in an error message that could affect user experience and code professionalism, though not impacting functionality.

    4
    Learned
    best practice
    Include actual values in exception messages to improve debugging experience

    The exception message for negative throughput values should include the actual
    invalid value to help with debugging. Update the validation check in
    DownloadThroughput and UploadThroughput properties to include the invalid value in
    the error message.

    dotnet/src/webdriver/Chromium/ChromiumNetworkConditions.cs [59]

    -throw new WebDriverException("Download throughput cannot be negative.");
    +throw new WebDriverException($"Download throughput cannot be negative. Provided value: {value}");
    • Apply this suggestion
    6

    @RenderMichael RenderMichael merged commit 4666186 into SeleniumHQ:trunk Jan 31, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the chromium-nullable branch January 31, 2025 18:22
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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