Update cookie expiry policy to attempt to avoid deleting "in-use" cookies. S...

Update cookie expiry policy to attempt to avoid deleting "in-use" cookies.  See bug for exact details.

BUG=8850
TEST=Covered by unittests

[Peter Kasting, 2009-04-28 21:33:24]

ping

[Dean McNamee, 2009-04-30 13:43:33]

My comments are all confused across snapshots.  Sending this out, and will
probably make a few more:

http://codereview.chromium.org/99100/diff/16/20
File chrome/common/net/cookie_monster_sqlite.cc (right):

http://codereview.chromium.org/99100/diff/16/20#newcode323
Line 323: base::Time* least_recent_access) {
oldest_access ?

http://codereview.chromium.org/99100/diff/16/21
File chrome/common/net/cookie_monster_sqlite.h (right):

http://codereview.chromium.org/99100/diff/16/21#newcode28
Line 28: base::Time*);
don't we generally give these names?

http://codereview.chromium.org/99100/diff/16/19
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/99100/diff/16/19#newcode75
Line 75: static const double kPurgeOverageFactor = 1.1;
Overage? I prefer the original code, I don't see a huge benefit from the new
floating point...

http://codereview.chromium.org/99100/diff/16/18
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/99100/diff/16/18#newcode216
Line 216: base::Time least_recent_access_;
oldest_access_ ?

http://codereview.chromium.org/99100/diff/26/31
File chrome/common/net/cookie_monster_sqlite.h (right):

http://codereview.chromium.org/99100/diff/26/31#newcode28
Line 28: base::Time*);
these all need names.

[Dean McNamee, 2009-04-30 13:57:30]

Overall I agree that we should do something, but I am scared of change, and this
code makes things a bit more complicated.  It was so great when things were more
straight forward.

I am a bit worried about the 30-day free-for-all.  It seems in a better system
the newness should take some sort of weight (maybe log or something), but it
still seems like if you had 2gb of cookies in 10 days, we should probably do
something about it.

http://codereview.chromium.org/99100/diff/26/29
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/99100/diff/26/29#newcode71
Line 71: // Cookie garbage collection thresholds.  Based off of the Mozilla
defaults.
You should add a big block comment here explaining our whole policy now, what
the purge limits mean, when they're hit, if they're ignored for a certain young
set, etc.

http://codereview.chromium.org/99100/diff/26/29#newcode625
Line 625: cookie_its.begin() + std::min(cookie_its.size(), num_purge + 1),
why + 1 ?

[Peter Kasting, 2009-04-30 13:57:58]

http://codereview.chromium.org/99100/diff/16/20
File chrome/common/net/cookie_monster_sqlite.cc (right):

http://codereview.chromium.org/99100/diff/16/20#newcode323
Line 323: base::Time* least_recent_access) {
On 2009/04/30 13:43:33, Dean McNamee wrote:
> oldest_access ?

I could do that.  Only worry is that it's the oldest "most recent access" so
it's not really a true oldest access... no name seems good here.  Say whatever
you think is clearest and I'll change to it :)

http://codereview.chromium.org/99100/diff/16/21
File chrome/common/net/cookie_monster_sqlite.h (right):

http://codereview.chromium.org/99100/diff/16/21#newcode28
Line 28: base::Time*);
On 2009/04/30 13:43:33, Dean McNamee wrote:
> don't we generally give these names?

We do in most code... I was going by the other virtual functions below (and in
the interface) which didn't.  Should I just update them all to have names?  I'd
be happy with that.

http://codereview.chromium.org/99100/diff/16/19
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/99100/diff/16/19#newcode75
Line 75: static const double kPurgeOverageFactor = 1.1;
On 2009/04/30 13:43:33, Dean McNamee wrote:
> Overage? I prefer the original code, I don't see a huge benefit from the new
> floating point...

It makes the day calculation a bit trickier to do it the old way (I had it that
way at first and going to this way simplifier things).  At the very least you
want to switch from "max & purge amount" to "max & purge target amount".

TBH I had to wrap my brain around the old algorithm anew every time, this one
seems clearer to me...