Hook up DomStorageArea with a DomStorageDatabase.

Hook up DomStorageArea with a DomStorageDatabase. Uses DomStorageTaskRunner to asynchronously write to the database.
BUG=106763
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123744

[benm (inactive), 2012-02-13 19:40:27]

Hi Michael,

Here's first pass at hooking DomStorageDatabase into DomStorageArea. Right now
everything is synchronous - we sync to backing on every write to the map. I see
two ways of this becoming asynchronous...

1/ SyncToBacking() uses the task_runner_ to post a task to write to the DB. I
tried this and have a patch locally that works. We still need to block on the
initial import though, and lock on items_to_sync_. So I wondered about 2/ which
I think you alluded to previously...

2/ Keep DomStorageArea synchronous - we perform blocking IO on the initial
import and SyncToBacking, but do not call SyncToBacking() when we update the
map, rather make it a public method that someone higher up invokes as they see
fit (DomStorageHost probably?). If we go this way, I don't think that
DomStorageArea needs to know about task_runner_ and it could be removed?

Please take a look and let me know what you think...

Cheers, Ben

[michaeln, 2012-02-14 06:39:26]

You say you have a start at a lazy async version that utilizes taskRunner, that
sounds like a better first cut at it. Since all access to the 'area' is
serialized per our 'sequence id', there should be need for locks.

https://chromiumcodereview.appspot.com/9389009/diff/5001/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_area.h (right):

https://chromiumcodereview.appspot.com/9389009/diff/5001/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.h:14: #include
"webkit/dom_storage/dom_storage_database.h"
instead of directly including, i think this could just be a forward decl in the
.h file

https://chromiumcodereview.appspot.com/9389009/diff/5001/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.h:56: void SyncToBacking();
should we stick with the CommitChanges terminology at to match whats used in
DomStorageDatabase?

https://chromiumcodereview.appspot.com/9389009/diff/5001/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.h:69: ValuesMap items_to_sync_;
if switching terminology, changed_values_?

[benm (inactive), 2012-02-14 11:12:07]

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

http://codereview.chromium.org/9389009/diff/5001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:14: #include
"webkit/dom_storage/dom_storage_database.h"
I think it needs to be #included as it's used in a scoped_ptr rather than a
scoped_refptr.

http://codereview.chromium.org/9389009/diff/5001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:56: void SyncToBacking();
On 2012/02/14 06:39:26, michaeln wrote:
> should we stick with the CommitChanges terminology at to match whats used in
> DomStorageDatabase?

Done.

http://codereview.chromium.org/9389009/diff/5001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:69: ValuesMap items_to_sync_;
On 2012/02/14 06:39:26, michaeln wrote:
> if switching terminology, changed_values_?

Done.

[benm (inactive), 2012-02-14 16:29:00]

new snapshot, please take a look!

Cheers, Ben

