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
