Framing for a DOMStorage backend that does not depend on in-process-webkit.

Framing for a DomStorage backend that does not depend on in-process-webkit. Only partial unittests coverage in this CL, more to come.

These classes aren't used yet.

Classes intended to be used by an embedder are DomStorageContext,DomStorageHost,
and DomStorageSession. The other classes are for internal consumption.

In general terms, the DomStorage object relationships are...
* Contexts (per-profile) own Namespaces which own Areas which share Maps.
* Hosts(per-renderer) refer to Namespaces and Areas open in it's renderer.
* Sessions (per-tab) cause the creation and deletion of session Namespaces.

Session Namespaces are cloned by initially making a shallow copy of
all contained Areas, the shallow copies refer to the same refcounted Map,
and does a deep copy-on-write if needed.

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

[michaeln, 2012-01-24 00:43:12]

ptal

[marja, 2012-01-24 10:00:55]

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_session.h (right):

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_session.h:16: class DOMStorageSession
DOMSessionStorage instead of DOMStorageSession?

[michaeln, 2012-01-24 17:28:04]

> DOMSessionStorage instead of DOMStorageSession?

I had it as "DOMStorage<Foo>" for consistency with other class names having that
prefix. Also, i think 'session' is a more correct noun for this guy.

[jsbell, 2012-01-24 19:37:25]

Overall lgtm, for what that's worth. Watching this evolve is helpful to see the
Chromium pattern for Host/Session.

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_host.h (right):

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_host.h:37: NullableString16 GetKey(int
connection_id, int index);
Nit: The naming here is slightly inconsistent - why is this not GetAreaKey()?
Presumably because GetAreaLength/ClearArea operate on the whole Area, vs methods
that operate on just keys/items. I'd kind of like to see "Area" in the other
method names, but not strongly.

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_session.h (right):

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_session.h:16: class DOMStorageSession
Add a comment explaining the use of this type, as you have for DOMStorageHost. I
think that would clear up the naming confusion.

[benm (inactive), 2012-01-25 18:46:59]

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

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:25: const FilePath& backing_file);
I was thinking this would be the path where the backing db should live and it
would be up to this class to create the filename for the backing based on the
GURL?

http://codereview.chromium.org/9146025/diff/5019/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:27: int Length();
why int and not unsigned?

[michaeln, 2012-01-31 03:25:04]

Hi ben, a new snapshot...
* switched to DomStorage<<>> for class names
* added DomStorageTaskRunner class with two methods, PostTask and
PostDelayedTask
* added an DomStorageContext::EventObserver interface, the intent is for
DomStorageMessageFilter up in the content layer to derived from this interface.

I've put a reference to the task runner as a datamember of DomStorageArea, the
thought being that it could be used to lazily commit changes.

[benm (inactive), 2012-01-31 18:03:37]

