DomStorage data deletion and memory purging.

DomStorage data deletion and memory purging.

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

[michaeln, 2012-03-21 19:29:40]

hi josh, this one adds the Purge and Delete functions to DomStorage that chrome
calls on... code is there, i'm working up unittests now

[jsbell, 2012-03-21 20:26:28]

lgtm with nits

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:58: // Frees up memory when possible.
Typcically, this method returns
typo "Typcically"

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_context.h (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:97: void
GetUsageInfo(std::vector<UsageInfo>* infos, bool get_file_info_too);
Naming: suggest "include_file_info"

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_namespace.cc:96: AreaMap::const_iterator
delete_it = it;
Can you just do: areas_.erase(it++) ?

[michaeln, 2012-03-21 20:30:12]

thnx, i'll ping you again when i have some tests too

[michaeln, 2012-03-21 20:30:20]

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_namespace.cc:96: AreaMap::const_iterator
delete_it = it;
On 2012/03/21 20:26:29, jsbell wrote:
> Can you just do: areas_.erase(it++) ?

looks like i could

[michaeln, 2012-03-21 20:46:10]

still no tests

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:58: // Frees up memory when possible.
Typcically, this method returns
On 2012/03/21 20:26:29, jsbell wrote:
> typo "Typcically"

Done.

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_context.h (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:97: void
GetUsageInfo(std::vector<UsageInfo>* infos, bool get_file_info_too);
On 2012/03/21 20:26:29, jsbell wrote:
> Naming: suggest "include_file_info"

Done.

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9817011/diff/5009/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_namespace.cc:96: AreaMap::const_iterator
delete_it = it;
On 2012/03/21 20:26:29, jsbell wrote:
> Can you just do: areas_.erase(it++) ?

Done.

[michaeln, 2012-03-21 23:22:02]

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

http://codereview.chromium.org/9817011/diff/20002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:81: void
DomStorageContext::DeleteOrigin(const GURL& origin) {
fyi: When this DeleteOrigin() method is invoked, no DomStorage events are raised
into connected pages. This matches the behavior of the current impl.

http://codereview.chromium.org/9817011/diff/5010/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9817011/diff/5010/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_database.cc:62: // Delete the db and any
lingering journal file from disk.
added additional logic to also delete lingering journal files after some local
testing

[michaeln, 2012-03-21 23:57:24]

Added logic to enforce content policy at shutdown in snapshot 8 and 9

[jsbell, 2012-03-22 00:27:46]

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

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:123: if (!clear_local_state_ &&
!has_session_only_origins)
Just for readability, perhaps change this to:

if (clear_local_state_ || has_session_only_origins) {
  // ... PostShutdownBlockingTask ...
}

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:204: GetUsageInfo(&infos, false);
nit: named constant here?

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:216:
file_util::Delete(database_file_path, false);
nit: and here?

[michaeln, 2012-03-22 00:53:17]

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

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:123: if (!clear_local_state_ &&
!has_session_only_origins)
On 2012/03/22 00:27:46, jsbell wrote:
> Just for readability, perhaps change this to:
> 
> if (clear_local_state_ || has_session_only_origins) {
>   // ... PostShutdownBlockingTask ...
> }

Done.

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:204: GetUsageInfo(&infos, false);
On 2012/03/22 00:27:46, jsbell wrote:
> nit: named constant here?

Done.

http://codereview.chromium.org/9817011/diff/21012/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:216:
file_util::Delete(database_file_path, false);
On 2012/03/22 00:27:46, jsbell wrote:
> nit: and here?

Done.

[michaeln, 2012-03-22 05:34:40]

http://codereview.chromium.org/9817011/diff/1012/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_context.cc (right):

http://codereview.chromium.org/9817011/diff/1012/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.cc:93: if (infos[i].last_modified >
cutoff)
ooops... i have to consult policy->IsStorageProtected() here too, to avoid
deleting those guys

[michaeln, 2012-03-22 19:00:57]

http://chromiumcodereview.appspot.com/9817011/diff/1012/webkit/dom_storage/do...
File webkit/dom_storage/dom_storage_context.cc (right):

http://chromiumcodereview.appspot.com/9817011/diff/1012/webkit/dom_storage/do...
webkit/dom_storage/dom_storage_context.cc:93: if (infos[i].last_modified >
cutoff)
On 2012/03/22 05:34:40, michaeln wrote:
> ooops... i have to consult policy->IsStorageProtected() here too, to avoid
> deleting those guys

Done.

[jsbell, 2012-03-22 23:03:19]

lgtm

It's a shame to have the vaguely repeated pattern if (policy.allows) {
DoSomething(data) }, but it didn't look like passing the policy into the
DoSomething functions (DeleteOrigin) would be cleaner.

[michaeln, 2012-03-22 23:03:32]

added some area tests... i think this is ready (enough) to commit... ptal

http://chromiumcodereview.appspot.com/9817011/diff/35001/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_database.cc (right):

http://chromiumcodereview.appspot.com/9817011/diff/35001/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_database.cc:87: // empty database, we return true
if the database file doesn't exist.
unittests turned this one up, lazyopen would return false since we set the
'create' flag to false and the file didn't exist... then this method would
return false indicating the commit failed

[michaeln, 2012-03-22 23:43:31]

thnx!

On 2012/03/22 23:03:19, jsbell wrote:
> lgtm
> 
> It's a shame to have the vaguely repeated pattern if (policy.allows) {
> DoSomething(data) }, but it didn't look like passing the policy into the
> DoSomething functions (DeleteOrigin) would be cleaner.

Having the cleanup on shutdown bits be handled in namespace->Shutdown() and
area->Shutdown() instead of up a the context might be cleaner. If/when
localstorage is switched to use a single leveldb instance, we can revisit.

[jsbell, 2012-03-22 23:51:46]

lgtm

(Would love to see unittests for DomStorageContext but those can wait.)

[michaeln, 2012-03-22 23:58:48]

> (Would love to see unittests for DomStorageContext but those can wait.)

Yup... after this and the gypi changes are in, i can run try jobs with the flag
turned on. Before fleshing out unittests more, i'd like to see what those
tryjobs turn up with this actually plugged into chrome and drt... there might be
some more important integration things todo in there that i've missed.