Looks fine to me John.

-Chris.

On 27/06/2013 17:09, John Zavgren wrote:
All:
I just posted a webrev image of the latest changes:


http://cr.openjdk.java.net/~jzavgren/8015799/webrev.03/

Thanks!
John

On 06/26/2013 04:52 PM, Kurchi Hazra wrote:
Alright, thanks for the clarification - the source code changes are
good as they are then.

- Kurchi

On 6/26/2013 1:49 PM, Chris Hegarty wrote:
To link this email thread, both in the archives, and for others. The
call for review on this bug started with:
http://mail.openjdk.java.net/pipermail/net-dev/2013-June/006607.html

On 06/26/2013 08:22 PM, Kurchi Hazra wrote:

On 6/26/2013 12:17 PM, Kurchi Hazra wrote:
Hi John,

Why not change lines 2810-2811 to:
if (value == null || value.length() == 0)
return value;
I meant return null. For other cookie-headers too, is there any reason
for us not returning null if the length of value is 0?

In the first webrev John had made this change, but I asked him to
revert it and only change the Set-Cookie(2) headers.

"Since all header retrieval passes through filterHeaderField, in one
way or another, I'm a little concerned about changing this. Also, as
the only issue we know of is with Set-Cookie(2), maybe you could add
the empty string check to these headers only? ( that is to say, move
the 'value.length() == 0' check into the ' if
(SET_COOKIE.equalsIgnoreCase(name)..... ' "

The difference is, currently if a header value is non-null and has a
length of 0 ( i.e. empty string ), then empty string is returned.
With the original change then null is returned.

We have been bitten by subtle changes in this area before. Returning
null from such an API, URLConnection.getHeaderField(s), for cases
where it did not return null before may lead to NPE's in some
applications.

-Chris.


Also, lots of formatting issue in the test, especially in
TestCookieHandler, try-catch block indentation is off in line 54.
Its also best to stop the server in a finally clause at line 58.
Alternatively, I also liked Andreas' use of autocloseable in his test
for 6563286. See [1].

Yes, please.

-Chris.


- Kurchi

[1]
http://cr.openjdk.java.net/~arieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html

<http://cr.openjdk.java.net/%7Earieber/6563286/webrev.00/test/sun/net/www/http/HttpURLConnection/MalformedFollowRedirect.java.html>


On 6/26/2013 10:54 AM, John Zavgren wrote:
Please consider the following changes to the Java cookie code.

http://cr.openjdk.java.net/~jzavgren/8015799/webrev.02/
<http://cr.openjdk.java.net/%7Ejzavgren/8015799/webrev.02/>

The problem I fixed occurs when a server returns an array of cookies
that contains a null cookie.

Thanks
John
--
John Zavgren
john.zavg...@oracle.com
603-821-0904
US-Burlington-MA


--
-Kurchi


Reply via email to