looks good overall, just a few nits really. I'm happy to merge  your version of
DomStorageArea into the one that I've been working on if you want to take it out
of this CL, or I can rebase later.

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

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:35: int Length();
I used unsigned here, wdyt? (also for Key below)

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:48: void SyncChanges();
SyncChangesToBacking?

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:57: FilePath backing_fail_path_;
file :)

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

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:42: // An interface for observing
LocalStorage events. Mutation events
s/LocalStorage/DomStorage/

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:132: void
CloneSessionStorageNamespaceHelper(int64 existing_id, int64 new_id);
Why do we have these for session storage and not local storage?

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_host.cc (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_host.cc:50: return NullableString16();
NullableString16 is not null by default... think you want to pass true here?

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_namespace.h:30:
scoped_refptr<DomStorageTaskRunner> task_runner_;
Do we want a GUrl,DomStorageArea map here too?

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_task_runner.h:18: // Tasks are gauranteed to run
serially with respect to
guaranteed

[michaeln, 2012-01-31 21:17:45]

no new snapshot yet, just responding to comments

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

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:35: int Length();
On 2012/01/31 18:03:38, benm wrote:
> I used unsigned here, wdyt? (also for Key below)

k... lets switch to unsigned so we retain the datatypes used in the existing
impl

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_area.h:48: void SyncChanges();
On 2012/01/31 18:03:38, benm wrote:
> SyncChangesToBacking?

'sync' can imply reading and writing, but really this is only about committing
changes in memory to the db

wdyt about using 'CommitChanges' instead to avoid the less clear 'sync'  term?

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

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:42: // An interface for observing
LocalStorage events. Mutation events
On 2012/01/31 18:03:38, benm wrote:
> s/LocalStorage/DomStorage/

Renderer's raise events associated with changes that originate within its
process w/o routing the event thru the browser process. Since sessionstorage is
only accessed from a single renderer, there's no need to invoke this interface
for it.

As coded in the CL in this snapshot it will get invoked for both types, but i'll
snip that so we only do this for localstorage.

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_context.h:132: void
CloneSessionStorageNamespaceHelper(int64 existing_id, int64 new_id);
On 2012/01/31 18:03:38, benm wrote:
> Why do we have these for session storage and not local storage?

LocalStorage has a magic namespace_id of 0 and there is only one localstorage
namespace per-profile (ala DomStorageContext) that lives for the duration (and
persists on disk). It exists implicitly.

SessionStorage namespaces come and go with tabs, each new tab gets a unique
sessionstorage namespace_id, and when the tab closes it gets deleted. (and until
marja's session restore stuff comes along, none of this goes to disk). So these
namespace get explicitly created and destroyed by the embedder indirectly via
the DomStorageSession class.

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_host.cc (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_host.cc:50: return NullableString16();
On 2012/01/31 18:03:38, benm wrote:
> NullableString16 is not null by default... think you want to pass true here?

Done here and below

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_namespace.h:30:
scoped_refptr<DomStorageTaskRunner> task_runner_;
On 2012/01/31 18:03:38, benm wrote:
> Do we want a GUrl,DomStorageArea map here too?

Yup, I'm getting there, still working out ownership/lifetime semantics.

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9146025/diff/9001/webkit/dom_storage/dom_stora...
webkit/dom_storage/dom_storage_task_runner.h:18: // Tasks are gauranteed to run
serially with respect to
On 2012/01/31 18:03:38, benm wrote:
> guaranteed

thnx (geez, i misspell that word a lot)

[michaeln, 2012-02-02 02:41:39]

new snapshot

I have to stop adding code and start adding tests, these classes are less then
skeletal now. I was dragging my feet about who should own what and how things
get deleted and such. After settling on an approach, i wrote a bunch of method
bodies. What's coded populates collections in memory only and establishes
lifecycles of the various objects, and implements 'cloning' for session storage
by making a shallow copy first, and a then deep copies as needed.

Contexts own Namespaces which own Areas which share Maps
Hosts refer to Namespace/Area
Sessions cause the creation/deletion of SessionNamespaces

I'm keeping track of which areas are open/closed but don't take advantage of
that yet.

I decided to refcount thing more so than in the current in_process_webkit impl
to avoid backpointers. Used threadsafe refcounts mostly because of the nature of
the 'sequenced worker pool' class that uses multiple threads.

ptal

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

Looking good overall, but I think it'd be nice to try and split this into a
bunch of CLs... one that's purely the skeleton without any tests, and then a
series that adds the impl with tests. wdyt about splitting it? It'd make it
easier to track how this was developed when looking back over the git history, I
think.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:24: // background thread.
Can you add comments to these classes like those you made in your last message
regarding who owns what?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:56: scoped_refptr<DomStorageMap> map_;
We're still planning on having a changed_values_ DomStorageMap here, right?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:57: // TODO(michaeln): make it so this
class can persist stuff to disk
Should just need a DomStorageDatabase instance and a way to get at the
underlying std::map in DomStorageMap.

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

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:23: // AtomicSequenceNum starts at 0
but we want to start at one.
... because of the local storage special id? Maybe worth elaborating the
comment.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.h:90: // features, we'll have to provide
them, but just not yet.
Not sure what the Chrome style is on this but in my personal opinion, lets not
check in function names we think we need until we know exactly what they need to
do? I don't like the idea of making it feel like we're tied into doing what the
old impl did... wdyt?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_host.cc (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_host.cc:31: return -1;  // TODO(michaeln):
constant definition somewhere?
kInvalidAreaId in dom_storage_area.h?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_host.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_host.h:49: // Struct to hold referernces needed.
references

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map.cc (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.cc:34: // TODO(michaeln): check quota and
return false if exceeded
I was thinking that this would be more the responsibility of the DomStorageArea
or some other higher-up class?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.h:20: class DomStorageMap
This looks very similar to the DomStorageArea CL I was working on. As I already
have tests set up for that and have addressed some of the TODOs I see in the cc,
I could merge this with it and rename my class to DomStorageMap? I guess
DomStorageArea should still be responsible for syncing things to a backing
database, so I'd remove those bits from my CL.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.h:31: 
The way that the DomStorageDatabase is set up right now I guess we also need an
accessor to get at the underlying map.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:27: // TODO(michaeln): Compute the
right filepath.
Do you mean whether there should be a backing file or not, something like

namespace_id_ == 0 ? directory_ : FilePath()

?

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:22: class DomStorageTaskRunner
This change is getting a bit big IMHO with a mix of skeleton files and real
impl... this class feels like it could go into it's own patch (nice and self
contained)? It'd be nice to keep this patch as just skeletons, I think.

[michaeln, 2012-02-03 00:33:42]

thnx for looking...

i'll keep this CL to show how things hang together and carve off smaller CLs
along the lines described below (at the end)

new snapshot

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_area.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:24: // background thread.
On 2012/02/02 16:23:19, benm wrote:
> Can you add comments to these classes like those you made in your last message
> regarding who owns what?

Done, added class with an overview on DomStorageContext.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:56: scoped_refptr<DomStorageMap> map_;
On 2012/02/02 16:23:19, benm wrote:
> We're still planning on having a changed_values_ DomStorageMap here, right?

yes, but i think just need strictly owned ValuesMap data member to keep track of
the changed_values_ in the area instead of the refcounted StorageMap obj

i've been staying away from coding logic to integrate these classes with
DomStorageDatabase (to keep the CL smaller)

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area.h:57: // TODO(michaeln): make it so this
class can persist stuff to disk
On 2012/02/02 16:23:19, benm wrote:
> Should just need a DomStorageDatabase instance and a way to get at the
> underlying std::map in DomStorageMap.

Right... i think a DomStorageMap.SwapValues(ValuesMap* map) could work?

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

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.cc:23: // AtomicSequenceNum starts at 0
but we want to start at one.
On 2012/02/02 16:23:19, benm wrote:
> ... because of the local storage special id? Maybe worth elaborating the
> comment.

Done.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_context.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_context.h:90: // features, we'll have to provide
them, but just not yet.
On 2012/02/02 16:23:19, benm wrote:
> Not sure what the Chrome style is on this but in my personal opinion, lets not
> check in function names we think we need until we know exactly what they need
to
> do? I don't like the idea of making it feel like we're tied into doing what
the
> old impl did... wdyt?

nuked... we have svn history to see what interfaces have been skipped for now,
that and the complier will tell us what's missing too when we get to plugging
this into chrome

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_host.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_host.h:49: // Struct to hold referernces needed.
On 2012/02/02 16:23:19, benm wrote:
> references

Done.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map.cc (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.cc:34: // TODO(michaeln): check quota and
return false if exceeded
On 2012/02/02 16:23:19, benm wrote:
> I was thinking that this would be more the responsibility of the
DomStorageArea
> or some other higher-up class?

Since we need the size of the old values and need to distiquish between the
insert and replace cases to do the book keeping, this class seems like a better
home for that logic.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.h:20: class DomStorageMap
On 2012/02/02 16:23:19, benm wrote:
> This looks very similar to the DomStorageArea CL I was working on. As I
already
> have tests set up for that and have addressed some of the TODOs I see in the
cc,
> I could merge this with it and rename my class to DomStorageMap? I guess
> DomStorageArea should still be responsible for syncing things to a backing
> database, so I'd remove those bits from my CL.

Ack about the 'area' being responsible for reading/writing and the 'map' just
being a collection. We need to start landing stuff so we can work in parallel on
the same source files and all that

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map.h:31: 
On 2012/02/02 16:23:19, benm wrote:
> The way that the DomStorageDatabase is set up right now I guess we also need
an
> accessor to get at the underlying map.

Done, added a SwapValues(values_map) method.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.cc (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.cc:27: // TODO(michaeln): Compute the
right filepath.
On 2012/02/02 16:23:19, benm wrote:
> Do you mean whether there should be a backing file or not, something like
> namespace_id_ == 0 ? directory_ : FilePath()

Done. I see in your CL you've got the 'area' computing the filename and its just
expecting the directory path in the ctor.

I've also made two different 'namespace' ctors, one for localstorage namespaces
and the other for sessionstorage namespaces, that codify and document special id
values and empty directory paths and such.

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9146025/diff/20004/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:22: class DomStorageTaskRunner
On 2012/02/02 16:23:19, benm wrote:
> This change is getting a bit big IMHO with a mix of skeleton files and real
> impl... this class feels like it could go into it's own patch (nice and self
> contained)? It'd be nice to keep this patch as just skeletons, I think.

It's actually not so skeletal anymore. There are no bindings to read/write from
disk, and it doesn't track/enforce quota limits, but otherwise i think these
classes should function. I'd rather not strip the method bodies out an land just
.h file.

Not sure where to draw what lines for landing things?

The TaskRunner is an outlyer and for testing purposes it should would be nice to
have an impl that really just posts everything to the MessageLoop of the current
thread. Maybe i can make that more of an abstract class and commit a 'mock' impl
that does just that as one CL, and come back later with an impl that uses
SequencedWorkerPool.

Area and Map overlap with some of your work thus far, so it'd be good to get
those in asap so you can start binding things the Database class.

So how's this for a commit sequence...
1) abstract taskrunner + mock that uses MessageLoop only
2) Map + unittests
3) Area + unittests

Since area depends on namespace.h for kLocalStorageNamespaceId, and we collide
on the definition of ValueMap, i'll make a dom_storage_types.h file for those
definitions and put it in CL 2. You say you've already got Area/Map 'ish
unittests in the works, maybe I can commit w/o tests and we can back fill?

4) Host/Context/Namespace + unittests
5) TaskRunner backed by sequenced worker

