Fix bug in DeleteAllForURL; deletes entire store instead of just

Fix bug in DeleteAllForURL; deletes entire store instead of just 
cookies related to URL (found by inspection.)  Also changed name 
and semantics to more closely reflect usage of primary caller (extension 
data deleter), and added test for that set of semantics.

BUG=none
TEST=Linux CookieMonsterTest.*:ParsedCookieTest.* (especially new CookieMonsterTest.DeleteAllHost)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51544

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51766

[Randy Smith (Not in Mondays), 2010-06-29 22:09:37]

Could you folks take a look at this CL?  I noticed a bug in DeleteAllForURL
while reading the code for other reasons, and thought I'd toss in a fix and a
test.  Mattias: I'm hoping you can verify that the functionality change meets
the requirements of the consumer; Andy & Eric, I hope you can verify that I'm
not messing up the CookieMonster :-}.

Mattias: As we talked about in separate, I've changed the semantics of the call
to be somewhat simpler to implement, and (I believe) more potentially useful to
other callers; it now only deletes host cookies and leaves domain cookies alone
(the change), and ignores paths, http_only, and secure (old behavior).  I'm most
interested in your comments on the test and header files; if you'd like to
suggest other tests for me to implement that more closely match how you expect
the interface to be used, that would be welcome.

I've documented that both in the header file and in the test.  If you could give
m

[Randy Smith (Not in Mondays), 2010-06-29 22:10:24]

On 2010/06/29 22:09:37, rdsmith wrote:
> Could you folks take a look at this CL?  I noticed a bug in DeleteAllForURL
> while reading the code for other reasons, and thought I'd toss in a fix and a
> test.  Mattias: I'm hoping you can verify that the functionality change meets
> the requirements of the consumer; Andy & Eric, I hope you can verify that I'm
> not messing up the CookieMonster :-}.
> 
> Mattias: As we talked about in separate, I've changed the semantics of the
call
> to be somewhat simpler to implement, and (I believe) more potentially useful
to
> other callers; it now only deletes host cookies and leaves domain cookies
alone
> (the change), and ignores paths, http_only, and secure (old behavior).  I'm
most
> interested in your comments on the test and header files; if you'd like to
> suggest other tests for me to implement that more closely match how you expect
> the interface to be used, that would be welcome.
> 


