Hi,

Please find the updated webrev(http://cr.openjdk.java.net/~vtewari/8114860/webrev0.1/index.html <http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.1/index.html>). I incorporated the review comments and remove the "HashCodeTest" where it compares the hashcode with hard coded hashcode.

Thanks,
Vyom


On Monday 20 June 2016 03:57 PM, Pavel Rappo wrote:
Hi Vyom, thanks for looking into this!

0.  * Copyright (c) 2013,2016 Oracle and/or its affiliates. All rights reserved.

If I understand correctly, we're not required to update years in the header
manually. But if you decided to do so, please follow the established pattern (in
both files you've changed):

     * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights 
reserved.
                          ^    ^

1. I wonder if we could use String convenience methods instead of bringing
dependency on java.util.stream.Collector. Could something like this be a bit
better both in terms of dependencies and readability?

     private String actions() {
         String b = String.join(",", methods);
         if (!requestHeaders.isEmpty()) {
             b = ":" + String.join(",", requestHeaders);
         }
         return b;
     }

2. Is there a particular reason for these parentheses?

     150             return (expectedActions.equals(urlp.getActions()));
                            ^                                         ^

3. Why are these lines commented out?

     259     static Test[] hashTests = {
     260         //hashtest("http://www.foo.com/path";, "GET:X-Foo", 388644203),
     261         //hashtest("http:*", "*:*", 3255810)
     262     };
IMO they either have to be updates or removed altogether.

Thanks,
-Pavel

On 20 Jun 2016, at 08:44, Vyom Tewari <vyom.tew...@oracle.com> wrote:

ping!

On Saturday 11 June 2016 10:34 AM, vyom wrote:
Hi All,

Please review the below fix.

Bug               : JDK-8114860 Behavior of java.net.URLPermission.getActions() 
contradicts spec
Webrev         : http://cr.openjdk.java.net/~vtewari/8114860/webrev0.0/index.html 
<http://cr.openjdk.java.net/%7Evtewari/8114860/webrev0.0/index.html>

Thanks,
Vyom

Reply via email to