[rfc][icedtea-web] Fix regression in recent PR1204 patch

[Andrew Azores]

On 10/01/2013 12:01 PM, Omair Majid wrote:
> On 10/01/2013 11:45 AM, Andrew Azores wrote:
>> I just caught a regression in my own patch. The "path" portion of the
>> URI was already percent-encoded seeing as it was obtained with
>> URL#getPath(). This was then fed back into the constructor of a new URI,
>> and the .toURL() result of this URI was returned. This process repeated
>> the percent-encoding, so eg spaces would become "%2520" rather than
>> simply "%20".
> Could you include a unit test? Otherwise looks good to me.
>
> Cheers,
> Omair
>

Sure, added three new tests to ensure correct percent-encoding 
behaviour. The SpacesAreEverywhere(Signed) reproducer(s) also catch this 
regression, which is how I noticed it to begin with. I also went ahead 
and refactored the existing unit tests a bit.

I had to add some simple input validation to the getVersionedUrl call. I 
realized that the older version of this method was not actually dealing 
with percent encoding on its own - if you passed it an invalid URL 
containing a space, it would happily accept this, work with it, and 
return to you another invalid URL containing the same spaces. This seems 
incorrect at first but I didn't want to go about changing the behaviour 
of this method. If the method started handling percent-encoding, then it 
could be passing back up URLs that actually do point to valid resources, 
when in fact that isn't *really* the same URL it was asked to work with. 
Rather than causing an explicit failure on invalid input, I kept the 
behaviour the same, so that the invalid URL is simply returned, and the 
same later point of failure should still occur as it does now. Of the 
two new test cases, two of them contain spaces. The return value of the 
method on these inputs is simply the same URL that was given in the 
first place, as this is what the method used to do. The other new test 
case ensures that a properly-encoded URL is not mistakenly re-encoded.

On another note, I have a question about this method now that I'm 
looking back into it again. It's only called from two places in the 
entire codebase - one place is these unit tests. The other place is 
ResourceUrlCreator#getUrls(). getUrls() and getVersionedUrl() are both 
instance methods of the class. When getUrls calls getVersionedUrl, it 
passes it a Resource reference - but this Resource is also a field of 
the instance. So, why does getVersionedUrl take a parameter at all if 
the same reference is already available to it? Or, on the other hand, 
getVersionedUrl doesn't care at all about its containing instance - the 
only information it needs to perform its duty is a Resource reference. 
So then getVersionedUrl can be made static and passed the Resource 
reference. I don't understand that there's any reason for it to be in 
this in-between state. Anyone seeing something I'm not?

Thanks,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR1204_regression.patch
Type: text/x-patch
Size: 10673 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20131001/4df84ff6/PR1204_regression.patch