On 21 Jun 2016, at 11:21, Chris Hegarty <chris.hega...@oracle.com> wrote: > > 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 ) ?
Seems like a corner case and not worth trying to contort the spec to cover it. Such a permission is effectively useless anyway. I’ll sponsor this change for you. -Chris. > > -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 >> >