Your db change can go in anytime prior to the Area + DB integration.

[michaeln, 2012-02-03 05:33:49]

Refactored the TaskRunner as described ealier.
Added unittests for the Map and Area classes.
Fixed some compile errors on mac/linux.

The other option is to commit this as one CL w/o complete unittest coverage
initially and to follow up on tests as things get more fleshed out. I think
that's a pretty good option.

[benm (inactive), 2012-02-03 16:03:31]

On 2012/02/03 05:33:49, michaeln wrote:
> Refactored the TaskRunner as described ealier.
> Added unittests for the Map and Area classes.
> Fixed some compile errors on mac/linux.
> 
> The other option is to commit this as one CL w/o complete unittest coverage
> initially and to follow up on tests as things get more fleshed out. I think
> that's a pretty good option.

I don't think that we need to necessarily check in complete suites of tests if
we went with the set of patches as long as we have plans to check the tests in
later, which I know we do :) (or even just a really simple test that verifies
some basic functionality, not the entire class)

My personal preference would be to go with the chunked approach you described
above. This CL looks pretty good to me, so it should be possible to send the
smaller patches pretty much one after the other I think? Once the makefile is in
I can rebase my Database change and after 2) make use of dom_storage_types.h.
There are no other dependencies in the DB change, so it should be good to go in
anytime after 2). Then I can copy some of the area tests I had into the Map
tests, look at the TODOs in Map and start working on gluing the DB and area
after 3).

