Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-28 Thread Mark Powers
On Wed, 27 Apr 2022 20:22:42 GMT, Mark Powers  wrote:

>> JDK-6725221 is about obtaining boolean properties, so not an exact match. 
>> The suggested change is so easy, I'm going to do it.
>
> sun.security.action.GetPropertyAction::privilegedGetProperty doesn't trim the 
> return value. Could this be a problem?

Answer is no.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Mark Powers
On Wed, 27 Apr 2022 19:12:37 GMT, Mark Powers  wrote:

>> No problem.
>
> JDK-6725221 is about obtaining boolean properties, so not an exact match. The 
> suggested change is so easy, I'm going to do it.

sun.security.action.GetPropertyAction::privilegedGetProperty doesn't trim the 
return value. Could this be a problem?

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Mark Powers
On Tue, 26 Apr 2022 22:08:42 GMT, Weijun Wang  wrote:

>> Perhaps as part of 
>> [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)?
>
> No problem.

JDK-6725221 is about obtaining boolean properties, so not an exact match. The 
suggested change is so easy, I'm going to do it.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Mark Powers
On Tue, 26 Apr 2022 19:09:46 GMT, Weijun Wang  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 81:
> 
>> 79: String type;
>> 80: type = AccessController.doPrivileged((PrivilegedAction) 
>> () ->
>> 81: Security.getProperty( "ssl.TrustManagerFactory.algorithm"));
> 
> Another `GetPropertyAction::privilegedGetProperty` candidate.

