DOM Storage: Add renderer-process IPC code.

The final CL for plumbing DOM Storage.

Add webKitClient plumbing for getting/creating storage namespaces.  Add a chromium implementation for WebStorageArea and WebStorageNamespace which communicates via IPC with the dom_storage_dispatcher_host in the browser process.  Flesh out the StorageAreaProxy and StorageNamespaceProxy to use the aforementioned implementations.

The WebStorageArea implementation includes decently aggressive caching optimizations.  There's still a lot of work to do, though.

BUG=4360
TEST=none


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

[jorlow, 2009-06-26 23:08:45]

This is still VERY early, but I wanted to make sure my approach with the
webkit/api was correct.  Any advice about that or high level advice on the rest
is welcome.

[jam, 2009-06-27 00:09:22]

the approach is definitely sound & consistent

http://codereview.chromium.org/147248/diff/1/6
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/1/6#newcode39
Line 39: bool DOMStorageDispatcherHost::OnMessageReceived(const IPC::Message&
message) {
you should add the "msg_is_ok" code here, it helps guard against exploited
renderers trying to fuzz the browser looking for an exploit

http://codereview.chromium.org/147248/diff/1/6#newcode93
Line 93: &DOMStorageDispatcherHost::Send,
nit: the spacing is off.  if it can't fit under "this", then they should all
start on a new line tabbed 4 spaces.

http://codereview.chromium.org/147248/diff/1/7
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/147248/diff/1/7#newcode15
Line 15: class WebStorage;
nit: no spaces

http://codereview.chromium.org/147248/diff/1/7#newcode51
Line 51: void OnNamespaceId(const bool is_local_storage, IPC::Message*
reply_msg);
nit: we don't put "const" infront of simple data types

http://codereview.chromium.org/147248/diff/1/8
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/147248/diff/1/8#newcode1409
Line 1409: bool /* is local storage */,
nit: I know this isn't followed everywhere, but the convention is to use the
parameter name inside the comment (i.e. is_local_storage)

http://codereview.chromium.org/147248/diff/1/3
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/147248/diff/1/3#newcode1480
Line 1480: // FIXME: Before DOM_STORAGE is enabled by default, this needs to
change.
perhaps add a command line flag now, so you get the benefits of always being
compiled in, but not having to worry about intermediate work getting shipped to
users.

[jorlow, 2009-06-27 02:48:12]

I implemented the renderer side of the plumbing and made all the changes you
suggested.

Still far away from being ready to land, but initial feedback would be nice.

http://codereview.chromium.org/147248/diff/1/6
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/1/6#newcode39
Line 39: bool DOMStorageDispatcherHost::OnMessageReceived(const IPC::Message&
message) {
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> you should add the "msg_is_ok" code here, it helps guard against exploited
> renderers trying to fuzz the browser looking for an exploit

Done.

http://codereview.chromium.org/147248/diff/1/6#newcode93
Line 93: &DOMStorageDispatcherHost::Send,
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> nit: the spacing is off.  if it can't fit under "this", then they should all
> start on a new line tabbed 4 spaces.

Done.

http://codereview.chromium.org/147248/diff/1/7
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/147248/diff/1/7#newcode15
Line 15: class WebStorage;
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> nit: no spaces

Done.

http://codereview.chromium.org/147248/diff/1/7#newcode51
Line 51: void OnNamespaceId(const bool is_local_storage, IPC::Message*
reply_msg);
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> nit: we don't put "const" infront of simple data types

Done.

http://codereview.chromium.org/147248/diff/1/8
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/147248/diff/1/8#newcode1409
Line 1409: bool /* is local storage */,
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> nit: I know this isn't followed everywhere, but the convention is to use the
> parameter name inside the comment (i.e. is_local_storage)

Done.

http://codereview.chromium.org/147248/diff/1/3
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/147248/diff/1/3#newcode1480
Line 1480: // FIXME: Before DOM_STORAGE is enabled by default, this needs to
change.
On 2009/06/27 00:09:22, John Abd-El-Malek wrote:
> perhaps add a command line flag now, so you get the benefits of always being
> compiled in, but not having to worry about intermediate work getting shipped
to
> users.

I agree we should do this, but it's not clear to me how we can.  We can talk
Monday about how best to do it.

[darin (slow to review), 2009-06-27 04:33:36]

http://codereview.chromium.org/147248/diff/1006/1010
File webkit/api/public/WebKit.h (right):

http://codereview.chromium.org/147248/diff/1006/1010#newcode91
Line 91: WEBKIT_API WebStorage* webStorage();
instead of putting this here, i think it should just be a static method on
WebStorage named "create".  see WebWorker.

http://codereview.chromium.org/147248/diff/1006/1018
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/147248/diff/1006/1018#newcode120
Line 120: // DOM Storage --------------------------------------------------
nit: insert newline after this comment

http://codereview.chromium.org/147248/diff/1006/1018#newcode121
Line 121: virtual WebStorage* webStorage() = 0;
I think the intent here is to instantiate an instance that the receiver needs to
'delete' once they are done with it, right?  In that case, it is like
createURLLoader, and should be named createStorage.  (We drop the "Web" prefix
in method names.)

http://codereview.chromium.org/147248/diff/1006/1016
File webkit/api/public/WebStorage.h (right):

http://codereview.chromium.org/147248/diff/1006/1016#newcode42
Line 42: 
some of these methods are not super obvious.  brief comments might help.  maybe
add a comment to define namespaces and storage areas?

http://codereview.chromium.org/147248/diff/1006/1016#newcode47
Line 47: virtual void lock(long long storageAreaId, bool* invalidateCache, long
long* bytesLeftInQuota) = 0;
since so many of these methods are prefixed with storageAreaId, perhaps there
should be a class named WebStorageArea?  then WebStorage would just have a
method to get the WebStorageArea corresponding to the given storageAreaId?  or
would that just add another layer for no real benefit?

http://codereview.chromium.org/147248/diff/1006/1016#newcode49
Line 49: virtual unsigned length(long long storageAreaId) = 0;
is length a byte count?  if so, prefer size_t.

[jorlow, 2009-06-27 05:48:58]

http://codereview.chromium.org/147248/diff/1006/1010
File webkit/api/public/WebKit.h (right):

http://codereview.chromium.org/147248/diff/1006/1010#newcode91
Line 91: WEBKIT_API WebStorage* webStorage();
On 2009/06/27 04:33:36, darin wrote:
> instead of putting this here, i think it should just be a static method on
> WebStorage named "create".  see WebWorker.

The problem is that there are 2 different versions of the WebStorage object
depending on if we're in the renderer or the browser process.  I could have 2
static methods, but that seems a bit clunky.  What do you think is best?

http://codereview.chromium.org/147248/diff/1006/1018
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/147248/diff/1006/1018#newcode120
Line 120: // DOM Storage --------------------------------------------------
On 2009/06/27 04:33:36, darin wrote:
> nit: insert newline after this comment

will do

http://codereview.chromium.org/147248/diff/1006/1018#newcode121
Line 121: virtual WebStorage* webStorage() = 0;
On 2009/06/27 04:33:36, darin wrote:
> I think the intent here is to instantiate an instance that the receiver needs
to
> 'delete' once they are done with it, right?  In that case, it is like
> createURLLoader, and should be named createStorage.  (We drop the "Web" prefix
> in method names.)

will do

http://codereview.chromium.org/147248/diff/1006/1016
File webkit/api/public/WebStorage.h (right):

http://codereview.chromium.org/147248/diff/1006/1016#newcode42
Line 42: 
On 2009/06/27 04:33:36, darin wrote:
> some of these methods are not super obvious.  brief comments might help. 
maybe
> add a comment to define namespaces and storage areas?

Oops...yeah, I guess I should move some of the documentation from
render_messages_internal.h over here.

http://codereview.chromium.org/147248/diff/1006/1016#newcode47
Line 47: virtual void lock(long long storageAreaId, bool* invalidateCache, long
long* bytesLeftInQuota) = 0;
On 2009/06/27 04:33:36, darin wrote:
> since so many of these methods are prefixed with storageAreaId, perhaps there
> should be a class named WebStorageArea?  then WebStorage would just have a
> method to get the WebStorageArea corresponding to the given storageAreaId?  or
> would that just add another layer for no real benefit?

I don't think that's a good idea.  I think this should mirror the IPC messages
as closely as possible and that the code to convert from an ID to an object
should be completely in WebCore.  I'm pretty sure this will be cleaner.

http://codereview.chromium.org/147248/diff/1006/1016#newcode49
Line 49: virtual unsigned length(long long storageAreaId) = 0;
On 2009/06/27 04:33:36, darin wrote:
> is length a byte count?  if so, prefer size_t.

unsigned is what WebCore uses internally.  Do you think I should submit a patch
to change it?  If not, I'd like to leave it unsigned here.

[darin (slow to review), 2009-06-29 16:53:56]

http://codereview.chromium.org/147248/diff/1006/1010
File webkit/api/public/WebKit.h (right):

http://codereview.chromium.org/147248/diff/1006/1010#newcode91
Line 91: WEBKIT_API WebStorage* webStorage();
The one in the renderer is an IPC proxy, right?  If so, then that fits with the
model I was recommending.  WebStorage::create() returns the real WebStorage
implementation, and then WebKitClient::createStorage() returns the IPC proxy
implementation in the renderer.

http://codereview.chromium.org/147248/diff/1006/1016
File webkit/api/public/WebStorage.h (right):

http://codereview.chromium.org/147248/diff/1006/1016#newcode47
Line 47: virtual void lock(long long storageAreaId, bool* invalidateCache, long
long* bytesLeftInQuota) = 0;
OK

http://codereview.chromium.org/147248/diff/1006/1016#newcode49
Line 49: virtual unsigned length(long long storageAreaId) = 0;
WebCore is not too consistent about when it uses size_t and when it uses
unsigned.  for the WebKit API, i'm trying to be consistent.  for byte counts,
size_t is what i've standardized on.  is it a problem for this to be size_t when
WebCore uses unsigned?

[jorlow, 2009-06-29 17:56:24]

http://codereview.chromium.org/147248/diff/1006/1010
File webkit/api/public/WebKit.h (right):

http://codereview.chromium.org/147248/diff/1006/1010#newcode91
Line 91: WEBKIT_API WebStorage* webStorage();
On 2009/06/29 16:53:56, darin wrote:
> The one in the renderer is an IPC proxy, right?  If so, then that fits with
the
> model I was recommending.  WebStorage::create() returns the real WebStorage
> implementation, and then WebKitClient::createStorage() returns the IPC proxy
> implementation in the renderer.

I think that should work.

http://codereview.chromium.org/147248/diff/1006/1016
File webkit/api/public/WebStorage.h (right):

http://codereview.chromium.org/147248/diff/1006/1016#newcode49
Line 49: virtual unsigned length(long long storageAreaId) = 0;
On 2009/06/29 16:53:56, darin wrote:
> WebCore is not too consistent about when it uses size_t and when it uses
> unsigned.  for the WebKit API, i'm trying to be consistent.  for byte counts,
> size_t is what i've standardized on.  is it a problem for this to be size_t
when
> WebCore uses unsigned?

I'd like them to be the same.  Overflows would be very hard to accomplish (if
even possible) but it seems like we should try anyhow.

I'll make this size_t and send in a patch to convert types.  We'll see what the
webkit guys say.

[michaeln, 2009-07-06 19:35:34]

http://codereview.chromium.org/147248/diff/1006/1011
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/1006/1011#newcode103
Line 103: return web_storage_;
Do we need to distinquish between incognito and not at this point and create a
webStorage instance aimed at the appropiate backing db file?

http://codereview.chromium.org/147248/diff/1006/1010
File webkit/api/public/WebKit.h (right):

http://codereview.chromium.org/147248/diff/1006/1010#newcode91
Line 91: WEBKIT_API WebStorage* webStorage();
+1 explicit create methods

http://codereview.chromium.org/147248/diff/1006/1016
File webkit/api/public/WebStorage.h (right):

http://codereview.chromium.org/147248/diff/1006/1016#newcode49
Line 49: virtual unsigned length(long long storageAreaId) = 0;
'length' is an item count (right), the number of key/value pairs in the area...
'size_t' is still a reasonable choice for this data type.

[jorlow, 2009-07-06 19:57:38]

There's a lot of work that remains to be done on this...I'll let you know when
it's ready for review again.

http://codereview.chromium.org/147248/diff/1006/1011
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/1006/1011#newcode103
Line 103: return web_storage_;
On 2009/07/06 19:35:34, michaeln wrote:
> Do we need to distinquish between incognito and not at this point and create a
> webStorage instance aimed at the appropiate backing db file?

Such things will be maintained in the WebKit context.

[jorlow, 2009-07-10 05:33:13]

It's not quite working yet (stuck on a really weird bug) and there are a bunch
of hacks (mostly documented with TODOs) but the overall structure is pretty much
done.

[jorlow, 2009-07-11 02:08:14]

It's working!

Remaining work for this CL:
* Put it behind a flag.

Work before it isn't behind a flag:
* Don't leak memory.
* Have a good story for resource reclamation.
* Quota support (even if the quota is a fixed size).
* Track namespace_id's for tabs (so session storage persists even if you
navigate your tab somewhere else in the mean time)
* Enable existing layout tests.
* Add new layout tests.
* Add unit tests.

