Skip to content

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

Merged
diemol merged 5 commits intoSeleniumHQ:trunkfrom
kool79:cleanup_deprecated_code
Jul 11, 2024
Merged

[java] Revert workaround for old netty http client (addendum to #12843)#14134
diemol merged 5 commits intoSeleniumHQ:trunkfrom
kool79:cleanup_deprecated_code

Conversation

@kool79
Copy link
Contributor

@kool79 kool79 commented Jun 13, 2024 •
edited by qodo-code-review bot
Loading

User description

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

PR Type

enhancement

Description

  • Removed deprecated workaround code for --remote-allow-origins argument in ChromiumOptions.java.
  • Corrected a typo in the Javadoc comment for the addArguments method.

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

    @kool79
    [java] Revert workaround for old netty http client (addendum to Selen…
    49b0344 
    …iumHQ#12843)

Workaround was made as part of the fix (3f7f57c) the bug SeleniumHQ#11750 which was specific for netty client only.
Since SeleniumHQ#12843 the netty client was removed.
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 13, 2024

    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.

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Jun 13, 2024

    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]

     public T addArguments(List<String> 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]

     public T addArguments(List<String> 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]

     public T addArguments(List<String> 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]

     public T addArguments(List<String> 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

    @titusfortner titusfortner added the C-java Java Bindings label Jun 14, 2024
    diemol
    diemol approved these changes Jul 11, 2024
    View reviewed changes
    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    This makes sense, thank you!

    @diemol diemol merged commit 12769d6 into SeleniumHQ:trunk Jul 11, 2024
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @kool79 @diemol @sandeepsuryaprasad
    [java] Revert workaround for old netty http client (addendum to Selen…
    45f3af2 
    …iumHQ#12843) (SeleniumHQ#14134)

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

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

* Running format script

---------

Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
Co-authored-by: Diego Molina <diemol@gmail.com>
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Nov 8, 2024
    Open
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @kool79 @diemol @M1troll
    [java] Revert workaround for old netty http client (addendum to Selen…
    4d6a11b 
    …iumHQ#12843) (SeleniumHQ#14134)

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

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

* Running format script

---------

Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
Co-authored-by: Diego Molina <diemol@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    C-java Java Bindings P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @kool79 @diemol @titusfortner