SessionRestore: Store session cookies and restore them if chrome crashes or auto-restarts.

Store session cookies and restore them if chrome crashes or autorestarts.

The feature is behind a command line flag --enable-restore-session-state.

BUG=1870
TEST=CookieMonsterTest.PersistSessionCookies, SQLitePersistentCookieStoreTest.Test(Dont)?LoadOldSessionCookies, SQLitePersistentCookieStoreTest.PersistHasExpiresAndIsPersistent


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

[marja, 2011-11-15 09:23:14]

Hi Jochen,

Does this approach look ok & is this going to the right direction?

(Restoring session cookies kinda works now, but it's not very elegant yet, e.g.,
if a cookie is removed, it's not pruned from the session command list. When it's
inserted again, CookieMonster will notice it is outdated, though.)

[jochen (gone - plz use gerrit), 2011-11-15 10:42:05]

On 2011/11/15 09:23:14, marja wrote:
> Hi Jochen,
> 
> Does this approach look ok & is this going to the right direction?
> 
> (Restoring session cookies kinda works now, but it's not very elegant yet,
e.g.,
> if a cookie is removed, it's not pruned from the session command list. When
it's
> inserted again, CookieMonster will notice it is outdated, though.)

I think that's generally ok.

Can you ask Patrick to file a meta bug for this feature and then create a bug
for cookies etc. blocking the meta bug?

[jochen (gone - plz use gerrit), 2011-11-15 10:42:14]