Making the change.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Mark Powers
On Tue, 26 Apr 2022 19:08:50 GMT, Weijun Wang  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 96:
> 
>> 94: s = s.trim();
>> 95: if (s.isEmpty()) {
>> 96: s = null;
> 
> According to the implementation of `Security::getProperty`. The return value 
> is already trimmed.

Removed those lines.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Bradford Wetmore
On Wed, 27 Apr 2022 15:22:08 GMT, Mark Powers  wrote:

>> src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 
>> 37:
>> 
>>> 35:  * {@link SSLSession#putValue(String, Object)}
>>> 36:  * or {@link SSLSession#removeValue(String)}, objects which
>>> 37:  * implement the SSLSessionBindingListener will receive an
>> 
>> If you're up for it, you could fix the missing ``  or `@link` 
>> throughout this small file.  Ignore otherwise.
>
> I'll ignore for now. Javadoc issues are already being tracked for 
> javax.crypto with JDK-8284851. This bug could easily be expanded to include 
> javax.net.

Ok.  I'm going to do a little surgery in a java.security this morning, but 
JDK-8284851 could probably be expanded to address both the JCA/JCE code.  
(java/security, javax/crypto).

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-27 Thread Mark Powers
On Tue, 26 Apr 2022 18:46:02 GMT, Bradford Wetmore  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 37:
> 
>> 35:  * {@link SSLSession#putValue(String, Object)}
>> 36:  * or {@link SSLSession#removeValue(String)}, objects which
>> 37:  * implement the SSLSessionBindingListener will receive an
> 
> If you're up for it, you could fix the missing ``  or `@link` 
> throughout this small file.  Ignore otherwise.

I'll ignore for now. Javadoc issues are already being tracked for javax.crypto 
with JDK-8284851. This bug could easily be expanded to include javax.net.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Weijun Wang
On Tue, 26 Apr 2022 20:40:50 GMT, Bradford Wetmore  wrote:

>> Wasn't there another bug to address this?
>
> Perhaps as part of 
> [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)?

No problem.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Bradford Wetmore
On Tue, 26 Apr 2022 19:49:11 GMT, Bradford Wetmore  wrote:

>> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:
>> 
>>> 67: String type;
>>> 68: type = AccessController.doPrivileged((PrivilegedAction) 
>>> () ->
>>> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));
>> 
>> We can probably use 
>> `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is 
>> inside `java.base` so that class is always available.
>
> Wasn't there another bug to address this?

Perhaps as part of 
[JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)?

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Bradford Wetmore
On Tue, 26 Apr 2022 19:05:05 GMT, Weijun Wang  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:
> 
>> 67: String type;
>> 68: type = AccessController.doPrivileged((PrivilegedAction) 
>> () ->
>> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));
> 
> We can probably use 
> `sun.security.action.GetPropertyAction::privilegedGetProperty`. This is 
> inside `java.base` so that class is always available.

Wasn't there another bug to address this?

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Mark Powers
On Tue, 26 Apr 2022 18:39:33 GMT, Bradford Wetmore  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 
> 72:
> 
>> 70: 
>> 71: this.parameters = List.copyOf(parameters);
>> 72: }
> 
> If you leave this as is, you can use `<>`

Using <>.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Weijun Wang
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

Looks fine. Some small comments.

src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:

> 67: String type;
> 68: type = AccessController.doPrivileged((PrivilegedAction) 
> () ->
> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));

We can probably use 
`sun.security.action.GetPropertyAction::privilegedGetProperty`. This is inside 
`java.base` so that class is always available.

src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 96:

> 94: s = s.trim();
> 95: if (s.isEmpty()) {
> 96: s = null;

According to the implementation of `Security::getProperty`. The return value is 
already trimmed.

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 81:

> 79: String type;
> 80: type = AccessController.doPrivileged((PrivilegedAction) 
> () ->
> 81: Security.getProperty( "ssl.TrustManagerFactory.algorithm"));

Another `GetPropertyAction::privilegedGetProperty` candidate.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Bradford Wetmore
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

Other than the comments mentioned, LGTM.

src/java.base/share/classes/javax/net/ssl/SSLSessionBindingEvent.java line 37:

> 35:  * {@link SSLSession#putValue(String, Object)}
> 36:  * or {@link SSLSession#removeValue(String)}, objects which
> 37:  * implement the SSLSessionBindingListener will receive an

If you're up for it, you could fix the missing ``  or `@link` 
throughout this small file.  Ignore otherwise.

-

Marked as reviewed by wetmore (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Bradford Wetmore
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 
72:

> 70: 
> 71: this.parameters = List.copyOf(parameters);
> 72: }

If you leave this as is, you can use `<>`

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-26 Thread Mark Powers
On Tue, 26 Apr 2022 04:37:58 GMT, Jaikiran Pai  wrote:

>> Mark Powers has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Alan Bateman comments
>
> src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 
> 71:
> 
>> 69: }
>> 70: 
>> 71: this.parameters = List.copyOf(parameters);
> 
> Hello Mark, this would actually be a change in behaviour. The `List.copyOf` 
> says:
> 
>> The given Collection must not be null and it must not contain any null 
>> elements.
> 
> The current implementation of the public constructor on the public 
> `KeyStoreBuilderParameters` mandates no such requirement. So if there's some 
> code which currently passes a list with a null element in it, then this 
> change will now end up throwing a `NullPointerException` as per the contract 
> of `List.copyOf`.

You are correct. This is not a good change since it changes behavior. Going 
back to the original. Thanks for your review!

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-25 Thread Jaikiran Pai
On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

src/java.base/share/classes/javax/net/ssl/KeyStoreBuilderParameters.java line 
71:

> 69: }
> 70: 
> 71: this.parameters = List.copyOf(parameters);

Hello Mark, this would actually be a change in behaviour. The `List.copyOf` 
says:

> The given Collection must not be null and it must not contain any null 
> elements.

The current implementation of the public constructor on the public 
`KeyStoreBuilderParameters` mandates no such requirement. So if there's some 
code which currently passes a list with a null element in it, then this change 
will now end up throwing a `NullPointerException` as per the contract of 
`List.copyOf`.

-

PR: https://git.openjdk.java.net/jdk/pull/8384


Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

2022-04-25 Thread Mark Powers
> https://bugs.openjdk.java.net/browse/JDK-8285504
> 
> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
> a single code review, so it was decided to split into smaller chunks. This is 
> one such chunk: 
> 
> open/src/java.base/share/classes/java/net

Mark Powers has updated the pull request incrementally with one additional 
commit since the last revision:

  Alan Bateman comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8384/files
  - new: https://git.openjdk.java.net/jdk/pull/8384/files/5ac7f1ec..d964c05a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8384=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8384=00-01

  Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8384.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8384/head:pull/8384

PR: https://git.openjdk.java.net/jdk/pull/8384