> On 18 Jan 2016, at 16:20, Claes Redestad <claes.redes...@oracle.com> wrote:
>
> On 2016-01-18 15:34, Chris Hegarty wrote:
>> On 18/01/16 14:09, Alan Bateman wrote:
>>>
>>> On 17/01/2016 15:30, Claes Redestad wrote:
>>>> Hi,
>>>>
>>>> please review this patch which might make URI.toURL more efficient
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8147462
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8147462
>>>>
>>>> This patch exploits the fact that URI will apply the same validation
>>>> to the URI/URL specification for any valid non-opaque URL, thus it's
>>>> safe to use the component-based URL constructor. Also, URIs
>>>> representing malformed URLs will throw an exception as specified both
>>>> before and after. A number of simple test cases to capture and
>>>> document this was added to the existing java/net/URI/URItoURL jtreg test.
>>> I think the change to URI looks okay but it would be good to include a
>>> brief comment as otherwise it will be difficult for future maintainers
>>> to understand.
>>
>> +1. For a long time we have been suggesting that anyone requiring
>> a URL should retrieve it from URI::toURL, so it is good to have a
>> fast(er) common path.
>>
>> The handling of a null/empty host is a little esoteric, so a comment
>> would be good.
>
> How about this?
>
> http://cr.openjdk.java.net/~redestad/8147462/webrev.02
> <http://cr.openjdk.java.net/~redestad/8147462/webrev.02>
Looks good Claes.
-Chris.
>> I do have a little discomfort about this change, since the toURL spec
>> specifically says it is "equivalent to evaluating the expression new
>> URL(this.toString())". I wonder if your tests should cover this too,
>> i.e. the resulting URLs from toURL and URL(this.toString()) are equal?
>
> The test already does this. I've just added cases that exercise some
> interesting cases like URIs containing /./ and quoted characters.
>
> I restructured the exceptional tests slightly to differentiate the cases
> where both toURL and new URL throw MalformedURLException from cases where
> toURL throws IAE. It makes the test slightly more verbose but arguably a lot
> simpler.
>
> However: the javadoc spec for URL(String) seems incomplete, and I might have
> stumbled into a trap...
>
> * @exception MalformedURLException if no protocol is specified, or an
> * unknown protocol is found, or {@code spec} is {@code
> null}.
>
> Since this method delegates parsing to the URLStreamHandler associated with
> the protocol it might be overriden to throw exceptions (that will be wrapped
> in MalformedURLException) for a variety of reasons.
>
> The ability for URLStreamHandler implementations to override the parseURL
> method seem to prevent this improvement unless we only do this for a subset
> of known, well-behaved URLStreamHandlers, which likely defeat the
> optimization by adding complexity.
>
> Thanks!
>
> /Claes