[michaeln, 2009-07-14 01:31:19]

More questions than comments at this time...

http://codereview.chromium.org/147248/diff/4099/1139
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4099/1139#newcode32
Line 32: virtual WebKit::WebStorageNamespace* sessionStorageNamespace();
these are factory methods... they create new instances that the caller is
reponsible for deleting... is that right?

if so... maybe createFoo(...)

Also previous review comments had mentioned not having these factory methods on
the WebKitClient interface at all. And instead having CLASS::create(...) methods
to manufacture appropiate instances when needed. Wondering what happened to
that?

http://codereview.chromium.org/147248/diff/4099/1134
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/4099/1134#newcode116
Line 116: GetLocalStoragePath());
So we're actually creating many instances of WebStorageNamespace for the same
path.  Are these instances all functionally equivalent to one another, such that
they provide access to the same underlying collection of key/value pairs?

And if they all do result in a functaionally equivalent instances... can we just
return the ID of the a pre-existing instance. And if you pull on this thread far
enough... do we even need an ID for the "local" storage namespace at all since
there's one per profile?

This could be handled in your current IPC API with a well-known id of 0 for the
local storage namespace.

http://codereview.chromium.org/147248/diff/4099/1134#newcode125
Line 125: void DOMStorageDispatcherHost::OnCloneNamespaceId(int64 namespace_id,
Does this only apply to "session" storage namespaces, for use when creating new
browsing contexts from an existing context?

If so, maybe reflect that in the method name &| have some DCHECKs to that affect
in the method body to make that apparent to others.

http://codereview.chromium.org/147248/diff/4099/1134#newcode194
Line 194: WebStorageArea* storage_area = GetStorageArea(storage_area_id);
I think you may want some DCHECK(storage_area) and DCHECK(storage_namespace) in
here. Or perhaps even a call BadMessageReceived if an invalid ID makes its way
to this point.

Ditto for other handlers that operate on these IDs.

http://codereview.chromium.org/147248/diff/4099/1154
File chrome/renderer/renderer_webstoragenamespace_impl.cc (right):

http://codereview.chromium.org/147248/diff/4099/1154#newcode14
Line 14: namespace_id_(-1) {
maybe a const for kInvalidNamepaceId

http://codereview.chromium.org/147248/diff/4099/1152
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/147248/diff/4099/1152#newcode42
Line 42: WEBKIT_API static WebStorageNamespace* localStorageNamespace(const
WebString& path);
Just checking... 'path' points to the sqlite database file (or its directory) in
the file system where values are stored on disk... is that right?

http://codereview.chromium.org/147248/diff/4099/1152#newcode43
Line 43: WEBKIT_API static WebStorageNamespace* sessionStorageNamespace();
Where is which "session" distinguished in this interface? Or is the idea that
each returned WebStorageNamespace is long lived for the life of the "session",
and that the caller does not make a second call to get a reference to an already
existing "session" storage namespace.

http://codereview.chromium.org/147248/diff/4099/1148
File webkit/api/src/WebStorageAreaImpl.cpp (right):

http://codereview.chromium.org/147248/diff/4099/1148#newcode86
Line 86: // FIXME: Can we do any better than just passing 0 for the frame?
What purpose does the 'frame' serve to the webcore class?

http://codereview.chromium.org/147248/diff/4099/1149
File webkit/api/src/WebStorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/147248/diff/4099/1149#newcode43
Line 43: return new
WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::localStorageNamespace(path));
Does the underlying webcore class's factory method return a newly created object
or will an existing object for the given path be returned?

http://codereview.chromium.org/147248/diff/4099/1149#newcode63
Line 63: return new
WebStorageAreaImpl(m_storageNamespace->storageArea(origin.get()));
Does the underlying webcore class's storageArea() method return a newly created
object or will an existing object for the given origin be returned?

http://codereview.chromium.org/147248/diff/4099/1132
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/147248/diff/4099/1132#newcode1409
Line 1409: settings->setLocalStorageEnabled(true);
maybe enable this via a runtime flag for now, until the feature is closer to
being ready to ship

[jorlow, 2009-07-14 01:58:03]

Michael, I haven't looked at your comments yet.

I uploaded the webkit side of this.  There's definitely some cleanup to be done
in both parts.

I think I know how to solve all the "leak" issues.  On the renderer side, the
Storage*Proxy just needs to call delete on the WebStorage* classes when it's
done.  The browser side just needs to keep track of all of its WebStorage*
objects and delete them when it goes away....which I think happens when the dom
storage dispatcher host dies!

There's some issue with GYP that I need to work out, so this doesn't compile
without a little bit of massaging.

That said, I think I'll have my end of the cleanup done in the next day or two,
so please do start leaving comments!

The webkit side of this is http://codereview.chromium.org/155477

[michaeln1, 2009-07-14 02:07:53]

I'll take a closer look tomorrow too...

On Mon, Jul 13, 2009 at 6:58 PM, <jorlow@chromium.org> wrote:

> Michael, I haven't looked at your comments yet.
>
> I uploaded the webkit side of this.  There's definitely some cleanup to
> be done in both parts.
>
> I think I know how to solve all the "leak" issues.  On the renderer
> side, the Storage*Proxy just needs to call delete on the WebStorage*
> classes when it's done.  The browser side just needs to keep track of
> all of its WebStorage* objects and delete them when it goes
> away....which I think happens when the dom storage dispatcher host dies!
>
> There's some issue with GYP that I need to work out, so this doesn't
> compile without a little bit of massaging.
>
> That said, I think I'll have my end of the cleanup done in the next day
> or two, so please do start leaving comments!
>
> The webkit side of this is http://codereview.chromium.org/155477
>
>
> http://codereview.chromium.org/147248
>

[jorlow, 2009-07-14 05:51:26]

I'll start making the changes tomorrow.

http://codereview.chromium.org/147248/diff/4099/1139
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4099/1139#newcode32
Line 32: virtual WebKit::WebStorageNamespace* sessionStorageNamespace();
On 2009/07/14 01:31:19, michaeln wrote:
> these are factory methods... they create new instances that the caller is
> reponsible for deleting... is that right?
> 
> if so... maybe createFoo(...)
> 
> Also previous review comments had mentioned not having these factory methods
on
> the WebKitClient interface at all. And instead having CLASS::create(...)
methods
> to manufacture appropiate instances when needed. Wondering what happened to
> that?

I believe how it is now is what we had agreed on.  webcore -> chrome is done via
this and chrome -> webcore is via CLASS::create (well...right now it doesn't
have create in the name, but I'll change that.)

http://codereview.chromium.org/147248/diff/4099/1134
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/147248/diff/4099/1134#newcode116
Line 116: GetLocalStoragePath());
On 2009/07/14 01:31:19, michaeln wrote:
> So we're actually creating many instances of WebStorageNamespace for the same
> path.  Are these instances all functionally equivalent to one another, such
that
> they provide access to the same underlying collection of key/value pairs?
> 
> And if they all do result in a functaionally equivalent instances... can we
just
> return the ID of the a pre-existing instance. And if you pull on this thread
far
> enough... do we even need an ID for the "local" storage namespace at all since
> there's one per profile?
> 
> This could be handled in your current IPC API with a well-known id of 0 for
the
> local storage namespace.

I thought about that, but I think handling this in a general way (similar to
sessionStorage) is the better way to go.  At very least, we'd need 2 special
values: one for incognito and one for normal.  If profiles were added back in,
then we'd kind of be in trouble.  It's also possible that stuff like extensions
will want some special, separate localStorage namespace or something.  This API
also works better for others embedding it.  So no, I think it's best kept this
way.

http://codereview.chromium.org/147248/diff/4099/1134#newcode125
Line 125: void DOMStorageDispatcherHost::OnCloneNamespaceId(int64 namespace_id,
On 2009/07/14 01:31:19, michaeln wrote:
> Does this only apply to "session" storage namespaces, for use when creating
new
> browsing contexts from an existing context?
> 
> If so, maybe reflect that in the method name &| have some DCHECKs to that
affect
> in the method body to make that apparent to others.

Maybe I'll add an assert (if it's not already there) in the WebCore code and add
a comment on the WebStorageNamespace function headers.  Actually, come to think
of it, I probably need to add a lot of comments on a lot of headers.  :-)