> I've documented that both in the header file and in the test.  If you could
give
> m
Please ignore this last paragraph fragment; I missed in in final edit :-{.

[eroman, 2010-06-29 23:53:20]

LGTM

> and semantics more closely reflect usage of primary caller 

Sounds like there is a "to" missing. (changelist description).

http://codereview.chromium.org/2857029/diff/1/4
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2857029/diff/1/4#newcode141
net/base/cookie_monster.h:141: // Delete all cookies that match the host of the
given URL
nit: our style is to use passive tense in comments (as in "Deletes"). it looks
like this file doesn't follow that convention though. if you want to fix up the
whole file that would be great. (otherwise you can leave it as-is for
consistency).

http://codereview.chromium.org/2857029/diff/1/4#newcode143
net/base/cookie_monster.h:143: // but does not include any domain cookies that
may apply to this host.
seems there is a negative missing: "but does not include any domain cookies that
don't apply to this host" ?

http://codereview.chromium.org/2857029/diff/1/4#newcode144
net/base/cookie_monster.h:144: int DeleteAllHost(const GURL& url);
Can you document the return value?

Also, I think the name would be more consistent with other functions as:
"DeleteAllForHost" or "DeleteAllFromHost".

http://codereview.chromium.org/2857029/diff/1/5
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/2857029/diff/1/5#newcode1637
net/base/cookie_monster_unittest.cc:1637: static const char*
kTopLevelDomainPlus1 = "http://www.harvard.edu";
Can you move this to the anonymous namespace at the top of the file? (as it
contains the other test helpers).

At which point, you can remove the "static"

http://codereview.chromium.org/2857029/diff/1/5#newcode1644
net/base/cookie_monster_unittest.cc:1644: static void
populate_cm(scoped_refptr<net::CookieMonster> cm)
Can you name this with CamelCase()

http://codereview.chromium.org/2857029/diff/1/5#newcode1645
net/base/cookie_monster_unittest.cc:1645: {
nit: move this brace to the preceding line.

http://codereview.chromium.org/2857029/diff/1/5#newcode1723
net/base/cookie_monster_unittest.cc:1723: {
move the curly brace to previous line.

[Mattias Nissler (ping if slow), 2010-06-30 08:41:52]

http://codereview.chromium.org/2857029/diff/1/5
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/2857029/diff/1/5#newcode1741
net/base/cookie_monster_unittest.cc:1741: EXPECT_EQ(8U,
cm->GetAllCookies().size());
This actually doesn't check that the correct cookies get deleted, but only the
number of cookies deleted is correct. I think it's reasonable to check that the
remaining cookies are the ones you expect.

[Randy Smith (Not in Mondays), 2010-06-30 19:51:11]

Incorporated comments from eroman and mnissler (including typo in issue
description.)  Waiting on LGTMs from ahendrickson and mnissler.

http://codereview.chromium.org/2857029/diff/1/4
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/2857029/diff/1/4#newcode141
net/base/cookie_monster.h:141: // Delete all cookies that match the host of the
given URL
On 2010/06/29 23:53:20, eroman wrote:
> nit: our style is to use passive tense in comments (as in "Deletes"). it looks
> like this file doesn't follow that convention though. if you want to fix up
the
> whole file that would be great. (otherwise you can leave it as-is for
> consistency).

I'll put the whole file cleanup on my list of background cleanup tasks to do.

http://codereview.chromium.org/2857029/diff/1/4#newcode143
net/base/cookie_monster.h:143: // but does not include any domain cookies that
may apply to this host.
On 2010/06/29 23:53:20, eroman wrote:
> seems there is a negative missing: "but does not include any domain cookies
that
> don't apply to this host" ?

What I was trying to say was that if there's some domain cookie that
GetCookies() would return for this URL, it won't be deleted.  Thus domain
cookies that apply to this host will not be deleted.  So I think the comment has
the negatives right.

http://codereview.chromium.org/2857029/diff/1/4#newcode144
net/base/cookie_monster.h:144: int DeleteAllHost(const GURL& url);
On 2010/06/29 23:53:20, eroman wrote:
> Can you document the return value?

Done.

> Also, I think the name would be more consistent with other functions as:
> "DeleteAllForHost" or "DeleteAllFromHost".

Done.

http://codereview.chromium.org/2857029/diff/1/5
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/2857029/diff/1/5#newcode1637
net/base/cookie_monster_unittest.cc:1637: static const char*
kTopLevelDomainPlus1 = "http://www.harvard.edu";
On 2010/06/29 23:53:20, eroman wrote:
> Can you move this to the anonymous namespace at the top of the file? (as it
> contains the other test helpers).
> 
> At which point, you can remove the "static"

Done.

http://codereview.chromium.org/2857029/diff/1/5#newcode1644
net/base/cookie_monster_unittest.cc:1644: static void
populate_cm(scoped_refptr<net::CookieMonster> cm)
On 2010/06/29 23:53:20, eroman wrote:
> Can you name this with CamelCase()

Done.

http://codereview.chromium.org/2857029/diff/1/5#newcode1645
net/base/cookie_monster_unittest.cc:1645: {
On 2010/06/29 23:53:20, eroman wrote:
> nit: move this brace to the preceding line.

Done.

(Sorry, by the way, for all these little glitches--I'm assuming as I get more
experience with this code base, they'll go away.)

http://codereview.chromium.org/2857029/diff/1/5#newcode1723
net/base/cookie_monster_unittest.cc:1723: {
On 2010/06/29 23:53:20, eroman wrote:
> move the curly brace to previous line.

Done.

http://codereview.chromium.org/2857029/diff/1/5#newcode1741
net/base/cookie_monster_unittest.cc:1741: EXPECT_EQ(8U,
cm->GetAllCookies().size());
On 2010/06/30 08:41:53, mnissler wrote:
> This actually doesn't check that the correct cookies get deleted, but only the
> number of cookies deleted is correct. I think it's reasonable to check that
the
> remaining cookies are the ones you expect.

Done.

[Mattias Nissler (ping if slow), 2010-06-30 21:20:45]

LGTM

[ahendrickson, 2010-07-01 19:10:50]

LGTM.

On 2010/06/30 21:20:45, mnissler wrote:
> LGTM