Fwd: Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Pavel Rappo]

Sorry for the wrong method. There should've been onPing instead of onPong.
What's wrong with these two?

/**
 * Receives a Ping message.
 *
 *  A Ping message may be sent or received by either client or
 * server. It may serve either as a keepalive or as a means to verify
 * that the remote endpoint is still responsive.
 *
 *  The message will consist of not more than {@code 125} bytes:
 * {@code message.remaining() <= 125}.
 *
 *  The {@code onPing} is invoked zero or more times in between
 * {@code onOpen} and ({@code onClose} or {@code onError}).
 *
 *  If an exception is thrown from this method or the returned {@code
 * CompletionStage} completes exceptionally, then {@link
 * #onError(WebSocket, Throwable) onError} will be invoked with this
 * exception.
 *
 *  The WebSocket implementation responds to the received Ping
 * message using a strategy of its choice, but withing the boundaries of
 * the WebSocket Protocol. It may call this method after handling the
 * Ping message or before doing so. In other words no particular
 * ordering is guaranteed. If an error occurs while implementation
 * handles this Ping message, then {@code onError} will be invoked with
 * this error.
 *
 *  For more information on handling Ping messages see RFC 6455 sections
 * https://tools.ietf.org/html/rfc6455#section-5.5.2;>5.5.2. Ping 
and
 * https://tools.ietf.org/html/rfc6455#section-5.5.3;>5.5.2. Pong.
 *
 * @implSpec The default implementation behaves as if:
 *
 * {@code
 * webSocket.request(1);
 * return CompletableFuture.completedStage(null);
 * }
 *
 * @param webSocket
 * the WebSocket
 * @param message
 * the message
 *
 * @return a CompletionStage that completes when the message processing
 * is done; or {@code null} if already done
 */
default CompletionStage onPing(WebSocket webSocket,
  ByteBuffer message) {
webSocket.request(1);
return null;
}

> Begin forwarded message:
> 
> From: Pavel Rappo 
> Subject: Preliminary RFR JDK-8159053: Improve onPing/onClose behavior
> Date: 17 June 2016 at 16:15:59 IST
> To: OpenJDK Network Dev list 
> 
> Hi,
> 
> Could you please [*] review the following change for the upcoming JDK-8159053?
> This change addresses:
> 
> 1. Listener.onClose/onPing behaviour, making the implementation fully*
> responsible of protocol compliance. So reactions on onClose/onPing cannot be
> overridden/redefined in a Listener
> 
> 2. Simpler representation of the Close message.
> 
>/**
> * Receives a Pong message.
> *
> *  A Pong message may be unsolicited or may be received in response
> * to a previously sent Ping. In the latter case, the contents of the
> * Pong is identical to the originating Ping.
> *
> *  The message will consist of not more than {@code 125} bytes:
> * {@code message.remaining() <= 125}.
> *
> *  The {@code onPong} method is invoked zero or more times in
> * between {@code onOpen} and ({@code onClose} or {@code onError}).
> *
> *  If an exception is thrown from this method or the returned {@code
> * CompletionStage} completes exceptionally, then {@link
> * #onError(WebSocket, Throwable) onError} will be invoked with this
> * exception.
> *
> * @implSpec The default implementation behaves as if:
> *
> * {@code
> * webSocket.request(1);
> * return CompletableFuture.completedStage(null);
> * }
> *
> * @param webSocket
> * the WebSocket
> * @param message
> * the message
> *
> * @return a CompletionStage that completes when the message processing
> * is done; or {@code null} if already done
> */
>default CompletionStage onPong(WebSocket webSocket,
>  ByteBuffer message) {
>webSocket.request(1);
>return null;
>}
> 
>/**
> * Receives a Close message.
> *
> *  Once a Close message is received, the server will not send any
> * more messages.
> *
> *  A Close message consists of a status code and a reason for
> * closing. The status code is an integer in the range {@code 1000 <=
> * code <= 65535}. The reason is a short string that has an UTF-8
> * representation not longer than {@code 123} bytes.
> *
> *  For more information on status codes and reasons see
> * https://tools.ietf.org/html/rfc6455#section-7.4;>RFC 6455, 
> 7.4. Status Codes.
> *
> *  {@code onClose} is the last invocation on the {@code Listener}.
> * It is invoked at most once, but after {@code onOpen}. If an exception
> * is thrown from this method, it is ignored.
> *
> *  After a Close message has been received, the implementation will
> * close the connection automatically. However, the client is allowed to
> * finish sending the current sequence of partial message first. To
> * facilitate it, the 

Preliminary RFR JDK-8159053: Improve onPing/onClose behavior

[Pavel Rappo]

Hi,

Could you please [*] review the following change for the upcoming JDK-8159053?
This change addresses:

1. Listener.onClose/onPing behaviour, making the implementation fully*
responsible of protocol compliance. So reactions on onClose/onPing cannot be
overridden/redefined in a Listener

2. Simpler representation of the Close message.

/**
 * Receives a Pong message.
 *
 *  A Pong message may be unsolicited or may be received in response
 * to a previously sent Ping. In the latter case, the contents of the
 * Pong is identical to the originating Ping.
 *
 *  The message will consist of not more than {@code 125} bytes:
 * {@code message.remaining() <= 125}.
 *
 *  The {@code onPong} method is invoked zero or more times in
 * between {@code onOpen} and ({@code onClose} or {@code onError}).
 *
 *  If an exception is thrown from this method or the returned {@code
 * CompletionStage} completes exceptionally, then {@link
 * #onError(WebSocket, Throwable) onError} will be invoked with this
 * exception.
 *
 * @implSpec The default implementation behaves as if:
 *
 * {@code
 * webSocket.request(1);
 * return CompletableFuture.completedStage(null);
 * }
 *
 * @param webSocket
 * the WebSocket
 * @param message
 * the message
 *
 * @return a CompletionStage that completes when the message processing
 * is done; or {@code null} if already done
 */
