code review 7287046: exp/cookiejar: infrastructure for upcoming imple...

exp/cookiejar: infrastructure for upcoming implementation

This CL is the first of a handful of CLs which will provide
the implementation of cookiejar. It contains several helper
functions and the skeleton of Cookies and SetCookies.

Proper host name handling requires the ToASCII transformation
from package idna which currently lives in the go.net
subrepo. This CL thus contains just a TODO for this issue.

[volker.dobler, 2013-02-04 15:40:39]

Hello nigeltao@golang.org, rsc@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[nigeltao, 2013-02-05 10:35:28]

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:81: Name       string
I would still rather that this embeds an http.Cookie. I don't find the memory
argument compelling; you're using map[string]map[string]entry, which is easy to
use, but it isn't the cheapest data structure, in terms of memory.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:98: cookies := make([]*http.Cookie, 0)
var cookies []*http.Cookie

or move this into the return value:

func (j *Jar) Cookies(u *url.URL) (cookies []*http.Cookie) {

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:187: // strip trailing dot from fully qualified
domain names
Comments should be complete sentences, and thus start with a capital letter and
end with a full stop.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:198: // hostPort returns true if host contains a
port number. host may be a host name,
s/true if/whether/

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:220: func (_ idnaT) ToASCII(s string) (string,
error) { return s, nil }
If this is a no-op, just don't bother with the type, and move the TODO above.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:235: // provided public suffix list psl is broken
// The provided public etc
// etc stopgap.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:243: // isIP checks if host is formally an IP
address.
s/formally//

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:245: ip := net.ParseIP(host)
return net.ParseIP(host) != nil

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar_te...
File src/pkg/exp/cookiejar/jar_test.go (right):

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar_te...
src/pkg/exp/cookiejar/jar_test.go:16: func (_ testPSL) String() string {
Drop the _.

[volker.dobler, 2013-02-05 13:51:13]

PTAL

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:81: Name       string
On 2013/02/05 10:35:28, nigeltao wrote:
> I would still rather that this embeds an http.Cookie. I don't find the memory
> argument compelling; you're using map[string]map[string]entry, which is easy
to
> use, but it isn't the cheapest data structure, in terms of memory.

I do not think that embedding a http.Cookie is right:
 - entry should not inherit the method of http.Cookie
 - nowhere in the implementation is a http.Cookie copied
   to or from an entry, they are just too different.
 - if http.Cookie acquires more fields (e.g. Creation)
   the code here must be changed anyway
 - completely new field for http.Cookie -- others than
   needed by cookiejar -- are very unlikely and I believe
   that these won't need storage in a jar
Thus I cannot see any advantage of embedding a http.Cookie
in entry.

On the other hand: not embedding has the following
advantages:
 - it saves a few bytes (but I agree that this is peanuts
   compared to the map overhead an the possible large
   Value)
 - it is pretty clear what happens to Raw, Unparsed and
   RawExpires

I thus think, that a entry type with all fields is slighty
better.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:98: cookies := make([]*http.Cookie, 0)
On 2013/02/05 10:35:28, nigeltao wrote:
> var cookies []*http.Cookie
> 
> or move this into the return value:
> 
> func (j *Jar) Cookies(u *url.URL) (cookies []*http.Cookie) {

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:187: // strip trailing dot from fully qualified
domain names
On 2013/02/05 10:35:28, nigeltao wrote:
> Comments should be complete sentences, and thus start with a capital letter
and
> end with a full stop.

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:220: func (_ idnaT) ToASCII(s string) (string,
error) { return s, nil }
On 2013/02/05 10:35:28, nigeltao wrote:
> If this is a no-op, just don't bother with the type, and move the TODO above.

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:235: // provided public suffix list psl is broken
On 2013/02/05 10:35:28, nigeltao wrote:
> // The provided public etc
> // etc stopgap.

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:243: // isIP checks if host is formally an IP
address.
On 2013/02/05 10:35:28, nigeltao wrote:
> s/formally//

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:245: ip := net.ParseIP(host)
On 2013/02/05 10:35:28, nigeltao wrote:
> return net.ParseIP(host) != nil

Done.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar_te...
File src/pkg/exp/cookiejar/jar_test.go (right):

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar_te...
src/pkg/exp/cookiejar/jar_test.go:16: func (_ testPSL) String() string {
On 2013/02/05 10:35:28, nigeltao wrote:
> Drop the _.

Done.

[nigeltao, 2013-02-06 11:31:44]

LGTM.

All my comments are minor. I'll fix them when I submit.

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:81: Name       string
On 2013/02/05 13:51:13, volker.dobler wrote:
> I do not think that embedding a http.Cookie is right:

Well, I guess we'll agree to disagree, then. But let's not that turn into a
showstopper.

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:126: j.entries[key] = submap
This won't be necessary if you modify submap in-place, but I guess we'll see
that when you implement the TODOs.

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:150: if len(cookies) == 0 {
This test is cheapest, I'd do it first.

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:187: host = host[:len(host)-1]
Is "www.example.com..." worth considering?

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:198: // hostPort returns true whether host contains
a port number. host may be a
s/hostPort/hasPort/ and s/true//

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:208: if host[0] == '[' && strings.Index(host, "]:")
!= -1 {
s/Index/Contains/

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:237: // isIP checks if host is an IP address.
s/checks if/returns whether/

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar_te...
File src/pkg/exp/cookiejar/jar_test.go (right):

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar_te...
src/pkg/exp/cookiejar/jar_test.go:88: t.Errorf("%q: got %q, want \"com\"", host,
got)
Use `` quotes.

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar_te...
src/pkg/exp/cookiejar/jar_test.go:104: for h, want := range isIPTests {
s/h/host/

[nigeltao, 2013-02-06 11:36:54]

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:107: key := jarKey(host, j.psList)
One more thing: SetCookies checks if key == "". Should the two be consistent?

[nigeltao, 2013-02-06 11:37:48]

*** Submitted as https://code.google.com/p/go/source/detail?r=8f9b0fbf4c15 ***

exp/cookiejar: infrastructure for upcoming implementation

This CL is the first of a handful of CLs which will provide
the implementation of cookiejar. It contains several helper
functions and the skeleton of Cookies and SetCookies.

Proper host name handling requires the ToASCII transformation
from package idna which currently lives in the go.net
subrepo. This CL thus contains just a TODO for this issue.

R=nigeltao, rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/7287046

Committer: Nigel Tao <nigeltao@golang.org>

[volker.dobler, 2013-02-06 11:50:03]

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7287046/diff/9002/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:187: host = host[:len(host)-1]
On 2013/02/06 11:31:44, nigeltao wrote:
> Is "www.example.com..." worth considering?

I don't think so: "www.example.com..." is not a valid
host name while "www.example.com." is.