Unwire the clear on exit preference from the storage systems.

Unwire the clear on exit preference from the storage systems.

The "session only" rules should cover the functionality now

UI changes and migration code will follow

BUG=129349
TEST=added unit tests for the chrome/browser/net/sqlite* classes

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140502

[jochen (gone - plz use gerrit), 2012-05-31 14:45:18]

Please review:

Matt, ServerBoundCert stuff
Will, chrome/browser/profiles/ and net/ approval
Michael,  webkit/, */dom_storage, and in_process_webkit
Erik, cookies

Marja, overall logic
Bernhard, fyi, this touches the plugins logic

[marja, 2012-05-31 15:30:32]

I didn't yet finish reading through it all, but...

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:920: return setting !=
CONTENT_SETTING_SESSION_ONLY;
What if the default setting is .._BLOCK?

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_persistent_cookie_store.h:48: virtual void
SetKeepSessionCookies() OVERRIDE;
Isn't this the same as SaveSessionState in other storage systems? Could it also
be named SaveSessionState?

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_server_bound_cert_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_server_bound_cert_store_unittest.cc:530:
temp_dir_.path().Append(chrome::kOBCertFilename), NULL);
<unrelated whine>Instead of deleting the previous data, can't this test just use
a separate temp dir?</unrelated whine>

[jochen (gone - plz use gerrit), 2012-05-31 15:54:40]

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:920: return setting !=
CONTENT_SETTING_SESSION_ONLY;
On 2012/05/31 15:30:32, marja wrote:
> What if the default setting is .._BLOCK?

It's a bug that this doesn't keep flash from saving LSOs - but it's already now
a bug, and Bernhard suggested to keep the bug for now

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_persistent_cookie_store.h:48: virtual void
SetKeepSessionCookies() OVERRIDE;
On 2012/05/31 15:30:32, marja wrote:
> Isn't this the same as SaveSessionState in other storage systems? Could it
also
> be named SaveSessionState?

sure. Erik, wdyt?

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_server_bound_cert_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_server_bound_cert_store_unittest.cc:530:
temp_dir_.path().Append(chrome::kOBCertFilename), NULL);
On 2012/05/31 15:30:32, marja wrote:
> <unrelated whine>Instead of deleting the previous data, can't this test just
use
> a separate temp dir?</unrelated whine>
 
will do

[Bernhard Bauer, 2012-05-31 16:08:16]

If the UI isn't updated yet, does that mean there will be an interval where the
checkbox won't have any effect?

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:918: ContentSetting setting =
cookie_settings->GetDefaultCookieSetting(NULL);
Ooh, now that we're hooking this up to content settings, we could actually ask
for a setting for a specific origin! That would require a bit more plumbing
though. Could you add a TODO(bauerb) for that?

[jochen (gone - plz use gerrit), 2012-05-31 16:13:40]

On 2012/05/31 16:08:16, Bernhard Bauer wrote:
> If the UI isn't updated yet, does that mean there will be an interval where
the
> checkbox won't have any effect?
> 

Markus promised me to do the UI and migration until end of next week. I'll delay
landing this CL until he's done.

>
https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/chrome_...
> chrome/browser/chrome_content_browser_client.cc:918: ContentSetting setting =
> cookie_settings->GetDefaultCookieSetting(NULL);
> Ooh, now that we're hooking this up to content settings, we could actually ask
> for a setting for a specific origin! That would require a bit more plumbing
> though. Could you add a TODO(bauerb) for that?
sure

[Bernhard Bauer, 2012-05-31 16:33:52]

On 2012/05/31 16:13:40, jochen wrote:
> On 2012/05/31 16:08:16, Bernhard Bauer wrote:
> > If the UI isn't updated yet, does that mean there will be an interval where
> the
> > checkbox won't have any effect?
> > 
> 
> Markus promised me to do the UI and migration until end of next week. I'll
delay
> landing this CL until he's done.

OK, cool.

[erikwright (departed), 2012-05-31 18:02:02]

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_persistent_cookie_store.h:48: virtual void
SetKeepSessionCookies() OVERRIDE;
On 2012/05/31 15:54:41, jochen wrote:
> On 2012/05/31 15:30:32, marja wrote:
> > Isn't this the same as SaveSessionState in other storage systems? Could it
> also
> > be named SaveSessionState?
> 
> sure. Erik, wdyt?

I think I prefer it having the 'Set' in the name.

