code review 7235065: exp/cookiejar: remove external storage

exp/cookiejar: remove external storage

This CL removes the external storage of a cookie jar
and minimized the exported API as discussed in [1].

[1] https://groups.google.com/d/topic/golang-dev/ygDB3nbir00/discussion

Update issue 1960.

[volker.dobler, 2013-01-30 13:00:16]

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

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

[nigeltao, 2013-01-31 01:22:16]

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

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:42: type NoPublicSuffixList struct{}
I don't see the need for this type. Users of this package won't want to
explicitly use it, and Jar can just check if j.psList == nil.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:51: // A nil value is valid in which case the Jar
will use the TLD of a
This should note that it is also not secure. Also, the meaning of a nil value
here should probably be the same as the one described in lines 23-25: "An
implementation that always etc.". I think that `return TLD` instead of `return
""` would provide a false sense of security.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:55: // Other options may be added in Go 1.2 and
beyond.
I know that this was in the mail I sent, but I'd drop this line.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:63: mu sync.Mutex
This (and the entries field, and type entry) belong in a follow-up change, not
in this change.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:78: type entry struct {
This belongs in a follow-up change, but FWIW, I would prefer entry to embed an
http.Cookie.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:94: // It returns nil if the URL's scheme is not
HTTP or HTTPS.
I would prefer to revert this change.

[volker.dobler, 2013-01-31 08:54:31]

PTAL

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

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:42: type NoPublicSuffixList struct{}
On 2013/01/31 01:22:16, nigeltao wrote:
> I don't see the need for this type. Users of this package won't want to
> explicitly use it, and Jar can just check if j.psList == nil.

Done.

This type was Brad's whish, but I agree that "a nil PSL
means something safe" is impossible

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:51: // A nil value is valid in which case the Jar
will use the TLD of a
On 2013/01/31 01:22:16, nigeltao wrote:
> This should note that it is also not secure. Also, the meaning of a nil value
> here should probably be the same as the one described in lines 23-25: "An
> implementation that always etc.". I think that `return TLD` instead of `return
> ""` would provide a false sense of security.

Done.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:55: // Other options may be added in Go 1.2 and
beyond.
On 2013/01/31 01:22:16, nigeltao wrote:
> I know that this was in the mail I sent, but I'd drop this line.

Done.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:63: mu sync.Mutex
On 2013/01/31 01:22:16, nigeltao wrote:
> This (and the entries field, and type entry) belong in a follow-up change, not
> in this change.

Done.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:78: type entry struct {
On 2013/01/31 01:22:16, nigeltao wrote:
> This belongs in a follow-up change, but FWIW, I would prefer entry to embed an
> http.Cookie.

Removed.

Why embeding a http.Cookie? The methods of http.Cookie are
pretty useless here and the entry type here has 144 bytes (6g)
which can be reduced to 104 bytes, whereas embedding a
http.Cookie requires 216 bytes.

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:94: // It returns nil if the URL's scheme is not
HTTP or HTTPS.
On 2013/01/31 01:22:16, nigeltao wrote:
> I would prefer to revert this change.

Done.

Why? The empty slice must be allocated while returning nil
saves one allocation.

[nigeltao, 2013-01-31 23:53:18]

Submitting...

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

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:78: type entry struct {
On 2013/01/31 08:54:32, volker.dobler wrote:
> On 2013/01/31 01:22:16, nigeltao wrote:
> > This belongs in a follow-up change, but FWIW, I would prefer entry to embed
an
> > http.Cookie.
> 
> Removed.
> 
> Why embeding a http.Cookie? The methods of http.Cookie are
> pretty useless here and the entry type here has 144 bytes (6g)
> which can be reduced to 104 bytes, whereas embedding a
> http.Cookie requires 216 bytes.

I'm not too worried about the bytes. If it turns out to be a problem, we can
change it. On the other hand, it's easier to write "entry.cookie = *cookie" than
it is to copy out the fields by hand. The latter is also trickier if we ever add
fields to the http.Cookie type.

Out of curiosity, how do you reduce 144 bytes to 104 bytes?

https://codereview.appspot.com/7235065/diff/4001/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:94: // It returns nil if the URL's scheme is not
HTTP or HTTPS.
On 2013/01/31 08:54:32, volker.dobler wrote:
> On 2013/01/31 01:22:16, nigeltao wrote:
> > I would prefer to revert this change.
> 
> Done.
> 
> Why? The empty slice must be allocated while returning nil
> saves one allocation.

Nil is an empty slice; len(nil) == 0. I'd rather the callee be free to return
either nil or an allocated empty slice, than the caller to assume it is nil.

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

https://codereview.appspot.com/7235065/diff/2008/src/pkg/exp/cookiejar/jar.go...
src/pkg/exp/cookiejar/jar.go:41: // an HTTP server can set a cookie for a
domain.
I'd add a blank line to break the paragraphs.

[nigeltao, 2013-01-31 23:57:15]

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

exp/cookiejar: remove external storage

This CL removes the external storage of a cookie jar
and minimized the exported API as discussed in [1].

[1] https://groups.google.com/d/topic/golang-dev/ygDB3nbir00/discussion

Update issue 1960.

R=nigeltao
CC=golang-dev
https://codereview.appspot.com/7235065

Committer: Nigel Tao <nigeltao@golang.org>

[volker.dobler, 2013-02-01 10:21:33]

> I'm not too worried about the bytes. If it turns out to be a problem, we
> can change it. On the other hand, it's easier to write "entry.cookie =
> *cookie" than it is to copy out the fields by hand. The latter is also
> trickier if we ever add fields to the http.Cookie type.
>

It turns out that not that many fields are just copied: Domain and Path
e.g. need heavy processing, MaxAge and Expires are combined into
Expires and the Raw/Unparsed stuff has to be cleared. The only direct
copies are Name, Value, HttpOnly and Secure


>
> Out of curiosity, how do you reduce 144 bytes to 104 bytes?

Combine the three bools to a single uint, replace absolute
LastAccess and Expires by time.Duration relative to Creation
and make Domain a *string.