[michaeln, 2012-02-15 04:10:53]

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.cc:134: if (!clear_all_next_commit_ &&
changed_values_.empty())
given the usage of this method, it might make more sense to DCHECK() there's
something to be done instead of early returning if not

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.cc:138: if (!task_runner_->PostTask(
As coded, this will result in many small transactions, one per every call to
set(), remove() and clear(). That would be noticable regression from the current
impl which batches changes and commits them on a one second timer.

Can you take a stab as logic to batch them up? I think the PostDelayedTask
function should help with that. Something like when a mutation happens, schedule
a delayed commit task if one is not already scheduled. When it runs clear the
flag that says one is scheduled.

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_area.h (left):

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_area.h:14: #include
"webkit/dom_storage/dom_storage_task_runner.h"
thnx for cleaning up this include

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

https://chromiumcodereview.appspot.com/9389009/diff/7002/webkit/dom_storage/d...
webkit/dom_storage/dom_storage_task_runner.cc:67:
base::Bind(&DomStorageWorkerPoolTaskRunner::PostTaskVoid, this,
You may be able to use base::IgnoreResult() on the PostTask method directly
instead of creating the <<>>Void shim.

[benm (inactive), 2012-02-15 17:38:16]

http://codereview.chromium.org/9389009/diff/7002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9389009/diff/7002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:134: if (!clear_all_next_commit_ &&
changed_values_.empty())
sgtm.

http://codereview.chromium.org/9389009/diff/7002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.cc:138: if (!task_runner_->PostTask(
On 2012/02/15 04:10:53, michaeln wrote:
> As coded, this will result in many small transactions, one per every call to
> set(), remove() and clear(). That would be noticable regression from the
current
> impl which batches changes and commits them on a one second timer.
> 
> Can you take a stab as logic to batch them up? I think the PostDelayedTask
> function should help with that. Something like when a mutation happens,
schedule
> a delayed commit task if one is not already scheduled. When it runs clear the
> flag that says one is scheduled.

Done.

http://codereview.chromium.org/9389009/diff/7002/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/7002/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_task_runner.cc:67:
base::Bind(&DomStorageWorkerPoolTaskRunner::PostTaskVoid, this,
I think the best I can do is wrap a call to PostTask with ignore_result inside
the Void method, that avoids the code duplication.

[benm (inactive), 2012-02-15 20:45:46]

New snapshot that should batch changes up and commit them 1s after the first
change is made. Please take a look!

Thanks, Ben

[benm (inactive), 2012-02-22 19:57:55]

Rebased, please take a look!

Cheers, ben

[michaeln, 2012-02-23 05:15:29]

I think there's another behavior our impl should have. Deleting empty database
files on exit. There are existing ui tests that check for that behavior.

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_area.cc (right):

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:48: DCHECK(changed_values_.empty());
Would it make sense to move these checks into the CommitChanges method? Not sure
we need them though really. Is the intent to catch a programming error or to
inform a developer that the db is hosed for some reason. If the latter, is that
something that should be DCHECKed?

Ultimately, UMA stats on how often backing->CommitChanges and ReadAllValues()
fail might be more useful than these dchecks?

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:145: // to commit all the remaining
changes to disk.
The comment is more about the code in the dtor than the code here, wdyt about
moving it there.

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_area.cc:153: // so that we don't lose data.
I think we should not handle this unexpected case in this way. Under normal
circumstances, this should not happen. The messageloops and workerpool we're
running on should be there.

Maybe DCHECK that the task is posted. If that check triggers, it would indicate
that something is wrong in the embedding program (a programming error that
failed to start a thread or shutdown a workerpool sooner than it should have
been).

commit_in_flight_ = PostDelayedTask(...);
DCHECK(commit_in_flight_);

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

https://chromiumcodereview.appspot.com/9389009/diff/25001/webkit/dom_storage/...
webkit/dom_storage/dom_storage_task_runner.cc:76:
ignore_result(PostTask(from_here, task));
Did you try base::IgnoreResult (see example usage below)? If that doesn't work
for some reason, let's not add ignore_result here.

http://code.google.com/p/chromium/source/search?q=base%3A%3AIgnoreResult&orig...

[benm (inactive), 2012-02-23 12:27:40]

Thanks for looking! New snapshot on the way.

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.cc (right):

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:48: DCHECK(changed_values_.empty());
Yeah, I actually had the same though re. UMA tracking last night after looking
over this patch after uploading. I think it makes sense to do that. I'm not
entirely sure how much work it is to integrate with UMA though, so I'd rather
tackle it in a separate patch (unless it's really simple?). Will add a todo.

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:145: // to commit all the remaining
changes to disk.
sgtm

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:153: // so that we don't lose data.
ok, sg.

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/25001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:76:
ignore_result(PostTask(from_here, task));
Oh cool, I didn't notice there were two ignoreresults. Will fix up.

[benm (inactive), 2012-02-23 19:52:38]

New snapshot ready, ptal!

[michaeln, 2012-02-23 20:48:48]

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area_unittest.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:20: void QuitMessageLoop() {
should either make this static or put it in an anon namespace to keep it out of
the dom_storage namespace

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:70: const int64
kNonLocalStorageNamespaceId = kLocalStorageNamespaceId + 1;
kSessionStorageNamespaceId?

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:71: const GURL
origin("http://www.google.com");
kOrigin

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:87:
DomStorageArea::DatabaseFileNameFromOrigin(origin))));
consider making a local const for this FilePath too, maybe kExpectedFilePath,
since it's used multiple times with harsh on the eyes lengthy line wrappings

EXPECT_FALSE(file_util::PathExists(kExpectedFilePath));

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:113: base::SequencedWorkerPool
pool(2, "dom_storage_area_test");
might be easier to compose this test w/o using a real SequencedWorkerPool + real
DomStorageWorkerPoolTaskRunner

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:135:
base::TimeDelta::FromSeconds(2));
Does this result in a 2 second delay of test execution? If so, is there some
unittest'y way of running all including delayed tasks (such that the delay
elapses immediately)?

2 seconds is a long time

if not, maybe a messageloop only test task runner derivative that squashes the
delay to zero, just call RunAllPending instead of multi-step
posting/spinning/quiting/pool.flushing

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.cc:45: "SELECT COUNT(key) from
ItemTable"));
like we chatted about, can the 'its empty' condition be determined w/o incurring
disk io at dtor time... probably worth some extra statefullness in this class to
avoid many small bits of io at browser shutdown