default CompletionStage onPong(WebSocket webSocket,
  ByteBuffer message) {
webSocket.request(1);
return null;
}

/**
 * Receives a Close message.
 *
 *  Once a Close message is received, the server will not send any
 * more messages.
 *
 *  A Close message consists of a status code and a reason for
 * closing. The status code is an integer in the range {@code 1000 <=
 * code <= 65535}. The reason is a short string that has an UTF-8
 * representation not longer than {@code 123} bytes.
 *
 *  For more information on status codes and reasons see
 * https://tools.ietf.org/html/rfc6455#section-7.4;>RFC 6455, 7.4. 
Status Codes.
 *
 *  {@code onClose} is the last invocation on the {@code Listener}.
 * It is invoked at most once, but after {@code onOpen}. If an exception
 * is thrown from this method, it is ignored.
 *
 *  After a Close message has been received, the implementation will
 * close the connection automatically. However, the client is allowed to
 * finish sending the current sequence of partial message first. To
 * facilitate it, the WebSocket implementation expects this method to
 * return a {@code CompletionStage}. The implementation then will
 * attempt to close the connection at the earliest of, either the
 * completion of the returned {@code CompletionStage} or the last part
 * of the current message has been sent.
 *
 * @implSpec The default implementation behaves as if:
 *
 * {@code
 * return CompletableFuture.completedStage(null);
 * }
 *
 * @param webSocket
 * the WebSocket
 * @param statusCode
 * the status code
 * @param reason
 * the reason
 *
 * @return a CompletionStage that when completed will trigger closing of
 * the WebSocket
 */
default CompletionStage onClose(WebSocket webSocket,
   int statusCode,
   String reason) {
return null;
}

