Refactored cookies persistent store clean-up on shutdown.

Refactored cookies persistent store clean-up on shutdown.

Changed the static call of SQLitePersistantCookieStore::ClearLocalState to
a call of a virtual method PersistantCookieStore::ClearLocalState from
inside the destruction sequence of the owning UrlRequestContext.

Thus the code will now work with any other persistancy implementation and
allow for better parallelism.

To test. Enable deleting of cookies and user data on shutdown and check if
the Cookies file in the profile directory has been deleted.

BUG=64920
TEST=Manual.

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

[pastarmovj, 2010-12-01 16:53:59]

Please review.

[Mattias Nissler (ping if slow), 2010-12-01 17:23:24]

Looks pretty good, just a few questions/comments. As I'm not intimately familiar
with this part of the code, please wait for Jochen to comment.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:890: {
brace goes on previous line.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:444:
file_util::Delete(path_, false);
What's the contract on the question which thread this is safe to call on? Is
there a policy for PersistentCookieStore? If so, should we do an explicit switch
to the FILE thread (if necessary) here?

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.h:38: virtual void
ClearLocalState();
Could use a comment :) I'm aware this class doesn't have many comments, but it
doesn't have to stay that way.

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

http://codereview.chromium.org/5430004/diff/2001/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:209: PersistentCookieStore* GetPersistentCookieStore()
{
We might want to consider to not provide the getter here, but to provide a
ClearLocalState() instead that delegates to ClearLocalState() on the store.
WDYT?

[Randy Smith (Not in Mondays), 2010-12-01 20:39:49]

For understanding this change, it would help me to understand the use cases for
kClearSiteDataOnExit.  Is this 
just a way with slightly different corner cases for the user to say "make all my
cookies be session cookies"?  Or is it targeted at some usage pattern I don't
yet understand?  My difficulty is that my ideal implementation of "make all my
cookies session cookies" is not to construct a cookie monster with a store to
begin with, and I want to make sure that that doesn't address the relevant use
case before I give up on it.  But I'll proactively give up on that
implementation for the rest of this review :-}.

I'll comment up front that sqlite_persistent_cookie_store.* has some real
problems, and should be cleaned up at some point.  Having said that, the right
place for this functionality strikes me as being in
SQLitePersistentCookieStore::Backend::InternalBackgroundClose().  That implies
that what needs to happen is plumbing through SQLitePersistentCookieStore and
...::Backend to set a flag indicating that the store should be deleted on
destruction.  

In terms of how that flag gets set, I'm torn.  I don't like the addition of
controls for the persistent store to CookieMonster; until this point,
CookieMonster has been an encapsulation of a CookieStore that may or may not
have a persistent store behind it, but the behavior exported is the same either
way--that difference is just specified at creation.  If the CookieMonster is
considered to own the cookie store, it's appropriate to put a persistent store
control in it ({set/clear}DeletePersistentStoreOnClose or something); if the
CookieMonster is just consider a (== the only :-}) client of the persistent
store, it would be more appropriate to keep a pointer to the cookie store in the
URLRequestContext, and tweak that pointer directly if the user changes this
preference.  I'm ok with (i.e. don't particularly like :-}) either choice--I can
see arguments in both directions.  But I'd rather avoid the "detach store"
interface that's currently in the cookie monster; that seems like an invitation
to database inconsistency if used improperly by clients.

Other minor comments follow; they may be rendered moot by the above.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:49: using net::CookieMonster;
The Chrome style guide forbids "using" directives.  Instead,
you always specify the namespace/class at point of call.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:552: }
It looks to me as if the other url request context related destruction work
occurs in the ChromeURLRequestContext destructor, not in the getter destructor. 
Is there a reason you didn't do this there?

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:223: bool
clear_local_state_on_exit_;
It didn't look to me as if you were initializing this variable in the
constructors (either default or copy); did I miss that?  Even if
ChromeURLRequestContextGetter sets it immediately after creation, it should be
created with a good default value.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:444:
file_util::Delete(path_, false);
On 2010/12/01 17:23:24, Mattias Nissler wrote:
> What's the contract on the question which thread this is safe to call on? Is
> there a policy for PersistentCookieStore? If so, should we do an explicit
switch
> to the FILE thread (if necessary) here?

