The code changes look fine, but the bigger question is whether we want to support actions without any ‘method’ being specified, like “:X-Bar”? Should the constructor throw IAE, or is that even possible now, it would require a spec clarification ( would that invalidate existing code already doing this ) ?
-Chris. > On 21 Jun 2016, at 11:16, Vyom Tewari <vyom.tew...@oracle.com> wrote: > > Hi Pavel, > > Please find the latest > webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.4/index.html > <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.4/index.html>). I had > incorporated the review comments. > > Thanks, > Vyom > > > On Monday 20 June 2016 07:27 PM, Pavel Rappo wrote: >> Vyom, please consider the following changes: >> >> 1. Append 8071660 to the lists of tests here: >> >> * @bug 8010464 8027570 8027687 8029354 8071660 >> ^ >> 2. Reformat the code the way it's in the rest of the file: >> >> from: >> >> 266 URLPermission that = (URLPermission)p; >> 267 >> 268 if(this.methods.isEmpty() && !that.methods.isEmpty()) >> 269 return false; >> 270 >> 271 if (!this.methods.isEmpty() && !this.methods.get(0).equals("*") >> && >> 272 Collections.indexOfSubList(this.methods, that.methods) >> == -1) { >> 273 return false; >> 274 } >> >> to: >> >> 266 URLPermission that = (URLPermission)p; >> 267 >> 268 if (this.methods.isEmpty() && !that.methods.isEmpty()) { >> 269 return false; >> 270 } >> 271 >> 272 if (!this.methods.isEmpty() && >> 273 !this.methods.get(0).equals("*") && >> 274 Collections.indexOfSubList(this.methods, >> 275 that.methods) == -1) { >> 276 return false; >> 277 } >> >> Other than these minor things, looks good. >> >> Thanks, >> -Pavel >> >>> On 20 Jun 2016, at 12:36, Daniel Fuchs <daniel.fu...@oracle.com> wrote: >>> >>> On 17/06/16 15:09, Vyom Tewari wrote: >>>> Hi, >>>> >>>> Please find the latest >>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html >>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.3/index.html>). I >>>> fixed the typo along with spaces issue. >>> Looks good! >>> >>> -- daniel >>> >>>> Thanks, >>>> Vyom >>>> >>>> >>>> On Friday 17 June 2016 06:34 PM, Roger Riggs wrote: >>>>> Hi Vyom, >>>>> >>>>> URLPermissionTest.java: >>>>> >>>>> line 125: it looks odd to assign the same variable twice with the >>>>> same value. >>>>> Perhaps it should have been assigning this.url2 = url2. >>>>> >>>>> line 330-332: please add a space after "," in argument lists. >>>>> >>>>> Roger >>>>> >>>>> >>>>> >>>>> >>>>> On 6/17/2016 8:42 AM, Daniel Fuchs wrote: >>>>>> On 17/06/16 13:25, Vyom Tewari wrote: >>>>>>> Hi Daniel, >>>>>>> >>>>>>> thanks for review please find the latest >>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html >>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.2/index.html>) . >>>>>>> I added some more tests where base url is different. >>>>>> Thanks Vyom, >>>>>> Looks good to me. >>>>>> >>>>>> See if you can get someone more experienced in the area >>>>>> to give a thumb up too :-) >>>>>> >>>>>> best regards, >>>>>> >>>>>> -- daniel >>>>>> >>>>>> >>>>>>> Thanks, >>>>>>> Vyom >>>>>>> >>>>>>> On Friday 17 June 2016 04:57 PM, Daniel Fuchs wrote: >>>>>>>> On 17/06/16 09:21, Vyom Tewari wrote: >>>>>>>>> Hi All, >>>>>>>>> >>>>>>>>> Please find the new >>>>>>>>> webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html >>>>>>>>> >>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.1/index.html>). >>>>>>>>> >>>>>>>>> I addressed the review comments given by Daniel. >>>>>>>> Hi Vyom, >>>>>>>> >>>>>>>> Looks good to me - but I'm a bit concerned that the previous >>>>>>>> mistake was not caught that by any test. >>>>>>>> Could you add a test that fails with you previous fix >>>>>>>> but passes with the new one? >>>>>>>> >>>>>>>> best regards, >>>>>>>> >>>>>>>> -- daniel >>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Vyom >>>>>>>>> >>>>>>>>> >>>>>>>>> On Sunday 12 June 2016 02:04 PM, Daniel Fuchs wrote: >>>>>>>>>> Hi Vyom, >>>>>>>>>> >>>>>>>>>> This looks strange to me: >>>>>>>>>> >>>>>>>>>> 268 if(!this.methods.isEmpty() && that.methods.isEmpty()) >>>>>>>>>> 269 return true; >>>>>>>>>> 270 if(this.methods.isEmpty() && !that.methods.isEmpty()) >>>>>>>>>> 271 return false; >>>>>>>>>> 272 if(this.methods.isEmpty() && that.methods.isEmpty()) >>>>>>>>>> 273 return true; >>>>>>>>>> >>>>>>>>>> Namely, lines 269 & 273 will return true before the URL part >>>>>>>>>> of the permission has been checked. >>>>>>>>>> Is that really the expected behavior? >>>>>>>>>> >>>>>>>>>> best regards, >>>>>>>>>> >>>>>>>>>> -- daniel >>>>>>>>>> >>>>>>>>>> On 11/06/16 05:50, vyom wrote: >>>>>>>>>>> Hi All, >>>>>>>>>>> >>>>>>>>>>> Please review the below fix. >>>>>>>>>>> >>>>>>>>>>> Bug : JDK-8071660 URLPermission not handling empty >>>>>>>>>>> method >>>>>>>>>>> lists correctly >>>>>>>>>>> Webrev : >>>>>>>>>>> http://cr.openjdk.java.net/~vtewari/8071660/webrev0.0/index.html >>>>>>>>>>> <http://cr.openjdk.java.net/%7Evtewari/8071660/webrev0.0/index.html> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Vyom >
