On Thu, 19 Jun 2025 15:36:13 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> David Beaumont has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Reworking comment and adjusting TODOs.
>
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 48:
>
>> 46:
>> 47: // ImageReader to access resources in jimage (never null).
>> 48: private static final ImageReader READER =
>> ImageReaderFactory.getImageReader();
>
> I think it simpler if you drop "(never null)".
I was just stressing that (unlike the old code) there's actually no need for a
null check.
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 50:
>
>> 48: private static final ImageReader READER =
>> ImageReaderFactory.getImageReader();
>> 49:
>> 50: // The module and resource name in the URL
>> ("jrt:/<module-name>/<resource-name>").
>
> In JEP 220 we use jrt:/[$MODULE[/$PATH]] to make it clear that the resource
> is optionally.
Well the resource is optional only it the same way that you can given any
invalid URL. Connection only works when both a module and a resource name are
present.
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 56:
>
>> 54: // about percent encoding, and it is likely that any differences
>> between how
>> 55: // module names and resource names are treated is unintentional. The
>> rules
>> 56: // about percent encoding may well be tightened up in the future.
>
> I think it would be better to drop this paragraph from the comment, it just
> begs too many questions.
Okay. This will become moot when the URL encoding is tidied up.
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 64:
>
>> 62:
>> 63: // The resource node (when connected).
>> 64: private volatile Node resource;
>
> Maybe better to rename to node, resourceNode, or imageNode here.
Done (resourceNode).
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 69:
>
>> 67: super(url);
>> 68: // TODO: Allow percent encoding in module names.
>> 69: // TODO: Consider rejecting URLs with fragments, queries or
>> authority.
>
> I'd prefer not include all the TODO messages.
These were here explicitly to be discussed in review. If you think it's now
covered by JDK-8359949, I can remove them.
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java
> line 78:
>
>> 76: // No trailing resource path. This can never "connect" or
>> return a
>> 77: // resource, but might be useful as a representation to pass
>> around.
>> 78: // The module name *can* be empty here (e.g. "jrt:/") but
>> not null.
>
> If the URL scheme from JEP 220 goes into the first comment then it will be
> clear that the resource is optionally. This will allow the comment here to be
> reduced.
Can you elaborate? I'm not quite clear what changes you're suggesting here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158674474
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158681461
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158672447
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158683837
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158677926
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158683599