This file's somewhat messy WRT thread access.  The basic flow is that
SQLitePersistentCookieStore accesses occur on the IO thread, and database
reads/writes are handed off to the backend, which primarily runs on the DB
thread.  But the initial open of the file appears to occur on the IO thread (in
Load()), which is a no-no.  And, of course, this access was (if I'm reading the
code right) happening on the IO thread before this change.  In an ideal world,
we'd proxy the request over to the FILE thread or (preferably, to reduce the
number of threads the code runs on) the DB thread through the Backend.  But I
wouldn't hold the change hostage to that, as this isn't a regression, but it
isn't correct as (originally) written.

[jochen (gone - plz use gerrit), 2010-12-02 08:45:18]

On 2010/12/01 20:39:49, rdsmith wrote:
> For understanding this change, it would help me to understand the use cases
for
> kClearSiteDataOnExit.  Is this 
> just a way with slightly different corner cases for the user to say "make all
my
> cookies be session cookies"?  Or is it targeted at some usage pattern I don't
> yet understand?  My difficulty is that my ideal implementation of "make all my
> cookies session cookies" is not to construct a cookie monster with a store to
> begin with, and I want to make sure that that doesn't address the relevant use
> case before I give up on it.  But I'll proactively give up on that
> implementation for the rest of this review :-}.


There's two differences to "make all cookies session cookies":

- the user can turn this option on and off (so you'd have to write them out, and
delete them again)
- it works for all site data, not just cookies

[jochen (gone - plz use gerrit), 2010-12-02 08:45:50]

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:38: #include
"chrome/browser/net/sqlite_persistent_cookie_store.h"
do you still need this?

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:205: // Clears the persistent
store if need.
needed

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:321: void
OnClearSiteDataOnExitChange(bool clear_site_data);
why not ClearLocalState and clear_local_state?

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.cc:444:
file_util::Delete(path_, false);
I think this should be done on the DB thread. Also, you need to make sure that
the sql file is actually closed before you delete it (otherwise it'll fail on
windows)

[pastarmovj, 2010-12-02 14:54:35]

@Jochen please review.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:38: #include
"chrome/browser/net/sqlite_persistent_cookie_store.h"
On 2010/12/02 08:45:50, jochen wrote:
> do you still need this?

Done.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:49: using net::CookieMonster;
On 2010/12/01 20:39:50, rdsmith wrote:
> The Chrome style guide forbids "using" directives.  Instead,
> you always specify the namespace/class at point of call.

Done.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:890: {
On 2010/12/01 17:23:24, Mattias Nissler wrote:
> brace goes on previous line.

Done.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:205: // Clears the persistent
store if need.
On 2010/12/02 08:45:50, jochen wrote:
> needed

Done.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:223: bool
clear_local_state_on_exit_;
On 2010/12/01 20:39:50, rdsmith wrote:
> It didn't look to me as if you were initializing this variable in the
> constructors (either default or copy); did I miss that?  Even if
> ChromeURLRequestContextGetter sets it immediately after creation, it should be
> created with a good default value.
> 

Done.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.h:321: void
OnClearSiteDataOnExitChange(bool clear_site_data);
On 2010/12/02 08:45:50, jochen wrote:
> why not ClearLocalState and clear_local_state?

To keep the naming convention of the other two functions that has the same names
as the pref keys they are responding to.

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/5430004/diff/2001/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.h:38: virtual void
ClearLocalState();
On 2010/12/01 17:23:24, Mattias Nissler wrote:
> Could use a comment :) I'm aware this class doesn't have many comments, but it
> doesn't have to stay that way.

Actually commented the class with a notice what it is and that the rest of the
documentation to the members should be sought in the base class header.

