[jdk8u-dev] RFR: 8339560: Unaddressed comments during code review of JDK-8337664 [v2]

[Severin Gehwolf]

On Tue, 25 Feb 2025 15:41:48 GMT, Francisco Ferrari Bihurriet <fferrari at openjdk.org> wrote:

> It looks good to me, I've only made a few completely minor comments.

Thank you for the review!

> jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java line 39:
> 
>> 37: 
>> 38: public final class Distrust {
>> 39: 
> 
> Nit: if we want to maximize the similarity with the `jdk11u-dev` version, we could remove this blank line (`Distrust.java:39`).
> 
> `Distrust.java` diff against `jdk11u-dev` (before openjdk/jdk11u-dev at 8322c66efa9da9210eca7d6081d2a8c2d65ba4e0):
> 
> diff --git a/jdk8u-dev at e8c86cc/jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java b/jdk11u-dev at 7dab5fe/test/jdk/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
> index eb8abec..18178f6 100644
> --- a/jdk8u-dev at e8c86cc/jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
> +++ b/jdk11u-dev at 7dab5fe/test/jdk/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java
> @@ -31,10 +31,11 @@ import javax.net.ssl.*;
>  import sun.security.validator.Validator;
>  import sun.security.validator.ValidatorException;
>  
> +import jdk.test.lib.security.SecurityUtils;
> +
>  /**
>   * Helper class that provides methods to facilitate testing of distrusted roots.
>   */
> -
>  public final class Distrust {
>  
>      private static final String TEST_SRC = System.getProperty("test.src", ".");
> @@ -76,6 +77,7 @@ public final class Distrust {
>          for (String test : tests) {
>              System.err.println("Testing " + test);
>              X509Certificate[] chain = loadCertificateChain(certPath, test);
> +
>              for (X509TrustManager tm : tms) {
>                  testTM(tm, chain, notBefore, isValid);
>              }

Since `SecurityUtils` is in no package in JDK 8u. I'll leave this alone (as the import would be different anyway).

> jdk/test/sun/security/ssl/X509TrustManagerImpl/distrust/Distrust.java line 78:
> 
>> 76:         for (String test : tests) {
>> 77:             System.err.println("Testing " + test);
>> 78:             X509Certificate[] chain = loadCertificateChain(certPath, test);
> 
> Nit: if we want to maximize the similarity with the `jdk11u-dev` version, we could add a blank line after this one (`Distrust.java:78`).
> 
> See `Distrust.java` diff against `jdk11u-dev` (before openjdk/jdk11u-dev at 8322c66efa9da9210eca7d6081d2a8c2d65ba4e0) in the previous comment.

OK. Added.

-------------

PR Comment: https://git.openjdk.org/jdk8u-dev/pull/626#issuecomment-2682650987
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/626#discussion_r1970180874
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/626#discussion_r1970178807