On Thu, 14 Nov 2024 20:53:02 GMT, Michael McMahon <micha...@openjdk.org> wrote:
>> Please find here a patch that cleans up the java.net.http module code to
>> remove permission checks and doPriviliged calls.
>> This was mostly mechanical.
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientBuilderImpl.java
> line 49:
>
>> 47: HttpClient.Version version;
>> 48: Executor executor;
>> 49: SSLContext sslContext;
>
> I think this line was referring to the SSLContext and SSLParameters rather
> than the security manager
Yes - I toyed with replacing `Security` with `SSL`, but decided to remove it
instead because there's other field declarations following that have nothing to
do with SSL, and this comment doesn't explain anything: it's obvious that
SSLContext and SSLParameters are for SSL... Plus, it's the only comment in the
whole field declaration list...
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java
> line 323:
>
>> 321: private final DelegatingExecutor delegatingExecutor;
>> 322: private final boolean isDefaultExecutor;
>> 323: private final SSLContext sslContext;
>
> Same here. I guess it's okay to remove them either way
Yes - same rationale for simply removing it.
> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java
> line 245:
>
>> 243:
>> 244: try {
>> 245: pathForDefaultFSCheck(path);
>
> Is this intended, calling the method but ignoring the return value? If so, a
> comment might be in order.
The method might throw UOE if the path doesn't come from the default file
system. I didn't want to investigate and change that logic now - I wasn't sure
I fully understood why we have special cases here, so decided to keep it.
> src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java
> line 55:
>
>> 53: private ResponseBodyHandlers() { }
>> 54:
>> 55: private static final String pathForDefaultFSCheck(Path path) {
>
> maybe this method and the equivalent in the previous file could be renamed
> `checkPathForDefaultFS` which would suggest an exception being thrown in
> error cases.
Good idea for the renaming. I will change that - and above too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843419538
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843421547
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843424520
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843425880