[michaeln, 2012-02-23 20:52:38]

also...

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:68:
base::Bind(IgnoreResult(&DomStorageWorkerPoolTaskRunner::PostTask), this,
maybe you don't need to fully quality base::IgnoreResult for it to compile (i
guess maybe since we're in the param list of a base:: prefixed function call),
but other usages i've seen consistently do fully quality it... so please fully
qualify it here too for consistency

[benm (inactive), 2012-02-24 12:42:01]

thanks, new snapshot uploaded.

I've moved the code to delete empty databases into
http://codereview.chromium.org/9467003/ as it didn't seem directly related to
this change.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area_unittest.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:20: void QuitMessageLoop() {
On 2012/02/23 20:48:48, michaeln wrote:
> should either make this static or put it in an anon namespace to keep it out
of
> the dom_storage namespace

Done.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:70: const int64
kNonLocalStorageNamespaceId = kLocalStorageNamespaceId + 1;
On 2012/02/23 20:48:48, michaeln wrote:
> kSessionStorageNamespaceId?

Done.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:71: const GURL
origin("http://www.google.com");
On 2012/02/23 20:48:48, michaeln wrote:
> kOrigin

Done.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:87:
DomStorageArea::DatabaseFileNameFromOrigin(origin))));
On 2012/02/23 20:48:48, michaeln wrote:
> consider making a local const for this FilePath too, maybe kExpectedFilePath,
> since it's used multiple times with harsh on the eyes lengthy line wrappings
> 
> EXPECT_FALSE(file_util::PathExists(kExpectedFilePath));

Done.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:113: base::SequencedWorkerPool
pool(2, "dom_storage_area_test");
Done - created a mock task runner.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:135:
base::TimeDelta::FromSeconds(2));
Done - the mock task runner ignores the timeout passed in PostDelayedTask and
runs the task immediately.

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_database.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_database.cc:45: "SELECT COUNT(key) from
ItemTable"));
Done, but moved to it's own CL in http://codereview.chromium.org/9467003/

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/32002/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:68:
base::Bind(IgnoreResult(&DomStorageWorkerPoolTaskRunner::PostTask), this,
On 2012/02/23 20:52:38, michaeln wrote:
> maybe you don't need to fully quality base::IgnoreResult for it to compile (i
> guess maybe since we're in the param list of a base:: prefixed function call),
> but other usages i've seen consistently do fully quality it... so please fully
> qualify it here too for consistency 

Done.

[michaeln, 2012-02-24 19:29:51]

lgtm

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area_unittest.cc (right):

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:113:
EXPECT_TRUE(area->backing_.get());
maybe EXPECT_FALSE(area->backing_->IsOpen()) here

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:114:
EXPECT_FALSE(area->initial_import_done_);
Consider replacing the 'file' backed DomStorageDatabase with an in-memory
instance to avoid actual fileio in this test, we can confirm that something was
written to the db after RunAllPending() by querying db->ReadAllValues() and
looking at the size of the resulting ValuesMap.

i think you could inject the in-memory guy at this point
area->backing_.reset(new DomStorageDatabase);

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:123: 
maybe EXPECT_TRUE(area->commit_in_flight_) before and FALSE after RunAllPending

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:84: // Don't wait in unit tests.
indent of off here and the next line

[benm (inactive), 2012-02-27 10:39:26]

thanks!

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area_unittest.cc (right):

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:113:
EXPECT_TRUE(area->backing_.get());
On 2012/02/24 19:29:52, michaeln wrote:
> maybe EXPECT_FALSE(area->backing_->IsOpen()) here

Done.

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:114:
EXPECT_FALSE(area->initial_import_done_);
Good idea. I thought it was nice to assert that the file was actually created on
disk, but I guess that is taken care of by the DatabaseTests and all we need to
do here is make sure it's written to the db.

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:123: 
On 2012/02/24 19:29:52, michaeln wrote:
> maybe EXPECT_TRUE(area->commit_in_flight_) before and FALSE after
RunAllPending

Done.

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.cc (right):

http://codereview.chromium.org/9389009/diff/40001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.cc:84: // Don't wait in unit tests.
On 2012/02/24 19:29:52, michaeln wrote:
> indent of off here and the next line

Done.

[commit-bot: I haz the power, 2012-02-27 14:37:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benm@chromium.org/9389009/42004

[commit-bot: I haz the power, 2012-02-27 16:26:05]

Change committed as 123744