/**
 * Sends a Close message with the given status code and the reason.
 *
 *  Returns a {@code CompletableFuture} which completes
 * normally when the message has been sent or completes exceptionally if an
 * error occurs.
 *
 *  The {@code statusCode} is an integer in the range {@code 1000 <= code
 * <= 4999}, except for {@code 1004}, {@code 1005}, {@code 1006} and {@code
 * 1015}. The {@code reason} is a short string that must have an UTF-8
 * representation not longer than {@code 123} bytes.
 *
 *  For more information about status codes see
 * https://tools.ietf.org/html/rfc6455#section-7.4;>RFC 6455, 7.4. 
Status Codes.)
 *
 *  If a Close message has been already sent or the {@code WebSocket} is
 * closed, then invoking this method has no effect and returned {@code
 * CompletableFuture} completes normally.
 *
 *  The returned {@code CompletableFuture} can complete exceptionally
 * with:
 * 
 *  {@link IOException}
 *  if an I/O error occurs during this operation
 * 
 *
 * @param statusCode
 * the status code
 * @param reason
 * the reason
 *
 * @return a CompletableFuture with this WebSocket
 *
 * @throws IllegalArgumentException
 * if the 

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Roger Riggs]

+1

Thanks, Roger


On 6/17/2016 10:27 AM, Chris Hegarty 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
). I
fixed the typo along with spaces issue.


Thanks Vyom.  Looks good.

-Chris.


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 

) 
.

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 



). 



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
 




Thanks,
Vyom

Re: RFR: 8159745: Remove java.net.Parts

[Claes Redestad]

On 2016-06-17 16:26, Chris Hegarty wrote:



On 17/06/16 15:05, Claes Redestad wrote:



On 2016-06-17 16:04, Alan Bateman wrote:

On 17/06/2016 14:43, Claes Redestad wrote:


Hi,

URL.java defines a package-private class Parts, which can be
trivially inlined in the one place where it's currently used.

Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159745


I suspect that class had other usages in old releases. Your patch
looks okay, maybe change `ind` to `index` or something that doesn't
look like `int` :-)


Thanks, I'll update ind before push. :-)


+1  Thanks Claes.


Thanks for the reviews, pushed!

/Claes

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Chris Hegarty]

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
). I
fixed the typo along with spaces issue.


Thanks Vyom.  Looks good.

-Chris.


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
) .
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

).

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



Thanks,
Vyom

Re: RFR: 8159745: Remove java.net.Parts

[Chris Hegarty]

On 17/06/16 15:05, Claes Redestad wrote:



On 2016-06-17 16:04, Alan Bateman wrote:

On 17/06/2016 14:43, Claes Redestad wrote:


Hi,

URL.java defines a package-private class Parts, which can be
trivially inlined in the one place where it's currently used.

Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159745


I suspect that class had other usages in old releases. Your patch
looks okay, maybe change `ind` to `index` or something that doesn't
look like `int` :-)


Thanks, I'll update ind before push. :-)


+1  Thanks Claes.

-Chris.

Re: RFR: 8159745: Remove java.net.Parts

[Claes Redestad]

On 2016-06-17 16:04, Alan Bateman wrote:

On 17/06/2016 14:43, Claes Redestad wrote:


Hi,

URL.java defines a package-private class Parts, which can be 
trivially inlined in the one place where it's currently used.


Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159745

I suspect that class had other usages in old releases. Your patch 
looks okay, maybe change `ind` to `index` or something that doesn't 
look like `int` :-)


Thanks, I'll update ind before push. :-)

/Claes

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi,

Please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.3/index.html 
). I 
fixed the typo along with spaces issue.


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
) .
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 

). 


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
 



Thanks,
Vyom

Re: RFR: 8159745: Remove java.net.Parts

[Alan Bateman]

On 17/06/2016 14:43, Claes Redestad wrote:


Hi,

URL.java defines a package-private class Parts, which can be trivially 
inlined in the one place where it's currently used.


Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159745

I suspect that class had other usages in old releases. Your patch looks 
okay, maybe change `ind` to `index` or something that doesn't look like 
`int` :-)


-Alan

RFR: 8159745: Remove java.net.Parts

[Claes Redestad]

Hi,

URL.java defines a package-private class Parts, which can be trivially 
inlined in the one place where it's currently used.


Webrev: http://cr.openjdk.java.net/~redestad/8159745/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8159745

Thanks!

/Claes

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Roger Riggs]

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
) .
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 


).
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


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Daniel Fuchs]

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
) .
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
).
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


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi Daniel,

thanks for review please find the latest 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.2/index.html 
) . 
I added some more tests where base url is different.


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
).
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


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Daniel Fuchs]

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
).
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


Thanks,
Vyom

Re: RFR 8071660 :URLPermission not handling empty method lists correctly

[Vyom Tewari]

Hi All,

Please find the new 
webrev(http://cr.openjdk.java.net/~vtewari/8071660/webrev0.1/index.html 
). 
I  addressed the review comments given by 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


Thanks,
Vyom