[benm (inactive), 2012-02-03 16:03:59]

Just a few nits but overall LGTM!

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

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:28: EXPECT_EQ(0,
area->Length());
Think you need a 0u here.

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:52: EXPECT_EQ(0,
area->Length());
0u

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_host.cc (right):

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_host.cc:87: const GURL& page_url,
nit: indent

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_map_unittest.cc (right):

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_map_unittest.cc:24: EXPECT_EQ(0, map->Length());
0u

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:29: // and an optional backing
directory on disk.
The only case I can think of where there would be no backing for local storage
is incognito, is that right? (Sorry if this seems obvious but in my
newbie-chrome mindset I find I need to keep reminding myself of this kind of
thing)

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:52: // Struct to hold a references to
our contained areas and
nit: no 'a'

[michaeln, 2012-02-03 20:15:57]

thnx for looking

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

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_area_unittest.cc:28: EXPECT_EQ(0,
area->Length());
On 2012/02/03 16:04:00, benm wrote:
> Think you need a 0u here.

Done, what a silly compile error

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_host.cc (right):

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_host.cc:87: const GURL& page_url,
On 2012/02/03 16:04:00, benm wrote:
> nit: indent

Done.

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_namespace.h (right):

http://codereview.chromium.org/9146025/diff/21036/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_namespace.h:52: // Struct to hold a references to
our contained areas and
On 2012/02/03 16:04:00, benm wrote:
> nit: no 'a'

Done.

[michaeln, 2012-02-04 01:46:52]

@tony or @darin

can i get an owners LGTM for touching webkit_support.gyp and test_shell.gypi?

[michaeln, 2012-02-04 02:31:50]

see snapshot 14 for green test runs

[darin (slow to review), 2012-02-04 05:26:07]

LGTM for .gypi changes

http://codereview.chromium.org/9146025/diff/21047/webkit/dom_storage/dom_stor...
File webkit/dom_storage/dom_storage_task_runner.h (right):

http://codereview.chromium.org/9146025/diff/21047/webkit/dom_storage/dom_stor...
webkit/dom_storage/dom_storage_task_runner.h:18: // Tasks must run serially with
respect to one another, but may
there's actually work underway by akalin to create *TaskRunner classes:  one
that will replace MessageLoopProxy and one that will wrap SequencedWorkerPool!