After all, it's not saving _anything_ now, just setting some state for later.

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_server_bound_cert_store.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_server_bound_cert_store.h:39: virtual void
SaveSessionState() OVERRIDE;
Again, I'd prefer the name include 'Set'.

https://chromiumcodereview.appspot.com/10447117/diff/1/net/cookies/cookie_mon...
File net/cookies/cookie_monster.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/net/cookies/cookie_mon...
net/cookies/cookie_monster.h:196: void SaveSessionCookies();
Let this start with Set, and match the name in the PersistentCookieStore.

I'm happy if that also matches the other stores.

[willchan no longer on Chromium, 2012-05-31 18:51:12]

chrome/browser/profile/ LGTM and net/ and chrome/browser/net non-cookie/cert
specific portions LGTM too.

[mattm, 2012-05-31 21:57:11]

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/chrome/browser/net/sql...
chrome/browser/net/sqlite_server_bound_cert_store.cc:117: bool
save_session_state_;
This variable name (and function that sets it) is kind of misleading,  Maybe
something like override_clear_on_exit_ or force_save_session_state_on_exit_ ?

[michaeln, 2012-06-01 02:06:16]

lgtm w a couple of nits

https://chromiumcodereview.appspot.com/10447117/diff/1/content/browser/browse...
File content/browser/browser_context.cc (left):

