SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.74k stars 8.02k forks source link

[java] Revert workaround for old netty http client (addendum to #12843) #14134

Open kool79 opened 2 weeks ago

kool79 commented 2 weeks ago

User description

Workaround was made as part of the fix (3f7f57cf) the bug #11750 which was specific for netty client only. Since #12843 the netty client was removed.


PR Type

enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Cleanup
ChromiumOptions.java
Remove deprecated workaround and fix Javadoc typo               

java/src/org/openqa/selenium/chromium/ChromiumOptions.java
  • Removed workaround code related to --remote-allow-origins argument.
  • Corrected typo in Javadoc comment.
  • +1/-13   

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

    codiumai-pr-agent-pro[bot] commented 2 weeks ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review [1-5] 2
    ๐Ÿงช Relevant tests No
    ๐Ÿ”’ Security concerns No
    โšก Key issues to review Possible Bug:
    The removal of the workaround for "--remote-allow-origins" assumes that all users will be on Chrome 111 or later. This might not be the case, and could lead to unexpected behavior if older versions of Chrome are used.
    codiumai-pr-agent-pro[bot] commented 2 weeks ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a null check for the arguments parameter to prevent potential NullPointerException ___ **Consider adding a null check for the arguments parameter to prevent potential
    NullPointerException when addArguments is called with a null value.** [java/src/org/openqa/selenium/chromium/ChromiumOptions.java [128-131]](https://github.com/SeleniumHQ/selenium/pull/14134/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR128-R131) ```diff public T addArguments(List arguments) { + if (arguments == null) { + throw new IllegalArgumentException("Arguments list cannot be null"); + } args.addAll(arguments); return (T) this; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Adding a null check is crucial to prevent runtime exceptions and ensure robustness of the method.
    8
    Add a check to ensure that the arguments list does not contain any null elements ___ **Consider adding a check to ensure that the arguments list does not contain any null
    elements to prevent potential issues when processing the arguments.** [java/src/org/openqa/selenium/chromium/ChromiumOptions.java [128-131]](https://github.com/SeleniumHQ/selenium/pull/14134/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR128-R131) ```diff public T addArguments(List arguments) { + Objects.requireNonNull(arguments, "Arguments list cannot be null"); + if (arguments.contains(null)) { + throw new IllegalArgumentException("Arguments list cannot contain null elements"); + } args.addAll(arguments); return (T) this; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Ensuring no null elements in the list can prevent runtime issues during the processing of arguments, enhancing method reliability.
    7
    Best practice
    Use Objects.requireNonNull for the null check on the arguments parameter to improve readability ___ **To improve readability and maintainability, consider using Objects.requireNonNull for the
    null check on the arguments parameter.** [java/src/org/openqa/selenium/chromium/ChromiumOptions.java [128-131]](https://github.com/SeleniumHQ/selenium/pull/14134/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR128-R131) ```diff public T addArguments(List arguments) { - if (arguments == null) { - throw new IllegalArgumentException("Arguments list cannot be null"); - } + Objects.requireNonNull(arguments, "Arguments list cannot be null"); args.addAll(arguments); return (T) this; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Using `Objects.requireNonNull` enhances code readability and maintainability, aligning with Java best practices.
    7
    Performance
    Automatically filter out any null elements from the arguments list to enhance performance ___ **To enhance performance, consider using
    args.addAll(arguments.stream().filter(Objects::nonNull).collect(Collectors.toList())) to
    automatically filter out any null elements from the arguments list.** [java/src/org/openqa/selenium/chromium/ChromiumOptions.java [128-131]](https://github.com/SeleniumHQ/selenium/pull/14134/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR128-R131) ```diff public T addArguments(List arguments) { - args.addAll(arguments); + Objects.requireNonNull(arguments, "Arguments list cannot be null"); + args.addAll(arguments.stream().filter(Objects::nonNull).collect(Collectors.toList())); return (T) this; } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: While filtering nulls enhances data integrity, the performance gain might be minimal unless the list is large or operations on it are frequent.
    6