[jochen (gone - plz use gerrit), 2010-12-02 15:09:51]

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:321: 
nit no empty line plz

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:255:
clear_local_state_on_exit_(false) {
i'd push this even further down to the backend. in InternalBackgroundClose() you
can then delete the file

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:25: // Implements the
PersistentCookieStore interface in terms of SQLite database.
... in terms of a ... or ... using a ...

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:26: // For documentation of
the actually member functions consult the documentation
documentation about

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:11: #include
"googleurl/src/gurl.h"
alphabetical ordering

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:23:
db_thread_(BrowserThread::DB) {}
why no message loop for the db thread?

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:27:
db_thread_.Start();
that shouldn't be required

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:31:
ASSERT_TRUE(store_->Load(&cookies_));
in a new directory, there cannot be any cookies

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:37:
store_->SetClearLocalStateOnExit(true);
please don't add additional tests in tear down

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:57:
scoped_refptr<SQLitePersistentCookieStore> store_;
i think you should move the store into the single tests. then you also don't
have to replace it when you really want to delete it

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:59:
std::vector<net::CookieMonster::CanonicalCookie*> cookies_;
this is only used in one test, so I'd move this variable to that test (same for
ClearCookies)

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:106: ASSERT_TRUE(1
== cookies_.size());
ASSERT_EQ

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:108:
ASSERT_EQ("A", cookies_[0]->Name());
ASSERT_STREQ

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:122: ASSERT_TRUE(0
== cookies_.size());
ASSERT_EQ

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:125: 
no empty lines please

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

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:214: store_ = NULL;
still required?

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster_sto...
File net/base/cookie_monster_store_test.h (right):

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.h:21: REMOVE
please leave the ,

[pastarmovj, 2010-12-02 16:29:07]

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/chrome_u...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/chrome_u...
chrome/browser/net/chrome_url_request_context.cc:321: 
On 2010/12/02 15:09:51, jochen wrote:
> nit no empty line plz

Done.

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:255:
clear_local_state_on_exit_(false) {
On 2010/12/02 15:09:51, jochen wrote:
> i'd push this even further down to the backend. in InternalBackgroundClose()
you
> can then delete the file

The backend doesn't know the file name now and I don't see reason to copy it
there only for the sake of deleting it. I can anyhow ensure that nobody is still
using the file at the point I delete it. Moreover this class opens the file so
it is not precedent to do something with it here again.

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:25: // Implements the
PersistentCookieStore interface in terms of SQLite database.
On 2010/12/02 15:09:51, jochen wrote:
> ... in terms of a ... or ... using a ...

Done.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:26: // For documentation of
the actually member functions consult the documentation
On 2010/12/02 15:09:51, jochen wrote:
> documentation about

Done.

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

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:11: #include
"googleurl/src/gurl.h"
On 2010/12/02 15:09:51, jochen wrote:
> alphabetical ordering

You got me on that one :). Done!

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:23:
db_thread_(BrowserThread::DB) {}
On 2010/12/02 15:09:51, jochen wrote:
> why no message loop for the db thread?

I start the thread myself in SetUp. A message loop is being created then and we
don't need to touch it anyhow. We need separate threads for DB and !DB threads
because the code has both checks "are we on DB thread" and "are we NOT on DB
thread".

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:27:
db_thread_.Start();
On 2010/12/02 15:09:51, jochen wrote:
> that shouldn't be required

See previous comment.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:31:
ASSERT_TRUE(store_->Load(&cookies_));
On 2010/12/02 15:09:51, jochen wrote:
> in a new directory, there cannot be any cookies

The method returns true whenever the database has been successfully loaded or
created if not existing yet.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:37:
store_->SetClearLocalStateOnExit(true);
On 2010/12/02 15:09:51, jochen wrote:
> please don't add additional tests in tear down

Done. Moved it to a separate test.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:57:
scoped_refptr<SQLitePersistentCookieStore> store_;
On 2010/12/02 15:09:51, jochen wrote:
> i think you should move the store into the single tests. then you also don't
> have to replace it when you really want to delete it

Well still share this between all 2 (now 3) tests so I find it nicer to keep
common parts in base class. Makes it more tidy I think...

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:59:
std::vector<net::CookieMonster::CanonicalCookie*> cookies_;
On 2010/12/02 15:09:51, jochen wrote:
> this is only used in one test, so I'd move this variable to that test (same
for
> ClearCookies)

