DomStorageArea: Allow multiple simultaneous in flight commit batches.

DomStorageArea: Allow multiple simultaneous in flight commit batches.

Currently, only one commit batch will be in flight at given time. This change is
needed to be able to support shallow copies of file-backed sessionStorage.

BUG=104292
TEST=Existing DomStorageAreaTest.* tests.


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

[marja, 2012-05-10 08:11:29]

Hi again,

I split off this part to generalize DomStorageArea (so that it supports multiple
in flight commit batches) from my sessionStorage work. This is needed to make
the shallow copies work.

When we realize we need a clone (a shallow copy), we first need to commit all
the pending changes (there might be some in in_flight_commit_batch_ and some in
commit_batch_), then make the clone (on the namespace level), and then carry on
committing all future changes. When the pending changes and the cloning are
still in the task queue, new changes might come (and the commit timer may fire).
We'd still need to make sure that before cloning we commit only the changes
which were there before the cloning was initiated.

It would be cumbersome to have a queue of CommitBatches, because then the
COMMIT_SEQUENCE would need some extra information to know how many of them to
commit. (The queue might contain changes to be done before the clone and after
the clone.)

Instead, this solution relies on scheduling things in the right order to be
executed by the COMMIT_SEQUENCE. When we need to make a clone, we schedule
committing commit_batch_ and reset it (other commits might be in flight at that
point, that's ok), then we schedule the actual cloning, and if later changes
happen and the commit timer fires, they will also get scheduled, but they will
happen after these 2 things.

This version can have only one commit batch in flight (
DCHECK(!commit_batches_in_flight_); in OnCommitTimer), and I'd change this in
the CL that integrates SessionStorageDatabase into DomStorageArea.

[michaeln, 2012-05-10 16:19:21]

sgtm modulo the comment about base::Owned<> usage

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

http://codereview.chromium.org/10389061/diff/3001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:267: *in_flight_commit_batch));
Please use base::Owned<> here to pass ownership of the heap allocated
CommitBatch to the bind closure instead of making a copy of the struct in the
closure. As coded, it looks like the original heap allocated block is being
leaked here. By use base::Owned<>, no leak and no memcpys needed.

http://codereview.chromium.org/10389061/diff/3001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:272: void
DomStorageArea::CommitChanges(const CommitBatch& commit_batch) {
When using base::Owned<>, this method would take a const CommitBatch* as input.

[marja, 2012-05-11 07:50:00]

Ahh, this was horribly wrong. Thanks for comments.

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

http://codereview.chromium.org/10389061/diff/3001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:267: *in_flight_commit_batch));
On 2012/05/10 16:19:22, michaeln wrote:
> Please use base::Owned<> here to pass ownership of the heap allocated
> CommitBatch to the bind closure instead of making a copy of the struct in the
> closure. As coded, it looks like the original heap allocated block is being
> leaked here. By use base::Owned<>, no leak and no memcpys needed.

Done.

http://codereview.chromium.org/10389061/diff/3001/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.cc:272: void
DomStorageArea::CommitChanges(const CommitBatch& commit_batch) {
On 2012/05/10 16:19:22, michaeln wrote:
> When using base::Owned<>, this method would take a const CommitBatch* as
input.

Done.

[michaeln, 2012-05-11 15:40:42]

lgtm

[commit-bot: I haz the power, 2012-05-14 08:36:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10389061/5001

[commit-bot: I haz the power, 2012-05-14 10:07:31]

Change committed as 136849