http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:1189: case
kCommandSessionCookieCreated: {
does this restore the cookies even if you just re-open the last opened tabs?

I think for now we should only do this for restart for upgrade or on crash

http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:151: extern const char
kEnableRestoreSessionCookies[];
I'd pick a more generic name, as you'll eventually restore more than just
cookies.

[erikwright (departed), 2011-11-15 14:41:25]

One concern I have is that this is unlikely to stay current with potential
additions to the fields of cookies. On the other hand, if you created your
own SQLitePersistentCookieStore instance to write out and read in the
cookies, you would be able to rely on the exact same serialization process
used by the actual cookie store.

Basically:

store = new SQLitePersistentCookieStore(path);
for each cookie: store.AddCookie(cookie);
store.Flush();

and later:

void MyCallback(std::vector<CanonicalCookie*>& cookies) {
  CookieList cookie_list;
  for each cookie in cookies: cookie_list.insert(*cookie) && delete cookie;

  real_cookie_monster->InitializeFrom(cookie_list);
}

store = new SQLitePersistentCookieStore(path);
store.Load(base::Bind(&MyCallback));


On Tue, Nov 15, 2011 at 5:42 AM, <jochen@chromium.org> wrote:

>
> http://codereview.chromium.**org/8533013/diff/8/chrome/**
>
browser/sessions/session_**service.cc<http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session_service.cc>
> File chrome/browser/sessions/**session_service.cc (right):
>
> http://codereview.chromium.**org/8533013/diff/8/chrome/**
>
browser/sessions/session_**service.cc#newcode1189<http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session_service.cc#newcode1189>
> chrome/browser/sessions/**session_service.cc:1189: case
> kCommandSessionCookieCreated: {
> does this restore the cookies even if you just re-open the last opened
> tabs?
>
> I think for now we should only do this for restart for upgrade or on
> crash
>
> http://codereview.chromium.**org/8533013/diff/8/chrome/**
>
common/chrome_switches.h<http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h>
> File chrome/common/chrome_switches.**h (right):
>
> http://codereview.chromium.**org/8533013/diff/8/chrome/**
>
common/chrome_switches.h#**newcode151<http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h#newcode151>
> chrome/common/chrome_switches.**h:151: extern const char
> kEnableRestoreSessionCookies[]**;
> I'd pick a more generic name, as you'll eventually restore more than
> just cookies.
>
>
http://codereview.chromium.**org/8533013/<http://codereview.chromium.org/8533...
>

[marja, 2011-11-17 12:54:46]

Thanks for comments!

In the current version, session cookies are stored in a separate CookieMonster.

http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/8533013/diff/8/chrome/browser/sessions/session...
chrome/browser/sessions/session_service.cc:1189: case
kCommandSessionCookieCreated: {
On 2011/11/15 10:42:14, jochen wrote:
> does this restore the cookies even if you just re-open the last opened tabs?
> 
> I think for now we should only do this for restart for upgrade or on crash

Done.

http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/8533013/diff/8/chrome/common/chrome_switches.h...
chrome/common/chrome_switches.h:151: extern const char
kEnableRestoreSessionCookies[];
On 2011/11/15 10:42:14, jochen wrote:
> I'd pick a more generic name, as you'll eventually restore more than just
> cookies. 

EnableRestoreSessionState?

[marja, 2011-11-22 17:14:01]

Hi Jochen,

Could you have another look at this? The latest version has
CookieMonster::MergeSessionCookies() for merging the cookies from another
CookieMonster. Does this look sane? (I'd still need to add tests..)

[jochen (gone - plz use gerrit), 2011-11-22 17:46:53]

the sqlite persistent cookie store has a loading logic that allows for priority
loading cookies for certain keys.

By first loading all cookies into a cookie list you then merge you don't make
use of this.

I think the way to go is that the "real" cookie monster orchestrates the
loading: it should trigger the loading of all cookies by calling Load() on the
session cookie monster's cookie store, and if there's a request coming in for a
certain key (eTLD), it should invoke LoadCookiesForKey to priority-load those
cookies.

makes sense?

[erikwright (departed), 2011-11-22 18:49:38]

I think this CL is quite problematic.

Between the time that MergeSessionCookies is called and they are returned from
the session cookie store, all kinds of things could happen. For example:

* initialization could begin, but not complete. Since you call InvokeQueue, a
bunch of tasks will run that think (wrongly) that all cookies are loaded.
* initialization could begin and complete. Requests for cookies will run,
without having received all the session cookies they should have. They may write
session cookies that you might later override.
* Domain-specific requests could execute, priority loading their cookies but not
yet having the session cookies (like in the 2nd case).

The current approach will only work if you guarantee that the session cookies
are fully merged in before a single request is served. You could add extra state
to block on session cookie loading, or you could do this during construction of
the cookie monster (i.e., before anyone has access to it).

Sadly, I think the easiest thing is to pass the second
SQLitePersistentCookieStore to the CookieMonster during construction and make
sure that you call Load() on both stores and LoadCookiesForKey() on both stores.
Then you should only invoke the relevant queued tasks when you have received the
callbacks from both sources.

http://codereview.chromium.org/8533013/diff/21002/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/21002/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1404: loaded_ = false;
This is not designed to work this way. I suspect it is likely to cause bad
behaviour.

http://codereview.chromium.org/8533013/diff/21002/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:2268: InvokeQueue();
This is not likely to work as expected.

[marja, 2011-11-23 12:06:13]

The previous version was indeed horribly broken, thanks for pointing it out.

I uploaded a new version: CookieMonster has a second SQLitePersistentCookieStore
for the session cookies, and it loads cookies from both cookie stores upon
startup and when cookies are requested during the startup.
SQLitePersistentCookieStore::
SetClearLocalStateOnExit is used for setting the "clear on exit" mode for the
session cookie store. When doing autorestart, the mode is set to "don't clear on
exit".

[jochen (gone - plz use gerrit), 2011-11-25 14:45:13]

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/infobars/inf...
File chrome/browser/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/infobars/inf...
chrome/browser/infobars/infobar_delegate.h:78: void InfoBarClosed();
why no make this a virtual method instead of adding the new one?

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:825: if
(!db_->Execute("DELETE FROM cookies WHERE persistent == 0"))
maybe add a field that says "discard session cookies on next open", and then
when reading the database again, we just ignore all session cookies if that flag
is set?

Might be fast on shutdown (and startup is async anyway)

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1422: it->second->SetIsOldSessionCookie(false);
this should notify the delegate about the cookie being added

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1431: InternalDeleteCookie(curit, true,
DELETE_COOKIE_OLD_SESSION_COOKIE);
we need to make sure that this doesn't call the cookie monster delegate

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1758: InternalDeleteCookie(curit, true,
DELETE_COOKIE_OLD_SESSION_COOKIE);
again, this shouldn't notify

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:279: void SetSyncSessionCookies(bool
sync_session_cookies);
I would use a different verb, as "sync" implies it would be related to chrome
sync

maybe persist?

[marja, 2011-11-28 15:22:01]

Thanks again for comments!

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/infobars/inf...
File chrome/browser/infobars/infobar_delegate.h (right):

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/infobars/inf...
chrome/browser/infobars/infobar_delegate.h:78: void InfoBarClosed();
On 2011/11/25 14:45:14, jochen wrote:
> why no make this a virtual method instead of adding the new one?

Then the subclass funcs would need to remember to call the superclass func
(otherwise it would leak), and that's quite fragile. I created a separate CL for
solving this: http://codereview.chromium.org/8698016

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/46019/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:825: if
(!db_->Execute("DELETE FROM cookies WHERE persistent == 0"))
On 2011/11/25 14:45:14, jochen wrote:
> maybe add a field that says "discard session cookies on next open", and then
> when reading the database again, we just ignore all session cookies if that
flag
> is set?
> 
> Might be fast on shutdown (and startup is async anyway)

In the latest version, it uses the "did chrome crash / autoupdate" information
to decide whether to discard the old session cookies, merge them in, or keep
them separate and wait for the decision. If we decide to discard, it's done
async in the DB thread. It can still happen that this is ongoing, but a session
cookie is read from the DB, and CookieMonster deals with it. Does this look ok?
It's quite complicated. Another option would be just always read the session
cookies here (if they still are in the DB) and let CookieMonster ignore them (it
anyway needs that logic).

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1422: it->second->SetIsOldSessionCookie(false);
On 2011/11/25 14:45:14, jochen wrote:
> this should notify the delegate about the cookie being added

Done. (And made it so that StoreLoadedCookies won't notify in this case.)

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1431: InternalDeleteCookie(curit, true,
DELETE_COOKIE_OLD_SESSION_COOKIE);
On 2011/11/25 14:45:14, jochen wrote:
> we need to make sure that this doesn't call the cookie monster delegate

Should already work since the notifications are set to false for
DELETE_COOKIE_OLD_SESSION_COOKIE.

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1758: InternalDeleteCookie(curit, true,
DELETE_COOKIE_OLD_SESSION_COOKIE);
On 2011/11/25 14:45:14, jochen wrote:
> again, this shouldn't notify

Same here.

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/8533013/diff/46019/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:279: void SetSyncSessionCookies(bool
sync_session_cookies);
On 2011/11/25 14:45:14, jochen wrote:
> I would use a different verb, as "sync" implies it would be related to chrome
> sync
> 
> maybe persist?

I was using sync because CookieMonster already uses it in this meaning, e.g.,
sync_to_store. But now it's persist. I'm happy to change it back if you change
your mind ;)

[jochen (gone - plz use gerrit), 2011-11-28 15:47:19]

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1928: // We're setting a cookie, and there are
session cookies for which we
I think we should also delete all session cookies for a key when we read cookies
for that key, wdyt?

[erikwright (departed), 2011-11-28 16:18:16]

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:640: sql::Transaction
transaction(db_.get());
Please add a metric, recorded and reported here, for the duration of the
migration (i.e., from line 640 to 654). Name it
"Cookie.TimeDatabaseMigrationToV5".

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:644: "ADD COLUMN
has_expires INTEGER DEFAULT 1") ||
I don't see any documentation that states that the DEFAULT value here will
actually be applied to existing rows, although it does seem to be implied.

Also, the definition for migrated tables will differ from that of non-migrated
tables (the former have a DEFAULT, the latter do not).

1) Please confirm experimentally that the DEFAULT in the ALTER TABLE updates the
value of existing rows.
2) If it does not, remove the DEFAULT and add an UPDATE statement here to apply
the value.
3) If it does, also include the default in the CREATE TABLE statement.

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:327: if
(command_line.HasSwitch(switches::kEnableRestoreSessionState))
Is it fair to say that this flag would eventually be removed, and we would
always do this?

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:520: if
(CommandLine::ForCurrentProcess()->HasSwitch(
Should this not be occurring for ChromeOS?

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1414: store_->SetClearLocalStateOnExit(false);
...

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1424: *it->second, false,
CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT);
...

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1434: store_->DeleteSessionCookies();
Won't this also delete any new session cookies set for domains visited while the
restore prompt was up?

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1779: // contains no cookies marked as session
cookies.
'as _old_ session cookies'

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1781: continue;
I think we should also be deleting this cookie (from the Cookie Monster and the
SQLitePersistentCookieStore) here.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1928: // We're setting a cookie, and there are
session cookies for which we
On 2011/11/28 15:47:19, jochen wrote:
> I think we should also delete all session cookies for a key when we read
cookies
> for that key, wdyt?

Also, this should occur before the DeleteAnyEquivalentCookie statement. A
pre-existing HTTPONLY session cookie would alter the behaviour there.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1932: // marked as session cookies.
'as _old_ session cookies'

[erikwright (departed), 2011-11-28 16:31:44]

Can you state for the record what the intended behaviour is in the following
scenario:

1. Session restore is enabled
2. Browser crash
3. Launch browser, restore session prompt is displayed
4. The browser crashes again before accepting the prompt
5. Launch browser, ...?

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1414: store_->SetClearLocalStateOnExit(false);
On 2011/11/28 16:18:16, erikwright wrote:
> ...

Never mind, I've answered my own question.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1424: *it->second, false,
CookieMonster::Delegate::CHANGE_COOKIE_EXPLICIT);
On 2011/11/28 16:18:16, erikwright wrote:
> ...

I don't know what I was planning to comment on here.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1434: store_->DeleteSessionCookies();
On 2011/11/28 16:18:16, erikwright wrote:
> Won't this also delete any new session cookies set for domains visited while
the
> restore prompt was up?

I suppose one solution is to pass a timestamp (i.e., the time that loading
started) and only delete the ones not created/modified since then.

Another solution is to rely on deletion during InternalDeleteCookie (i.e., pass
true for sync_to_store).

[marja, 2011-11-29 12:56:01]

Hi,

Thanks for comments!

As discussed, the latest patch set changes the approach: Now we always restore
after a crash (don't wait for the user to click "restore" on the info bar), so
there is no unclear state where we don't yet know if we want to restore. This
makes the code a lot simpler.

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:640: sql::Transaction
transaction(db_.get());
On 2011/11/28 16:18:16, erikwright wrote:
> Please add a metric, recorded and reported here, for the duration of the
> migration (i.e., from line 640 to 654). Name it
> "Cookie.TimeDatabaseMigrationToV5".

Done.

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:644: "ADD COLUMN
has_expires INTEGER DEFAULT 1") ||
On 2011/11/28 16:18:16, erikwright wrote:
> I don't see any documentation that states that the DEFAULT value here will
> actually be applied to existing rows, although it does seem to be implied.
> 
> Also, the definition for migrated tables will differ from that of non-migrated
> tables (the former have a DEFAULT, the latter do not).
> 
> 1) Please confirm experimentally that the DEFAULT in the ALTER TABLE updates
the
> value of existing rows.
> 2) If it does not, remove the DEFAULT and add an UPDATE statement here to
apply
> the value.
> 3) If it does, also include the default in the CREATE TABLE statement.

Done. (1 & 3)

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl_io_data.cc:327: if
(command_line.HasSwitch(switches::kEnableRestoreSessionState))
On 2011/11/28 16:18:16, erikwright wrote:
> Is it fair to say that this flag would eventually be removed, and we would
> always do this?

Afaik yes. (Added "experimental" to the comment describing the flag.)

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8533013/diff/57001/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:520: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/11/28 16:18:16, erikwright wrote:
> Should this not be occurring for ChromeOS?

Made it occur also for ChromeOS.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1434: store_->DeleteSessionCookies();
On 2011/11/28 16:31:44, erikwright wrote:
> On 2011/11/28 16:18:16, erikwright wrote:
> > Won't this also delete any new session cookies set for domains visited while
> the
> > restore prompt was up?
> 
> I suppose one solution is to pass a timestamp (i.e., the time that loading
> started) and only delete the ones not created/modified since then.
> 
> Another solution is to rely on deletion during InternalDeleteCookie (i.e.,
pass
> true for sync_to_store).

Yep, it was buggy. The change of approach (always restore after a crash, don't
wait for the infobar) removes this problem.

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1779: // contains no cookies marked as session
cookies.
On 2011/11/28 16:18:16, erikwright wrote:
> 'as _old_ session cookies'

(Obsolete)

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1781: continue;
On 2011/11/28 16:18:16, erikwright wrote:
> I think we should also be deleting this cookie (from the Cookie Monster and
the
> SQLitePersistentCookieStore) here.

(Obsolete)

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1928: // We're setting a cookie, and there are
session cookies for which we
On 2011/11/28 15:47:19, jochen wrote:
> I think we should also delete all session cookies for a key when we read
cookies
> for that key, wdyt?

(Obsolete.)

http://codereview.chromium.org/8533013/diff/57001/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:1932: // marked as session cookies.
On 2011/11/28 16:18:16, erikwright wrote:
> 'as _old_ session cookies'

(Obsolete.)

[jochen (gone - plz use gerrit), 2011-11-29 13:20:55]

much nicer!

Now a test would make this super-awesome :)

[erikwright (departed), 2011-11-29 13:43:22]

It's looking pretty good. Yes, some tests would probably be the next step.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:848: void
SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnDBThread() {
I would move this up next to the other methods that execute SQL statements,
remove the 'OnDBThread' from the name (to match the others) and remove the check
on 'initialized_'. At this point, it is only a helper method with a single
caller, so it can be nice and simple.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:27: explicit
SQLitePersistentCookieStore(const FilePath& path,
Don't need 'explicit' anymore.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1576:
make_scoped_refptr(GetRequestContext())));
I'm under the impression that Bind automatically increments reference counted
objects passed via raw pointers. Try removing make_scoped_refptr.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:519: }
Add a blank line.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:526: 
Remove blank line.

[marja, 2011-11-29 14:58:20]

Tests added, too. (Unit tests for CookieMonster & SQLPersistentCookieStore.)

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:848: void
SQLitePersistentCookieStore::Backend::DeleteSessionCookiesOnDBThread() {
On 2011/11/29 13:43:22, erikwright wrote:
> I would move this up next to the other methods that execute SQL statements,
> remove the 'OnDBThread' from the name (to match the others) and remove the
check
> on 'initialized_'. At this point, it is only a helper method with a single
> caller, so it can be nice and simple.

Done. (Ah, true, this was leftover code.)

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:27: explicit
SQLitePersistentCookieStore(const FilePath& path,
On 2011/11/29 13:43:22, erikwright wrote:
> Don't need 'explicit' anymore.

Done.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:1576:
make_scoped_refptr(GetRequestContext())));
On 2011/11/29 13:43:22, erikwright wrote:
> I'm under the impression that Bind automatically increments reference counted
> objects passed via raw pointers. Try removing make_scoped_refptr.

If I remove it, the compile-time assert
p1_is_refcounted_type_and_needs_scoped_refptr fires.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:519: }
On 2011/11/29 13:43:22, erikwright wrote:
> Add a blank line.

Done.

http://codereview.chromium.org/8533013/diff/62029/chrome/browser/ui/browser_l...
chrome/browser/ui/browser_list.cc:526: 
On 2011/11/29 13:43:22, erikwright wrote:
> Remove blank line.

Done.

[jochen (gone - plz use gerrit), 2011-11-30 12:12:17]

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:235: static const int
kCurrentVersionNumber = 5;
can you add a comment what changed for version 5 and why it's not backwards
compatible?

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:385: new
base::ThreadTestHelper(
fits on last line?

http://codereview.chromium.org/8533013/diff/68001/net/base/cookie_monster_uni...
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/8533013/diff/68001/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:3588: cm->SetPersistSessionCookies(true);
what happens if you set this option after there is already a session cookie in
the monster?

[marja, 2011-11-30 12:57:48]

Thanks for comments!

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:235: static const int
kCurrentVersionNumber = 5;
On 2011/11/30 12:12:17, jochen wrote:
> can you add a comment what changed for version 5 and why it's not backwards
> compatible?

Done.

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc (right):

http://codereview.chromium.org/8533013/diff/68001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:385: new
base::ThreadTestHelper(
On 2011/11/30 12:12:17, jochen wrote:
> fits on last line?

Done.

http://codereview.chromium.org/8533013/diff/68001/net/base/cookie_monster_uni...
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/8533013/diff/68001/net/base/cookie_monster_uni...
net/base/cookie_monster_unittest.cc:3588: cm->SetPersistSessionCookies(true);
On 2011/11/30 12:12:17, jochen wrote:
> what happens if you set this option after there is already a session cookie in
> the monster?

Added DCHECK(!initialized_) to SetPersistSessionCookies.

[jochen (gone - plz use gerrit), 2011-11-30 13:04:34]

LGTM

[erikwright (departed), 2011-11-30 20:09:40]

A small test addition requested.

http://codereview.chromium.org/8533013/diff/68027/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/68027/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:779: add_smt.BindInt(9,
po->cc().DoesExpire());
Add tests that these two attributes are being properly persisted and restored
(i.e., non-session cookies come out with IsPersistent() == true, session cookies
opposite, likewise for DoesExpire...)

[marja, 2011-12-01 15:35:39]

http://codereview.chromium.org/8533013/diff/68027/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/8533013/diff/68027/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:779: add_smt.BindInt(9,
po->cc().DoesExpire());
On 2011/11/30 20:09:40, erikwright wrote:
> Add tests that these two attributes are being properly persisted and restored
> (i.e., non-session cookies come out with IsPersistent() == true, session
cookies
> opposite, likewise for DoesExpire...)

Done.

[erikwright (departed), 2011-12-01 17:39:29]

LGTM!

[commit-bot: I haz the power, 2011-12-02 08:15:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8533013/79016

[commit-bot: I haz the power, 2011-12-02 08:15:27]

Presubmit check for 8533013-79016 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile_impl_io_data.cc,chrome/browser/profiles/profile.h,chrome/browser/profiles/profile_impl_io_data.h,chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc,chrome/browser/profiles/profile_impl.h,chrome/browser/net/sqlite_persistent_cookie_store.cc,chrome/browser/net/sqlite_persistent_cookie_store.h,chrome/browser/profiles/profile_impl.cc

Presubmit checks took 2.7s to calculate.

[marja, 2011-12-02 08:22:27]

Oops, OWNERS check.

mirandac, could you review
chrome/browser/profiles/*

rdsmith, could you review / rubberstamp erikwrights review for
chrome/browser/net/*

Thanks & {jochen,erikwright}, thanks for review!

[Randy Smith (Not in Mondays), 2011-12-02 16:16:28]

Rubberstamp LGTM chrome/browser/net.

[Miranda Callahan, 2011-12-02 17:13:09]

LGTM with added DCHECK.

http://codereview.chromium.org/8533013/diff/84002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8533013/diff/84002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:173:
url_request_context_getter->GetURLRequestContext()->cookie_store()->
Could you add a DCHECK at the start of this method to make sure we're on the IO
Thread?

[marja, 2011-12-05 08:40:16]

Thanks for review!

http://codereview.chromium.org/8533013/diff/84002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8533013/diff/84002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:173:
url_request_context_getter->GetURLRequestContext()->cookie_store()->
On 2011/12/02 17:13:10, Miranda Callahan wrote:
> Could you add a DCHECK at the start of this method to make sure we're on the
IO
> Thread?

Done.

[commit-bot: I haz the power, 2011-12-05 08:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/8533013/86001

[commit-bot: I haz the power, 2011-12-05 10:40:45]

Change committed as 112970