Session-only local storage cleared on exit.

Session-only local storage cleared on exit.


BUG=47049
TEST=DOMStorageTest.SessionOnly


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

[jochen (gone - plz use gerrit), 2011-08-17 11:29:06]

LGTM with nits

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_special_storage_policy.cc:11: #include
"chrome/browser/content_settings/host_content_settings_map.h"
alphabetical ordering

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_special_storage_policy.cc:32: ContentSetting
content_setting = host_content_settings_map_->
since host_content_settings_map_ can be NULL, you should check for this (and
probably return false then)

[marja, 2011-08-17 11:51:16]

Thanks for comments.

Michael, could you have a look whether the approach of this CL is ok? (If so,
I'll add some tests here.)

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_special_storage_policy.cc:11: #include
"chrome/browser/content_settings/host_content_settings_map.h"
On 2011/08/17 11:29:06, jochen wrote:
> alphabetical ordering

Done.

http://codereview.chromium.org/7619010/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_special_storage_policy.cc:32: ContentSetting
content_setting = host_content_settings_map_->
On 2011/08/17 11:29:06, jochen wrote:
> since host_content_settings_map_ can be NULL, you should check for this (and
> probably return false then)

Done.

[michaeln, 2011-08-17 19:16:40]

LGTM2

http://codereview.chromium.org/7619010/diff/8001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/7619010/diff/8001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_context.cc:201: void
DOMStorageContext::DeleteSessionOnlyData() {
maybe DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));

http://codereview.chromium.org/7619010/diff/8001/webkit/quota/mock_special_st...
File webkit/quota/mock_special_storage_policy.h (right):

http://codereview.chromium.org/7619010/diff/8001/webkit/quota/mock_special_st...
webkit/quota/mock_special_storage_policy.h:48: file_handlers_.clear();
also reset session_only_ here and can u reset all_unlimited_ to false as well?

[jochen (gone - plz use gerrit), 2011-08-18 11:20:26]

some more comments. I don't think this needs to be fixed in this CL, but we
should keep it in mind

http://codereview.chromium.org/7619010/diff/4004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/7619010/diff/4004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:36:
GetCookieContentSetting(origin, origin, true);
btw, I think we still need to pass in both URLs (eventually):

a valid combination would be that third-party storage is blocked, and first
party is session only

In this case this doesn't really matter that much because all later access to
dom storage checks the content settings again.

[marja, 2011-08-18 11:22:14]

Did the small changes suggested by michaeln.

http://codereview.chromium.org/7619010/diff/8001/content/browser/in_process_w...
File content/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/7619010/diff/8001/content/browser/in_process_w...
content/browser/in_process_webkit/dom_storage_context.cc:201: void
DOMStorageContext::DeleteSessionOnlyData() {
On 2011/08/17 19:16:41, michaeln wrote:
> maybe DCHECK(BrowserThread::CurrentlyOn(BrowserThread::WEBKIT));

Done.

http://codereview.chromium.org/7619010/diff/8001/webkit/quota/mock_special_st...
File webkit/quota/mock_special_storage_policy.h (right):

http://codereview.chromium.org/7619010/diff/8001/webkit/quota/mock_special_st...
webkit/quota/mock_special_storage_policy.h:48: file_handlers_.clear();
On 2011/08/17 19:16:41, michaeln wrote:
> also reset session_only_ here and can u reset all_unlimited_ to false as well?

Done.

[marja, 2011-08-18 11:59:01]

http://codereview.chromium.org/7619010/diff/4004/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/7619010/diff/4004/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_special_storage_policy.cc:36:
GetCookieContentSetting(origin, origin, true);
On 2011/08/18 11:20:26, jochen wrote:
> btw, I think we still need to pass in both URLs (eventually):
> 
> a valid combination would be that third-party storage is blocked, and first
> party is session only

Aren't the "storage blocked" cases checked elsewhere in the code, before this
part of code is reached?

StorageAreaProxy::canAccessStorage -> ChromeRenderViewObserver::AllowStorage ->
ContentSettingsObserver::AllowStorage ->
ChromeRenderMessageFilter::OnAllowDOMStorage -> HostContentSettingsMap.

[marja, 2011-08-18 18:36:36]

Fyi, I had an illegal #include in the tests;
content/browser/in_process_webkit/dom_storage_unittest.cc included
chrome/browser/extensions/mock_extension_special_storage_policy.h. The latest
patch set fixes it.

[michaeln, 2011-08-18 21:34:35]

On 2011/08/18 18:36:36, marja wrote:
> Fyi, I had an illegal #include in the tests;
> content/browser/in_process_webkit/dom_storage_unittest.cc included
> chrome/browser/extensions/mock_extension_special_storage_policy.h. The latest
> patch set fixes it.

right... still lgtm!

[commit-bot: I haz the power, 2011-08-19 06:54:08]

Presubmit check for 7619010-7034 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Presubmit checks took 1.5s to calculate.

[marja, 2011-08-19 06:59:52]

Oops, missing the OWNERS again.

Miranda, could you review the parts affecting the profiles
(chrome/browser/profiles/profile_impl.cc ? Thanks!

[Miranda Callahan, 2011-08-19 11:11:10]

LGTM with nit.

http://codereview.chromium.org/7619010/diff/7034/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7619010/diff/7034/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:833: if
(!extension_special_storage_policy_.get())
nit: add { } around this if clause, as it now extends over two lines.

[marja, 2011-08-19 11:23:45]

Thanks for the comment.

http://codereview.chromium.org/7619010/diff/7034/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7619010/diff/7034/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:833: if
(!extension_special_storage_policy_.get())
On 2011/08/19 11:11:10, Miranda Callahan wrote:
> nit: add { } around this if clause, as it now extends over two lines.

Done.

[commit-bot: I haz the power, 2011-08-19 17:07:13]

Change committed as 97469