https://chromiumcodereview.appspot.com/10447117/diff/1/content/browser/browse...
content/browser/browser_context.cc:217:
base::Bind(&appcache::AppCacheService::set_clear_local_state_on_exit,
Can this method and associated stuff also be removed from the AppCacheService
class and AppCacheStorageImpl?

https://chromiumcodereview.appspot.com/10447117/diff/1/content/browser/in_pro...
File content/browser/in_process_webkit/indexed_db_context_impl.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/content/browser/in_pro...
content/browser/in_process_webkit/indexed_db_context_impl.cc:65: void
ClearLocalState(
ditto, name is odd now

https://chromiumcodereview.appspot.com/10447117/diff/1/webkit/database/databa...
File webkit/database/database_tracker.h (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/webkit/database/databa...
webkit/database/database_tracker.h:208: void ClearLocalState();
ditto name is off now

https://chromiumcodereview.appspot.com/10447117/diff/1/webkit/dom_storage/dom...
File webkit/dom_storage/dom_storage_context.cc (right):

https://chromiumcodereview.appspot.com/10447117/diff/1/webkit/dom_storage/dom...
webkit/dom_storage/dom_storage_context.cc:145:
base::Bind(&DomStorageContext::ClearLocalStateInCommitSequence, this));
nit: the name of the ClearLocalStateInCommitSequence method seems off now, maybe
rename to ClearSessionOnlyOrigins?

[jochen (gone - plz use gerrit), 2012-06-01 12:15:18]

all comments addressed

http://codereview.chromium.org/10447117/diff/1/chrome/browser/chrome_content_...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10447117/diff/1/chrome/browser/chrome_content_...
chrome/browser/chrome_content_browser_client.cc:918: ContentSetting setting =
cookie_settings->GetDefaultCookieSetting(NULL);
On 2012/05/31 16:08:17, Bernhard Bauer wrote:
> Ooh, now that we're hooking this up to content settings, we could actually ask
> for a setting for a specific origin! That would require a bit more plumbing
> though. Could you add a TODO(bauerb) for that?

Done.

http://codereview.chromium.org/10447117/diff/1/chrome/browser/net/sqlite_pers...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):

http://codereview.chromium.org/10447117/diff/1/chrome/browser/net/sqlite_pers...
chrome/browser/net/sqlite_persistent_cookie_store.h:48: virtual void
SetKeepSessionCookies() OVERRIDE;
On 2012/05/31 18:02:02, erikwright wrote:
> On 2012/05/31 15:54:41, jochen wrote:
> > On 2012/05/31 15:30:32, marja wrote:
> > > Isn't this the same as SaveSessionState in other storage systems? Could it
> > also
> > > be named SaveSessionState?
> > 
> > sure. Erik, wdyt?
> 
> I think I prefer it having the 'Set' in the name.
> 
> After all, it's not saving _anything_ now, just setting some state for later.

I called it SetForceKeepSessionState

http://codereview.chromium.org/10447117/diff/1/content/browser/browser_contex...
File content/browser/browser_context.cc (left):

http://codereview.chromium.org/10447117/diff/1/content/browser/browser_contex...
content/browser/browser_context.cc:217:
base::Bind(&appcache::AppCacheService::set_clear_local_state_on_exit,
On 2012/06/01 02:06:17, michaeln wrote:
> Can this method and associated stuff also be removed from the AppCacheService
> class and AppCacheStorageImpl?

Done.

http://codereview.chromium.org/10447117/diff/1/webkit/dom_storage/dom_storage...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/10447117/diff/1/webkit/dom_storage/dom_storage...
webkit/dom_storage/dom_storage_context.cc:145:
base::Bind(&DomStorageContext::ClearLocalStateInCommitSequence, this));
On 2012/06/01 02:06:17, michaeln wrote:
> nit: the name of the ClearLocalStateInCommitSequence method seems off now,
maybe
> rename to ClearSessionOnlyOrigins?

Done.

http://codereview.chromium.org/10447117/diff/1/webkit/dom_storage/dom_storage...
webkit/dom_storage/dom_storage_context.cc:145:
base::Bind(&DomStorageContext::ClearLocalStateInCommitSequence, this));
On 2012/06/01 02:06:17, michaeln wrote:
> nit: the name of the ClearLocalStateInCommitSequence method seems off now,
maybe
> rename to ClearSessionOnlyOrigins?

Done.

[marja, 2012-06-01 12:45:28]

lgtm with nits:

http://codereview.chromium.org/10447117/diff/8005/content/browser/in_process_...
File content/browser/in_process_webkit/indexed_db_context_impl.cc (right):

http://codereview.chromium.org/10447117/diff/8005/content/browser/in_process_...
content/browser/in_process_webkit/indexed_db_context_impl.cc:76: if
(!special_storage_policy->IsStorageSessionOnly(*iter))
Same here (see below).

http://codereview.chromium.org/10447117/diff/8005/webkit/database/database_tr...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/10447117/diff/8005/webkit/database/database_tr...
webkit/database/database_tracker.cc:835: if (special_storage_policy_.get() &&
Cleanup nit:

On the top of the function we now do an early return if
!special_storage_policy.get().

So this special_storage_policy_.get() can now be removed. (It is anyway accessed
without any checks 2 lines above.)

http://codereview.chromium.org/10447117/diff/8005/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/10447117/diff/8005/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:222: if (special_storage_policy_ &&
Same here, special_storage_policy cannot be NULL.

(Or at least the checks should be consistent; now the first if checks the
nullity but the second doesn't.)

[jochen (gone - plz use gerrit), 2012-06-01 12:58:08]

all done

[erikwright (departed), 2012-06-01 15:17:17]

*cookie* LGTM.

http://codereview.chromium.org/10447117/diff/17001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/10447117/diff/17001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_persistent_cookie_store.cc:1020: void
SQLitePersistentCookieStore::SetForceKeepSessionState() {
I gather that, by design, one would never _un_set force-keep-session?

[jochen (gone - plz use gerrit), 2012-06-01 15:57:20]

Right. In practice, we set this flag right before shutdown
On Jun 1, 2012 5:17 PM, <erikwright@chromium.org> wrote:

> *cookie* LGTM.
>
>
> http://codereview.chromium.**org/10447117/diff/17001/**
>
chrome/browser/net/sqlite_**persistent_cookie_store.cc<http://codereview.chromium.org/10447117/diff/17001/chrome/browser/net/sqlite_persistent_cookie_store.cc>
> File chrome/browser/net/sqlite_**persistent_cookie_store.cc (right):
>
> http://codereview.chromium.**org/10447117/diff/17001/**
>
chrome/browser/net/sqlite_**persistent_cookie_store.cc#**newcode1020<http://codereview.chromium.org/10447117/diff/17001/chrome/browser/net/sqlite_persistent_cookie_store.cc#newcode1020>
> chrome/browser/net/sqlite_**persistent_cookie_store.cc:**1020: void
> SQLitePersistentCookieStore::**SetForceKeepSessionState() {
> I gather that, by design, one would never _un_set force-keep-session?
>
>
http://codereview.chromium.**org/10447117/<http://codereview.chromium.org/104...
>

[mattm, 2012-06-01 21:15:01]

server_bound_cert stuff LGTM

[jochen (gone - plz use gerrit), 2012-06-02 19:56:28]

+avi for content/ approval

[Avi (use Gerrit), 2012-06-04 14:49:11]

content rubber stamp lgtm