Actually we need it in the initialization call of every class instance no matter
if we do something with it or not.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:106: ASSERT_TRUE(1
== cookies_.size());
On 2010/12/02 15:09:51, jochen wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:108:
ASSERT_EQ("A", cookies_[0]->Name());
On 2010/12/02 15:09:51, jochen wrote:
> ASSERT_STREQ

Done.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:122: ASSERT_TRUE(0
== cookies_.size());
On 2010/12/02 15:09:51, jochen wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/5430004/diff/14001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:125: 
On 2010/12/02 15:09:51, jochen wrote:
> no empty lines please

Done.

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

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:214: store_ = NULL;
On 2010/12/02 15:09:51, jochen wrote:
> still required?

Nope sorry.

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster_sto...
File net/base/cookie_monster_store_test.h (right):

http://codereview.chromium.org/5430004/diff/14001/net/base/cookie_monster_sto...
net/base/cookie_monster_store_test.h:21: REMOVE
On 2010/12/02 15:09:51, jochen wrote:
> please leave the ,

Done.

[jochen (gone - plz use gerrit), 2010-12-02 16:48:43]

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:41: void
SetClearLocalStateOnExit(bool clear_local_state) {
virtual

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:54: // If true the
persistent store should be deleted upon destruction.
True if ..

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:23:
db_thread_(BrowserThread::DB) {}
nit. closing bracket on new line

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:27:
db_thread_.Start();
I meant that you can start the db_thread just like the ui_thread by explicitly
passing it the message_loop_. it's fine to use the same loop for several threads
in unit tests

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:29:
ASSERT_TRUE(store_ = new SQLitePersistentCookieStore(
either make it ASSERT(store_) in the next line and check everywhere where you
create a store, or (better) just leave the ASSERT away

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:32: ASSERT_TRUE(0
== cookies_.size());
ASSERT_EQ

I think it's good ok to have the store in the SetUp method, but I still think
you should get rid of cookies_ and ClearCookies. It's only used in a single
test, and you only need to call ClearCookies once

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:41: 
no newline

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:60: store_ = new
SQLitePersistentCookieStore(
store_ = NULL (so it's symmetrical to the test below)

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:105:
ClearCookies();
cookies_ should be empty here. no need to clear

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

http://codereview.chromium.org/5430004/diff/24001/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:212: return store_.get();
instead of this, add SetClearLocalStateOnExit() to the cookie monster that
relays to it's persistent cookie store if it exists

[Randy Smith (Not in Mondays), 2010-12-02 21:19:26]

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:272: &file_util::Delete,
path_, false));
I'm willing to accept this interface, but I think a better one would be simply
to communicate the clear_local_state_on_exit_ to the backend whenever it's
set/cleared, and let the backend InternalClose function do the right thing. 
It's already on the right thread, so you don't need to do any thread hopping.

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

http://codereview.chromium.org/5430004/diff/24001/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:212: return store_.get();
On 2010/12/02 16:48:44, jochen wrote:
> instead of this, add SetClearLocalStateOnExit() to the cookie monster that
> relays to it's persistent cookie store if it exists

Seconded; to the extent possible, I'd like to hide the existence/lack of
existence of a persistent store behind the cookie monster from consumers.

That change should also include adding a test in cookie_monster_unittest.cc to
test the interface (sends to store when there is one, does nothing when there
isn't).

[pastarmovj, 2010-12-03 16:43:01]

Should be mostly done now. 
Running the try bots meanwhile...

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:272: &file_util::Delete,
path_, false));
On 2010/12/02 21:19:26, rdsmith wrote:
> I'm willing to accept this interface, but I think a better one would be simply
> to communicate the clear_local_state_on_exit_ to the backend whenever it's
> set/cleared, and let the backend InternalClose function do the right thing. 
> It's already on the right thread, so you don't need to do any thread hopping.

Obsoleted because of the moved logic to Backend.

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:41: void
SetClearLocalStateOnExit(bool clear_local_state) {
On 2010/12/02 16:48:44, jochen wrote:
> virtual

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.h:54: // If true the
persistent store should be deleted upon destruction.
On 2010/12/02 16:48:44, jochen wrote:
> True if ..

Done.

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:23:
db_thread_(BrowserThread::DB) {}
On 2010/12/02 16:48:44, jochen wrote:
> nit. closing bracket on new line

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:27:
db_thread_.Start();
On 2010/12/02 16:48:44, jochen wrote:
> I meant that you can start the db_thread just like the ui_thread by explicitly
> passing it the message_loop_. it's fine to use the same loop for several
threads
> in unit tests

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:29:
ASSERT_TRUE(store_ = new SQLitePersistentCookieStore(
On 2010/12/02 16:48:44, jochen wrote:
> either make it ASSERT(store_) in the next line and check everywhere where you
> create a store, or (better) just leave the ASSERT away

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:32: ASSERT_TRUE(0
== cookies_.size());
On 2010/12/02 16:48:44, jochen wrote:
> ASSERT_EQ
> 
> I think it's good ok to have the store in the SetUp method, but I still think
> you should get rid of cookies_ and ClearCookies. It's only used in a single
> test, and you only need to call ClearCookies once

However I can't use the store if don't call the Load function which actually
creates the tables if they don't exist. And this call needs the cookies array. I
either have to duplicate it once for the start up function and once for the test
or just reuse it as a member variable as it is now.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:41: 
On 2010/12/02 16:48:44, jochen wrote:
> no newline

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:60: store_ = new
SQLitePersistentCookieStore(
On 2010/12/02 16:48:44, jochen wrote:
> store_ = NULL (so it's symmetrical to the test below)

Done.

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:105:
ClearCookies();
On 2010/12/02 16:48:44, jochen wrote:
> cookies_ should be empty here. no need to clear

Done.

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

http://codereview.chromium.org/5430004/diff/24001/net/base/cookie_monster.h#n...
net/base/cookie_monster.h:212: return store_.get();
On 2010/12/02 16:48:44, jochen wrote:
> instead of this, add SetClearLocalStateOnExit() to the cookie monster that
> relays to it's persistent cookie store if it exists

Done.

[pastarmovj, 2010-12-03 16:46:40]

Just a remark about obsoleted comment in my previos post.

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

http://codereview.chromium.org/5430004/diff/24001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store_unittest.cc:32: ASSERT_TRUE(0
== cookies_.size());
On 2010/12/03 16:43:02, pastarmovj wrote:
> On 2010/12/02 16:48:44, jochen wrote:
> > ASSERT_EQ
> > 
> > I think it's good ok to have the store in the SetUp method, but I still
think
> > you should get rid of cookies_ and ClearCookies. It's only used in a single
> > test, and you only need to call ClearCookies once
> 
> However I can't use the store if don't call the Load function which actually
> creates the tables if they don't exist. And this call needs the cookies array.
I
> either have to duplicate it once for the start up function and once for the
test
> or just reuse it as a member variable as it is now.

Sorry I posted an obsoleted comment here ignore it.

[Randy Smith (Not in Mondays), 2010-12-03 18:42:27]

LGTM with one final request.

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

http://codereview.chromium.org/5430004/diff/48001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:474:
backend_->SetClearLocalStateOnExit(clear_local_state);
Could you protect this with the lock (and modify the comment on the lock to
indicate it protects this field as well)?  It shouldn't matter--boolean accesses
should be atomic so you don't really care who wins the possible races, and I
can't come up with a case in which the race would actually happen--but I'd
rather be extra careful around thread safety in chrome since most code is
single-threaded and hence most developers aren't used to thinking about thread
safety in detail.

[pastarmovj, 2010-12-06 08:57:40]

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

http://codereview.chromium.org/5430004/diff/48001/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:474:
backend_->SetClearLocalStateOnExit(clear_local_state);
On 2010/12/03 18:42:27, rdsmith wrote:
> Could you protect this with the lock (and modify the comment on the lock to
> indicate it protects this field as well)?  It shouldn't matter--boolean
accesses
> should be atomic so you don't really care who wins the possible races, and I
> can't come up with a case in which the race would actually happen--but I'd
> rather be extra careful around thread safety in chrome since most code is
> single-threaded and hence most developers aren't used to thinking about thread
> safety in detail.

Done. I had exactly the same thoughts and didn't put the lock there because all
calls to this function are routed through the same thread. However maybe it's
more failsafe this way anyhow and I don't see potential for deadlocks here
either.