http://codereview.chromium.org/147248/diff/4099/1134#newcode194
Line 194: WebStorageArea* storage_area = GetStorageArea(storage_area_id);
On 2009/07/14 01:31:19, michaeln wrote:
> I think you may want some DCHECK(storage_area) and DCHECK(storage_namespace)
in
> here. Or perhaps even a call BadMessageReceived if an invalid ID makes its way
> to this point.
> 
> Ditto for other handlers that operate on these IDs.

Good call.  I think I'll go with BadMessageReceived.

http://codereview.chromium.org/147248/diff/4099/1154
File chrome/renderer/renderer_webstoragenamespace_impl.cc (right):

http://codereview.chromium.org/147248/diff/4099/1154#newcode14
Line 14: namespace_id_(-1) {
On 2009/07/14 01:31:19, michaeln wrote:
> maybe a const for kInvalidNamepaceId

I like it.  The only question is where to put it.

http://codereview.chromium.org/147248/diff/4099/1152
File webkit/api/public/WebStorageNamespace.h (right):

http://codereview.chromium.org/147248/diff/4099/1152#newcode42
Line 42: WEBKIT_API static WebStorageNamespace* localStorageNamespace(const
WebString& path);
On 2009/07/14 01:31:19, michaeln wrote:
> Just checking... 'path' points to the sqlite database file (or its directory)
in
> the file system where values are stored on disk... is that right?

Yes, the directory.

Note that the browser process ignores the path the renderer asks to use (in
fact, we don't even send it across the wire).  But path is here for
completeness...so this API is useful to others who might want to embed with this
API.

http://codereview.chromium.org/147248/diff/4099/1152#newcode43
Line 43: WEBKIT_API static WebStorageNamespace* sessionStorageNamespace();
On 2009/07/14 01:31:19, michaeln wrote:
> Where is which "session" distinguished in this interface? Or is the idea that
> each returned WebStorageNamespace is long lived for the life of the "session",
> and that the caller does not make a second call to get a reference to an
already
> existing "session" storage namespace.

SessionStorage lifetimes are currently a subset of renderer process lifetimes. 
This is obviously broken.  Once LocalStorage is stable, I'll revisit this.  I
think this is fine for the first pass though.

http://codereview.chromium.org/147248/diff/4099/1148
File webkit/api/src/WebStorageAreaImpl.cpp (right):

http://codereview.chromium.org/147248/diff/4099/1148#newcode86
Line 86: // FIXME: Can we do any better than just passing 0 for the frame?
On 2009/07/14 01:31:19, michaeln wrote:
> What purpose does the 'frame' serve to the webcore class?

It's used for events.  The event handler is given a handle to the sending
window's WindowProxy object (basically a window object with looser semantics). 
It's part of the spec that we'll have to ignore...at least for now.  But it
doesn't seem very important.

http://codereview.chromium.org/147248/diff/4099/1149
File webkit/api/src/WebStorageNamespaceImpl.cpp (right):

http://codereview.chromium.org/147248/diff/4099/1149#newcode43
Line 43: return new
WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::localStorageNamespace(path));
On 2009/07/14 01:31:19, michaeln wrote:
> Does the underlying webcore class's factory method return a newly created
object
> or will an existing object for the given path be returned?

It will return an existing one.  That's why it doesn't have create in the name.

Of course this method is 'creating' a WebStorageNamespace, so I suppose I should
change the name.

http://codereview.chromium.org/147248/diff/4099/1149#newcode63
Line 63: return new
WebStorageAreaImpl(m_storageNamespace->storageArea(origin.get()));
On 2009/07/14 01:31:19, michaeln wrote:
> Does the underlying webcore class's storageArea() method return a newly
created
> object or will an existing object for the given origin be returned?

Once again, the underlying object is not necessarily created, but the wrapper
is.  I guess I should change the name.

http://codereview.chromium.org/147248/diff/4099/1132
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/147248/diff/4099/1132#newcode1409
Line 1409: settings->setLocalStorageEnabled(true);
On 2009/07/14 01:31:19, michaeln wrote:
> maybe enable this via a runtime flag for now, until the feature is closer to
> being ready to ship

I do it behind a flag in another part.

For the flag, I can either do it via the settings object (here) or by linking
directly from WebKit to V8.  I opted for the latter because it's what's been
done before, it's simpler, and it's completely isolated to Chromium.

The other option is to do it here, but then I'd need to add a
setSessionStorageEnabled flag as well.  I think I'd need to make that on by
default (since this is precedent).  I guess I'd then call set*StorageEnabled
with the status of whether the flag is there or not.

Let me know if you guys think I should switch to the latter way of doing it.

[jorlow, 2009-07-22 08:22:32]

I think this is ready for review as well.  This will go after the other I sent
out.

[darin (slow to review), 2009-07-22 16:51:36]

Looks great overall.  Just some nits...

http://codereview.chromium.org/147248/diff/4204/5065
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5065#newcode86
Line 86: WebKit::WebStorageNamespace*
BrowserWebKitClientImpl::createLocalStorageNamespace(const WebKit::WebString&
path) {
ditto... wrap at 80 chars

http://codereview.chromium.org/147248/diff/4204/5065#newcode94
Line 94: WebKit::WebStorageNamespace*
BrowserWebKitClientImpl::createSessionStorageNamespace() {
ditto

http://codereview.chromium.org/147248/diff/4204/5066
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5066#newcode31
Line 31: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
nit: this code needs to wrap at 80 chars since it is using google style

http://codereview.chromium.org/147248/diff/4204/5060
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5060#newcode108
Line 108: static bool single_process_ =
CommandLine::ForCurrentProcess()->HasSwitch(
nit: no trailing underscore since this is not a member var.

http://codereview.chromium.org/147248/diff/4204/5060#newcode118
Line 118: static bool single_process_ =
CommandLine::ForCurrentProcess()->HasSwitch(
ditto

http://codereview.chromium.org/147248/diff/4204/5061
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5061#newcode39
Line 39: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
ditto

http://codereview.chromium.org/147248/diff/4204/5067
File chrome/renderer/renderer_webstoragearea_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5067#newcode24
Line 24: void RendererWebStorageAreaImpl::lock(bool& invalidate_cache, size_t&
bytes_left_in_quota) {
nit: 80 chars

http://codereview.chromium.org/147248/diff/4204/5067#newcode29
Line 29: if (storage_area_id_ != kUninitializedStorageAreaId)
nit: i think it helps readability to use {}'s when the body of an if exceeds one
line even though it is not technically necessary in cases like this.

http://codereview.chromium.org/147248/diff/4204/5070
File chrome/renderer/renderer_webstoragenamespace_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5070#newcode18
Line 18: virtual WebKit::WebStorageArea* createStorageArea(const
WebKit::WebString& origin);
nit: 80 chars

http://codereview.chromium.org/147248/diff/4204/5062
File chrome/worker/worker_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5062#newcode66
Line 66: WebKit::WebStorageNamespace*
WorkerWebKitClientImpl::createLocalStorageNamespace(const WebKit::WebString&
path) {
nit: 80 chars

http://codereview.chromium.org/147248/diff/4204/5062#newcode71
Line 71: WebKit::WebStorageNamespace*
WorkerWebKitClientImpl::createSessionStorageNamespace() {
ditto

http://codereview.chromium.org/147248/diff/4204/5063
File chrome/worker/worker_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5063#newcode27
Line 27: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
nit: 80 chars

http://codereview.chromium.org/147248/diff/4204/5064
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/147248/diff/4204/5064#newcode131
Line 131: // DOM Storage --------------------------------------------------
nit: please insert this section in alphabetical order: up above File.

http://codereview.chromium.org/147248/diff/4204/5071
File webkit/api/src/StorageAreaProxy.cpp (right):

http://codereview.chromium.org/147248/diff/4204/5071#newcode92
Line 92: }
nit: it is good practice in webkit code to add a "// namespace WebCore" here.

[jam, 2009-07-22 18:04:20]

please have a look at the lint warnings in the future, as they cover a lot of
the simple nits (but of course, ignore it for files using webkit style)

http://codereview.chromium.org/147248/diff/4204/5060
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5060#newcode108
Line 108: static bool single_process_ =
CommandLine::ForCurrentProcess()->HasSwitch(
I think it's premature optimization to cache this return value.  Given that it
won't be called rapidly, and when it is, there'll be much slower sync IPC calls
to the browser process, we don't gain by caching it.

http://codereview.chromium.org/147248/diff/4204/5067
File chrome/renderer/renderer_webstoragearea_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5067#newcode17
Line 17: bytes_left_in_quota_(0),
note, this has to be in the same order as the header file or else gcc complains.

[jorlow, 2009-07-23 03:19:26]

I think this is pretty much good to go.

http://codereview.chromium.org/147248/diff/4204/5065
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5065#newcode86
Line 86: WebKit::WebStorageNamespace*
BrowserWebKitClientImpl::createLocalStorageNamespace(const WebKit::WebString&
path) {
On 2009/07/22 16:51:37, darin wrote:
> ditto... wrap at 80 chars

Done...but did I do it right?

http://codereview.chromium.org/147248/diff/4204/5065#newcode94
Line 94: WebKit::WebStorageNamespace*
BrowserWebKitClientImpl::createSessionStorageNamespace() {
On 2009/07/22 16:51:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/147248/diff/4204/5066
File chrome/browser/in_process_webkit/browser_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5066#newcode31
Line 31: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
On 2009/07/22 16:51:37, darin wrote:
> nit: this code needs to wrap at 80 chars since it is using google style

Done.

http://codereview.chromium.org/147248/diff/4204/5060
File chrome/renderer/renderer_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5060#newcode108
Line 108: static bool single_process_ =
CommandLine::ForCurrentProcess()->HasSwitch(
On 2009/07/22 18:04:20, John Abd-El-Malek wrote:
> I think it's premature optimization to cache this return value.  Given that it
> won't be called rapidly, and when it is, there'll be much slower sync IPC
calls
> to the browser process, we don't gain by caching it.

It pains me to look up a string in a hash table on every instantiation, but I
guess you're right that this probably isn't a performance concern.

Done.

http://codereview.chromium.org/147248/diff/4204/5060#newcode118
Line 118: static bool single_process_ =
CommandLine::ForCurrentProcess()->HasSwitch(
On 2009/07/22 16:51:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/147248/diff/4204/5061
File chrome/renderer/renderer_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5061#newcode39
Line 39: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
On 2009/07/22 16:51:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/147248/diff/4204/5067
File chrome/renderer/renderer_webstoragearea_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5067#newcode17
Line 17: bytes_left_in_quota_(0),
On 2009/07/22 18:04:20, John Abd-El-Malek wrote:
> note, this has to be in the same order as the header file or else gcc
complains.

Done.

http://codereview.chromium.org/147248/diff/4204/5067#newcode24
Line 24: void RendererWebStorageAreaImpl::lock(bool& invalidate_cache, size_t&
bytes_left_in_quota) {
On 2009/07/22 16:51:37, darin wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/147248/diff/4204/5067#newcode29
Line 29: if (storage_area_id_ != kUninitializedStorageAreaId)
On 2009/07/22 16:51:37, darin wrote:
> nit: i think it helps readability to use {}'s when the body of an if exceeds
one
> line even though it is not technically necessary in cases like this.

Done.

http://codereview.chromium.org/147248/diff/4204/5070
File chrome/renderer/renderer_webstoragenamespace_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5070#newcode18
Line 18: virtual WebKit::WebStorageArea* createStorageArea(const
WebKit::WebString& origin);
On 2009/07/22 16:51:37, darin wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/147248/diff/4204/5062
File chrome/worker/worker_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4204/5062#newcode66
Line 66: WebKit::WebStorageNamespace*
WorkerWebKitClientImpl::createLocalStorageNamespace(const WebKit::WebString&
path) {
On 2009/07/22 16:51:37, darin wrote:
> nit: 80 chars

Done, but did I do it right?

http://codereview.chromium.org/147248/diff/4204/5062#newcode71
Line 71: WebKit::WebStorageNamespace*
WorkerWebKitClientImpl::createSessionStorageNamespace() {
On 2009/07/22 16:51:37, darin wrote:
> ditto

Done.

http://codereview.chromium.org/147248/diff/4204/5063
File chrome/worker/worker_webkitclient_impl.h (right):

http://codereview.chromium.org/147248/diff/4204/5063#newcode27
Line 27: virtual WebKit::WebStorageNamespace* createLocalStorageNamespace(const
WebKit::WebString& path);
On 2009/07/22 16:51:37, darin wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/147248/diff/4204/5064
File webkit/api/public/WebKitClient.h (right):

http://codereview.chromium.org/147248/diff/4204/5064#newcode131
Line 131: // DOM Storage --------------------------------------------------
On 2009/07/22 16:51:37, darin wrote:
> nit: please insert this section in alphabetical order: up above File.

Done.

http://codereview.chromium.org/147248/diff/4204/5071
File webkit/api/src/StorageAreaProxy.cpp (right):

http://codereview.chromium.org/147248/diff/4204/5071#newcode92
Line 92: }
On 2009/07/22 16:51:37, darin wrote:
> nit: it is good practice in webkit code to add a "// namespace WebCore" here.

Done.

[darin (slow to review), 2009-07-23 05:42:24]

LGTM

http://codereview.chromium.org/147248/diff/4234/4241
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4234/4241#newcode87
Line 87: BrowserWebKitClientImpl::createLocalStorageNamespace(
hmm... it is sad when the 80 char limit forces this has to happen.  i don't know
what the convention is.  this seems fine to me though.

[darin (slow to review), 2009-07-23 05:43:29]

http://codereview.chromium.org/147248/diff/4234/4241
File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc (right):

http://codereview.chromium.org/147248/diff/4234/4241#newcode87
Line 87: BrowserWebKitClientImpl::createLocalStorageNamespace(
On 2009/07/23 05:42:24, darin wrote:
> hmm... it is sad when the 80 char limit forces this has to happen.  i don't
know
> what the convention is.  this seems fine to me though.

actually, you can avoid this awkward line breaking with some using directives at
the top of this file.  i've been writing those fairly consistently in .cc files.
 would be good to do the same here :)

[jam, 2009-07-23 05:52:23]

lgtm

On Wed, Jul 22, 2009 at 10:42 PM, <darin@chromium.org> wrote:

> LGTM
>
>
> http://codereview.chromium.org/147248/diff/4234/4241
> File chrome/browser/in_process_webkit/browser_webkitclient_impl.cc
> (right):
>
> http://codereview.chromium.org/147248/diff/4234/4241#newcode87
> Line 87: BrowserWebKitClientImpl::createLocalStorageNamespace(
> hmm... it is sad when the 80 char limit forces this has to happen.  i
> don't know what the convention is.  this seems fine to me though.


I thought the convention is to do

ReturnType
ClassName::FunctionName() {
}


>
>
> http://codereview.chromium.org/147248
>

[jorlow, 2009-07-23 22:25:23]

Need one more review.  Compare to patch set 12.  Had to teach base how to hash a
string16 and found a couple new implementations of WebKitClient that I needed to
add my new methods to.

[darin (slow to review), 2009-07-23 22:31:52]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
std::tr1::hash<string16> doesn't work?

[jorlow, 2009-07-23 22:51:16]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
On 2009/07/23 22:31:52, darin wrote:
> std::tr1::hash<string16> doesn't work?

I wish it did.  :-)

It's special cased for std::string and std::wstring.  On platforms where
string16 isn't a wstring it doesn't work.

I looked at the implementation of tr1/functional and tried to come up with a
better way, but I coudln't.  What's here is the hashing algorithm for
std::string and std::wstring's on the mac.  I'm not sure how to do it any
cleaner.

[darin (slow to review), 2009-07-23 23:06:20]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
OK... I'm probably not the best person to review this.  It'd be good to have
Mark Mentovai review this too.

At the very least, I think we should probably wrap this stuff in a #if
defined(WCHAR_T_IS_UTF32).

[jorlow, 2009-07-23 23:15:33]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
On 2009/07/23 23:06:20, darin wrote:
> OK... I'm probably not the best person to review this.  It'd be good to have
> Mark Mentovai review this too.
> 
> At the very least, I think we should probably wrap this stuff in a #if
> defined(WCHAR_T_IS_UTF32).

Split into its own review: http://codereview.chromium.org/160062

[Mark Mentovai, 2009-07-23 23:28:36]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
On 2009/07/23 23:15:33, Jeremy Orlow wrote:
> Split into its own review: http://codereview.chromium.org/160062

I picked this file over there.  Is there anything you need me to look at in this
change?

[jorlow, 2009-07-23 23:35:16]

http://codereview.chromium.org/147248/diff/5293/6131
File base/hash_tables.h (right):

http://codereview.chromium.org/147248/diff/5293/6131#newcode100
Line 100: std::size_t result = 0;
On 2009/07/23 23:28:37, Mark Mentovai wrote:
> On 2009/07/23 23:15:33, Jeremy Orlow wrote:
> > Split into its own review: http://codereview.chromium.org/160062
> 
> I picked this file over there.  Is there anything you need me to look at in
this
> change?

I don't think so.  Thanks!

[jorlow, 2009-07-23 23:59:48]

Uploaded a new version without base/hash_tables.h.

Any other issues?

[darin (slow to review), 2009-07-24 00:22:34]

LGTM