Refactor DOM Storage to pave the way for events and locking.

Refactor DOM storage to be more object oriented.  All the DOMStorageDispatcher hosts (which are each owned by one ResourceMessageFilter) for the same profile share a WebKit context, and each one of those contexts owns a DOMStorageContext.  The DOMStorageContext owns storage namespace objects which own storage area objects which wrap their WebKit counterparts.

Not only is this cleaner code wise and more efficient (we're not duplicating WebStorageNamespaces and Areas for each DOMStorageDispatcherHost) but this is necessary for events and locking.

TEST=none
BUG=none


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

[michaeln, 2009-09-03 20:52:02]

This looks close to me. I saw a couple of potentially material things, but
mostly some editorial comments.

http://codereview.chromium.org/192003/diff/6001/7011
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/192003/diff/6001/7011#newcode28
Line 28: int64 AllocateStorageNamespaceId() { return
++last_storage_namespace_id_; }
since this class manufactures the namespaces, can the namespace id generation be
privately assigned when the manufacturing happens?

http://codereview.chromium.org/192003/diff/6001/7011#newcode48
Line 48: static const int64 kFirstStorageAreaId = 1;
can these kFirst values just be const... no static?

http://codereview.chromium.org/192003/diff/6001/7004
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/192003/diff/6001/7004#newcode327
Line 327: size_t bytes_left_in_quota = 9999999;
interesting quota :)

http://codereview.chromium.org/192003/diff/6001/7004#newcode338
Line 338: DCHECK(!shutdown_);
would it make sense to have this DCHECK outside of the if?

Also, i'm not sure the cached dom_storage_context_ data member buys you anything
really. Consider inlining this method and just always calling thru to the
context to retrieve the ptr?

http://codereview.chromium.org/192003/diff/6001/7005
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/192003/diff/6001/7005#newcode57
Line 57: DOMStorageContext* Context();
much nicer encapsulation of this stuff in the new c++ class

http://codereview.chromium.org/192003/diff/6001/7009
File chrome/browser/in_process_webkit/storage_area.h (right):

http://codereview.chromium.org/192003/diff/6001/7009#newcode34
Line 34: string16 origin_;
would this be better represented as a GURL?

http://codereview.chromium.org/192003/diff/6001/7009#newcode49
Line 49: struct hash<StorageArea*> {
ah... the trick to using ptr types in hash maps :)

http://codereview.chromium.org/192003/diff/6001/7009#newcode51
Line 51: return reinterpret_cast<std::size_t>(p);
is size_t guaranteed to be the same size as a ptr type... i suppose
reinterpret_cast would complain if it were not... just checking?

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode62
Line 62: delete iter->second;
++iter; ?

http://codereview.chromium.org/192003/diff/6001/7012#newcode63
Line 63: }
alternatively... if the storageArea dtor unregisterted itself on destruction,
you may be able to avoid iterating here.

http://codereview.chromium.org/192003/diff/6001/7013
File chrome/browser/in_process_webkit/storage_namespace.h (right):

http://codereview.chromium.org/192003/diff/6001/7013#newcode29
Line 29: StorageArea* GetStorageArea(const string16& origin);
maybe use GURLs to represent origins?

http://codereview.chromium.org/192003/diff/6001/7013#newcode55
Line 55: bool is_local_storage_;
consider an enumerated type for this... a little more future proof (for the day
when new flavor of namespace is introduced).

enum NamespaceType {
  LOCAL_STORAGE,
  SESSION_STORAGE
};

and maybe a namespece_type() public accessor

http://codereview.chromium.org/192003/diff/6001/7006
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7006#newcode11
Line 11: class Cleanup : public base::RefCountedThreadSafe<Cleanup> {
Generally, i like the cleanup arrangment. You may be able to accomplish the same
behavior with a little less code... just a suggestion...

static void CleanupDOMStorageContext(cntx) {
  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::WEBKIT));
  delete cntx;
}

if (dom_storage_context_.get())
  loop->PostTask(FROM_HERE, NewRunnableFunction(
      &CleanupDOMStorageContext, dom_storage_context_.release());

... with that arrangment, you'd lose out on the DCHECK to ensure that the
Cleanup task ran, but otherwise i think this is functionally equivalent.

http://codereview.chromium.org/192003/diff/6001/7006#newcode37
Line 37: // WebKit thread.  Luckily we're guarenteed that the WebKit thread is
still
spelling... guaranteed

http://codereview.chromium.org/192003/diff/6001/7001
File chrome/browser/in_process_webkit/webkit_context.h (right):

http://codereview.chromium.org/192003/diff/6001/7001#newcode32
Line 32: const bool is_incognito_;
const is nice :)

[jorlow, 2009-09-04 02:35:27]

Uploaded a new version.

Darin, it'd be great if you could look at this soon.  It's blocking a couple
other patches of mine for events and locking.

http://codereview.chromium.org/192003/diff/6001/7011
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/192003/diff/6001/7011#newcode28
Line 28: int64 AllocateStorageNamespaceId() { return
++last_storage_namespace_id_; }
On 2009/09/03 20:52:02, michaeln wrote:
> since this class manufactures the namespaces, can the namespace id generation
be
> privately assigned when the manufacturing happens?

We could.  If we did this, then we'd add a setId method to StorageArea and
StorageNamespace and calling RegisterStorageNamespace and RegisterStorageArea
would call it.

Personally, I kind of prefer the id being set on initialization of the classes. 
(i.e. the way it is now.)  I could I could even make it const.

http://codereview.chromium.org/192003/diff/6001/7011#newcode48
Line 48: static const int64 kFirstStorageAreaId = 1;
On 2009/09/03 20:52:02, michaeln wrote:
> can these kFirst values just be const... no static?

Why shouldn't they be static.  If they're static, they're certainly all inlined.
 I'm not sure if this is true if they're a member variable (and thus they may
take up more space).  I don't see any downsides to being static, though.

http://codereview.chromium.org/192003/diff/6001/7004
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/192003/diff/6001/7004#newcode327
Line 327: size_t bytes_left_in_quota = 9999999;
On 2009/09/03 20:52:02, michaeln wrote:
> interesting quota :)

This will be solved in the next patch (which is pretty much already written). 
All this stuff is pretty much just an artifact of me splitting this patch. 
Things are no worse and no better than the way it was done before, but I didn't
feel like adding in plumbing that was about to go away soon and was a hack
anyway.

http://codereview.chromium.org/192003/diff/6001/7004#newcode338
Line 338: DCHECK(!shutdown_);
On 2009/09/03 20:52:02, michaeln wrote:
> would it make sense to have this DCHECK outside of the if?
> 
> Also, i'm not sure the cached dom_storage_context_ data member buys you
anything
> really. Consider inlining this method and just always calling thru to the
> context to retrieve the ptr?

Hmm....I guess shutdown should go out.

And yeah, I guess this might be a pre-mature optization.  The function was more
substantial at one point, but I guess it's fairly worthless now....especially if
it's not inlined.

http://codereview.chromium.org/192003/diff/6001/7005
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/192003/diff/6001/7005#newcode57
Line 57: DOMStorageContext* Context();
On 2009/09/03 20:52:02, michaeln wrote:
> much nicer encapsulation of this stuff in the new c++ class

I don't understand.  Is this a complement or something I need to do?  :-)

http://codereview.chromium.org/192003/diff/6001/7009
File chrome/browser/in_process_webkit/storage_area.h (right):

http://codereview.chromium.org/192003/diff/6001/7009#newcode34
Line 34: string16 origin_;
On 2009/09/03 20:52:02, michaeln wrote:
> would this be better represented as a GURL?

I don't know.  It's expressed as a SecurityOrigin in WebKit.  Can we cleanly and
quickly convert between the two?  Can we efficiently do equals checks between
two GURLs?  If so, sure.  If not, I think the way it is now is probably better
since we don't need any of the more advanced features of GURL.

http://codereview.chromium.org/192003/diff/6001/7009#newcode51
Line 51: return reinterpret_cast<std::size_t>(p);
On 2009/09/03 20:52:02, michaeln wrote:
> is size_t guaranteed to be the same size as a ptr type... i suppose
> reinterpret_cast would complain if it were not... just checking?

I believe so.  But even if it isn't, it's fine...it'll just truncate.  We do
this many places.

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode62
Line 62: delete iter->second;
On 2009/09/03 20:52:02, michaeln wrote:
> ++iter; ?

That's very worry some...does that mean we're never hitting this code?  I'll
investigate.

http://codereview.chromium.org/192003/diff/6001/7012#newcode63
Line 63: }
On 2009/09/03 20:52:02, michaeln wrote:
> alternatively... if the storageArea dtor unregisterted itself on destruction,
> you may be able to avoid iterating here.

It'd then need a pointer to dom_storage_context_.  Which isn't a big deal, but
seemed unnecessary.

http://codereview.chromium.org/192003/diff/6001/7013
File chrome/browser/in_process_webkit/storage_namespace.h (right):

http://codereview.chromium.org/192003/diff/6001/7013#newcode29
Line 29: StorageArea* GetStorageArea(const string16& origin);
On 2009/09/03 20:52:02, michaeln wrote:
> maybe use GURLs to represent origins?

Addressed this in another comment.

http://codereview.chromium.org/192003/diff/6001/7013#newcode55
Line 55: bool is_local_storage_;
On 2009/09/03 20:52:02, michaeln wrote:
> consider an enumerated type for this... a little more future proof (for the
day
> when new flavor of namespace is introduced).
> 
> enum NamespaceType {
>   LOCAL_STORAGE,
>   SESSION_STORAGE
> };
> 
> and maybe a namespece_type() public accessor

I'm not very concerned about future proofing (this would only be a small amount
of the work if another storage type were added), but I guess this would be a bit
cleaner.  I'll look into doing it in a future patch.

http://codereview.chromium.org/192003/diff/6001/7006
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7006#newcode11
Line 11: class Cleanup : public base::RefCountedThreadSafe<Cleanup> {
On 2009/09/03 20:52:02, michaeln wrote:
> Generally, i like the cleanup arrangment. You may be able to accomplish the
same
> behavior with a little less code... just a suggestion...
> 
> static void CleanupDOMStorageContext(cntx) {
>   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::WEBKIT));
>   delete cntx;
> }
> 
> if (dom_storage_context_.get())
>   loop->PostTask(FROM_HERE, NewRunnableFunction(
>       &CleanupDOMStorageContext, dom_storage_context_.release());
> 
> ... with that arrangment, you'd lose out on the DCHECK to ensure that the
> Cleanup task ran, but otherwise i think this is functionally equivalent.

I'll do this when I get back from lunch.

http://codereview.chromium.org/192003/diff/6001/7006#newcode37
Line 37: // WebKit thread.  Luckily we're guarenteed that the WebKit thread is
still
On 2009/09/03 20:52:02, michaeln wrote:
> spelling... guaranteed

Done.

[darin (slow to review), 2009-09-04 05:44:37]

Please update the CL description with details about what is changing.  I think
the comments about what to look for in the review are probably not interesting
to persist in the svn logs once this is committed, so it'd probably be good to
trim those out.

[darin (slow to review), 2009-09-04 06:06:41]

http://codereview.chromium.org/192003/diff/6001/7010
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7010#newcode31
Line 31: FilePath dir_path = data_path.empty() ? FilePath()
nit: like this seems a bit cleaner to me:

  FilePath dir_path;
  if (!data_path.empty())
    dir_path = data_path.AppendASCII("localStorage");

No need to assign dir_path to the empty FilePath.

http://codereview.chromium.org/192003/diff/6001/7011
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/192003/diff/6001/7011#newcode14
Line 14: 
It would be good to add a comment here giving some info about what this class is
for.

http://codereview.chromium.org/192003/diff/6001/7011#newcode48
Line 48: static const int64 kFirstStorageAreaId = 1;
On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > can these kFirst values just be const... no static?
> 
> Why shouldn't they be static.  If they're static, they're certainly all
inlined.
>  I'm not sure if this is true if they're a member variable (and thus they may
> take up more space).  I don't see any downsides to being static, though.

I'm pretty sure the 'static' keyword is unnecessary here.  I don't think they
become instance variables if you leave off the static.

http://codereview.chromium.org/192003/diff/6001/7004
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/192003/diff/6001/7004#newcode141
Line 141: StorageNamespace* existing_namespace = Context()->GetStorageNamespace(
nit: more common chrome style would be to do it like this:

  StorageNamespace* existing_namespace =
      Context()->GetStorageNamespace(namespace_id);

http://codereview.chromium.org/192003/diff/6001/7004#newcode160
Line 160: StorageNamespace* storage_namespace = Context()->GetStorageNamespace(
ditto

http://codereview.chromium.org/192003/diff/6001/7004#newcode180
Line 180: StorageNamespace* storage_namespace = Context()->GetStorageNamespace(
ditto

http://codereview.chromium.org/192003/diff/6001/7005
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/192003/diff/6001/7005#newcode57
Line 57: DOMStorageContext* Context();
On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > much nicer encapsulation of this stuff in the new c++ class
> 
> I don't understand.  Is this a complement or something I need to do?  :-)

i read it as a complement.  i concur!

http://codereview.chromium.org/192003/diff/6001/7009
File chrome/browser/in_process_webkit/storage_area.h (right):

http://codereview.chromium.org/192003/diff/6001/7009#newcode26
Line 26: bool* quotaException);
nit: quota_exception

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode25
Line 25: WebString path =
webkit_glue::FilePathStringToWebString(dir_path.value());
maybe we should have a FilePathToWebString function :)

http://codereview.chromium.org/192003/diff/6001/7006
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7006#newcode44
Line 44: loop->PostTask(FROM_HERE, NewRunnableMethod(cleanup, &Cleanup::OnRun));
just call loop->DeleteSoon(FROM_HERE, dom_storage_context_.release());

[jorlow, 2009-09-04 06:08:23]

Sure...but what about the rest of the patch?

On Fri, Sep 4, 2009 at 2:44 PM, <darin@chromium.org> wrote:

> Please update the CL description with details about what is changing.  I
> think the comments about what to look for in the review are probably not
> interesting to persist in the svn logs once this is committed, so it'd
> probably be good to trim those out.
>
>
> http://codereview.chromium.org/192003
>

[michaeln, 2009-09-04 06:56:07]

http://codereview.chromium.org/192003/diff/6001/7005
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h (right):

http://codereview.chromium.org/192003/diff/6001/7005#newcode57
Line 57: DOMStorageContext* Context();
On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > much nicer encapsulation of this stuff in the new c++ class
> 
> I don't understand.  Is this a complement or something I need to do?  :-)

Let me be more clear
(This is a) much nicer encapsulation of this stuff in the new c++ class (, I 
like it.)

http://codereview.chromium.org/192003/diff/6001/7009
File chrome/browser/in_process_webkit/storage_area.h (right):

http://codereview.chromium.org/192003/diff/6001/7009#newcode34
Line 34: string16 origin_;
On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > would this be better represented as a GURL?
> 
> I don't know.  It's expressed as a SecurityOrigin in WebKit.  Can we cleanly
and
> quickly convert between the two?  Can we efficiently do equals checks between
> two GURLs?  If so, sure.  If not, I think the way it is now is probably better
> since we don't need any of the more advanced features of GURL.

scheme://host:port/ 

I think GURL probably makes more sense as a data type for this. See the
GURL::origin() method, it returns a sripped down origin. I think we can convert
between the two with the similar clean'ness and quickness as String to
std::string translations. Does SecurityOrigin have a method that returns a KURL
representation?

Equality checks between two GURLs... yes.

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode62
Line 62: delete iter->second;
please do look into this

http://codereview.chromium.org/192003/diff/6001/7012#newcode63
Line 63: }
like i said... mostly editorial comments... but... if the storage area dtor did
the unregistration... there'd be no chance for an infinite loop bug above ;)

http://codereview.chromium.org/192003/diff/6001/7013
File chrome/browser/in_process_webkit/storage_namespace.h (right):

http://codereview.chromium.org/192003/diff/6001/7013#newcode43
Line 43: OriginToStorageAreaMap origin_to_storage_area_;
When does anything get removed from this collection? For SessionStorage I
understand its not backed by disk storage, so nothing get removed until the tab
(aka session) is closed. But for LocalStorage... how to 'areas' fall out of
memory when no page from an origin is open?

http://codereview.chromium.org/192003/diff/6001/7013#newcode55
Line 55: bool is_local_storage_;
this is fine as is... funny... i vaguely recall a similar exchange on a previous
code review :)

[jorlow, 2009-09-04 13:40:49]

New version uploaded.  Thanks for the good comments!

http://codereview.chromium.org/192003/diff/6001/7010
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7010#newcode31
Line 31: FilePath dir_path = data_path.empty() ? FilePath()
On 2009/09/04 06:06:41, darin wrote:
> nit: like this seems a bit cleaner to me:
> 
>   FilePath dir_path;
>   if (!data_path.empty())
>     dir_path = data_path.AppendASCII("localStorage");
> 
> No need to assign dir_path to the empty FilePath.

I had it like this, but I changed it.  Not a big deal though, I changed it back.

http://codereview.chromium.org/192003/diff/6001/7011
File chrome/browser/in_process_webkit/dom_storage_context.h (right):

http://codereview.chromium.org/192003/diff/6001/7011#newcode14
Line 14: 
On 2009/09/04 06:06:41, darin wrote:
> It would be good to add a comment here giving some info about what this class
is
> for.

Done.

http://codereview.chromium.org/192003/diff/6001/7011#newcode48
Line 48: static const int64 kFirstStorageAreaId = 1;
On 2009/09/04 06:06:41, darin wrote:
> On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> > On 2009/09/03 20:52:02, michaeln wrote:
> > > can these kFirst values just be const... no static?
> > 
> > Why shouldn't they be static.  If they're static, they're certainly all
> inlined.
> >  I'm not sure if this is true if they're a member variable (and thus they
may
> > take up more space).  I don't see any downsides to being static, though.
> 
> I'm pretty sure the 'static' keyword is unnecessary here.  I don't think they
> become instance variables if you leave off the static.

You can't specify the constant here in the definition if you do that.  I guess I
could do it in the constructor, but that seems silly.  IIRC, we do it with
static many places, so I'd like to leave it.

http://codereview.chromium.org/192003/diff/6001/7004
File chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc (right):

http://codereview.chromium.org/192003/diff/6001/7004#newcode141
Line 141: StorageNamespace* existing_namespace = Context()->GetStorageNamespace(
On 2009/09/04 06:06:41, darin wrote:
> nit: more common chrome style would be to do it like this:
> 
>   StorageNamespace* existing_namespace =
>       Context()->GetStorageNamespace(namespace_id);

Done.

http://codereview.chromium.org/192003/diff/6001/7004#newcode160
Line 160: StorageNamespace* storage_namespace = Context()->GetStorageNamespace(
On 2009/09/04 06:06:41, darin wrote:
> ditto

Done.

http://codereview.chromium.org/192003/diff/6001/7004#newcode180
Line 180: StorageNamespace* storage_namespace = Context()->GetStorageNamespace(
On 2009/09/04 06:06:41, darin wrote:
> ditto

Done.

http://codereview.chromium.org/192003/diff/6001/7004#newcode338
Line 338: DCHECK(!shutdown_);
On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > would it make sense to have this DCHECK outside of the if?
> > 
> > Also, i'm not sure the cached dom_storage_context_ data member buys you
> anything
> > really. Consider inlining this method and just always calling thru to the
> > context to retrieve the ptr?
> 
> Hmm....I guess shutdown should go out.
> 
> And yeah, I guess this might be a pre-mature optization.  The function was
more
> substantial at one point, but I guess it's fairly worthless now....especially
if
> it's not inlined.

Done.

http://codereview.chromium.org/192003/diff/6001/7009
File chrome/browser/in_process_webkit/storage_area.h (right):

http://codereview.chromium.org/192003/diff/6001/7009#newcode26
Line 26: bool* quotaException);
On 2009/09/04 06:06:41, darin wrote:
> nit: quota_exception

Done.

http://codereview.chromium.org/192003/diff/6001/7009#newcode34
Line 34: string16 origin_;
On 2009/09/04 06:56:07, michaeln wrote:
> On 2009/09/04 02:35:27, Jeremy Orlow wrote:
> > On 2009/09/03 20:52:02, michaeln wrote:
> > > would this be better represented as a GURL?
> > 
> > I don't know.  It's expressed as a SecurityOrigin in WebKit.  Can we cleanly
> and
> > quickly convert between the two?  Can we efficiently do equals checks
between
> > two GURLs?  If so, sure.  If not, I think the way it is now is probably
better
> > since we don't need any of the more advanced features of GURL.
> 
> scheme://host:port/ 
> 
> I think GURL probably makes more sense as a data type for this. See the
> GURL::origin() method, it returns a sripped down origin. I think we can
convert
> between the two with the similar clean'ness and quickness as String to
> std::string translations. Does SecurityOrigin have a method that returns a
KURL
> representation?
> 
> Equality checks between two GURLs... yes.
> 
> 
> 
> 

Darin, what do you think?  If I do make this change, I want to do it in another
CL because it'll be fairly invasive.

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode25
Line 25: WebString path =
webkit_glue::FilePathStringToWebString(dir_path.value());
On 2009/09/04 06:06:41, darin wrote:
> maybe we should have a FilePathToWebString function :)

Any idea why it wasn't done that way to begin with?

Done.

http://codereview.chromium.org/192003/diff/6001/7012#newcode62
Line 62: delete iter->second;
On 2009/09/03 20:52:02, michaeln wrote:
> ++iter; ?

Added line.  Will look more into it before I commit.

http://codereview.chromium.org/192003/diff/6001/7012#newcode63
Line 63: }
On 2009/09/04 06:56:07, michaeln wrote:
> like i said... mostly editorial comments... but... if the storage area dtor
did
> the unregistration... there'd be no chance for an infinite loop bug above ;)

I don't see how it'd loop infinitely if there was an issue.

And I think it's fine.  I added a comment to the top of StorageArea to err on
the side of caution.

http://codereview.chromium.org/192003/diff/6001/7013
File chrome/browser/in_process_webkit/storage_namespace.h (right):

http://codereview.chromium.org/192003/diff/6001/7013#newcode43
Line 43: OriginToStorageAreaMap origin_to_storage_area_;
On 2009/09/04 06:56:07, michaeln wrote:
> When does anything get removed from this collection? For SessionStorage I
> understand its not backed by disk storage, so nothing get removed until the
tab
> (aka session) is closed. But for LocalStorage... how to 'areas' fall out of
> memory when no page from an origin is open?

It never is.  Same with WebKit.  This is one of the todos for the future.

http://codereview.chromium.org/192003/diff/6001/7013#newcode55
Line 55: bool is_local_storage_;
On 2009/09/04 06:56:07, michaeln wrote:
> this is fine as is... funny... i vaguely recall a similar exchange on a
previous
> code review :)

Oops...really?  I'll bug myself then. 
http://code.google.com/p/chromium/issues/detail?id=21070

http://codereview.chromium.org/192003/diff/6001/7006
File chrome/browser/in_process_webkit/webkit_context.cc (right):

http://codereview.chromium.org/192003/diff/6001/7006#newcode44
Line 44: loop->PostTask(FROM_HERE, NewRunnableMethod(cleanup, &Cleanup::OnRun));
On 2009/09/04 06:06:41, darin wrote:
> just call loop->DeleteSoon(FROM_HERE, dom_storage_context_.release());

Oooo...nice!

[jorlow, 2009-09-04 14:23:49]

All issues have now been addressed.  I believe this is ready to land.  Please
take another look.

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode62
Line 62: delete iter->second;
On 2009/09/04 13:40:49, Jeremy Orlow wrote:
> On 2009/09/03 20:52:02, michaeln wrote:
> > ++iter; ?
> 
> Added line.  Will look more into it before I commit.

Fixed.  Forgot to have the dom storage context delete the namespaces it owns.  I
verified that the destructors of the storage namespaces and the storage areas
are getting called.  I think we're good now.

[michaeln, 2009-09-04 17:08:22]

LGTM

* About using GURL vs std::string16 for origin, I do think that would make for a
better data type. If we buy that, and it would be an 'invasive' change, then
later is ok. 

* Sorry for the distraction with static const int (i got distracted myself with
the discussion on the webkit list around that)

http://codereview.chromium.org/192003/diff/7015/6013
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/192003/diff/7015/6013#newcode26
Line 26: while (storage_namespace_map_.begin() != storage_namespace_map_.end())
until i read the comment, i was befuddled by the iteration loop... also looks
like end local var isn't used.

maybe...

while(!map.empty())
  delete first mapped value ;

[jorlow, 2009-09-04 22:48:30]

http://codereview.chromium.org/192003/diff/7015/6013
File chrome/browser/in_process_webkit/dom_storage_context.cc (right):

http://codereview.chromium.org/192003/diff/7015/6013#newcode26
Line 26: while (storage_namespace_map_.begin() != storage_namespace_map_.end())
On 2009/09/04 17:08:22, michaeln wrote:
> until i read the comment, i was befuddled by the iteration loop... also looks
> like end local var isn't used.
> 
> maybe...
> 
> while(!map.empty())
>   delete first mapped value ;
> 

Ahh yes, that would be better.  Will do it before I commit.

[darin (slow to review), 2009-09-04 23:25:45]

On 2009/09/04 17:08:22, michaeln wrote:
> LGTM
> 
> * About using GURL vs std::string16 for origin, I do think that would make for
a
> better data type. If we buy that, and it would be an 'invasive' change, then
> later is ok. 

I didn't see any places in the code where we do anything more than compare
origins in an opaque manner (hash keys, etc.).  We are using WebString to pass
origins at the WebKit API level because I didn't want to invent
WebSecurityOrigin to mirror SecurityOrigin.  I didn't think we needed to crack
an origin to check its component parts.  If that remains true, then I think a
string is an OK data structure.

[darin (slow to review), 2009-09-04 23:26:46]

http://codereview.chromium.org/192003/diff/6001/7012
File chrome/browser/in_process_webkit/storage_namespace.cc (right):

http://codereview.chromium.org/192003/diff/6001/7012#newcode25
Line 25: WebString path =
webkit_glue::FilePathStringToWebString(dir_path.value());
On 2009/09/04 13:40:49, Jeremy Orlow wrote:
> On 2009/09/04 06:06:41, darin wrote:
> > maybe we should have a FilePathToWebString function :)
> 
> Any idea why it wasn't done that way to begin with?
> 
> Done.

Nope.  Probably someone only needed FilePathStringToWebString.

[jorlow, 2009-09-07 16:08:33]

Darin, so if I make the one change Michael mentioned (the while loop) does
it LG to you?

On Sat, Sep 5, 2009 at 8:26 AM, <darin@chromium.org> wrote:

>
> http://codereview.chromium.org/192003/diff/6001/7012
> File chrome/browser/in_process_webkit/storage_namespace.cc (right):
>
> http://codereview.chromium.org/192003/diff/6001/7012#newcode25
> Line 25: WebString path =
> webkit_glue::FilePathStringToWebString(dir_path.value());
> On 2009/09/04 13:40:49, Jeremy Orlow wrote:
>
>> On 2009/09/04 06:06:41, darin wrote:
>> > maybe we should have a FilePathToWebString function :)
>>
>
>  Any idea why it wasn't done that way to begin with?
>>
>
>  Done.
>>
>
> Nope.  Probably someone only needed FilePathStringToWebString.
>
>
> http://codereview.chromium.org/192003
>

[jorlow, 2009-09-08 03:04:59]

Changes made and gclient sync'ed.

[jorlow, 2009-09-08 03:05:46]

Actually, I think I'm just going to submit since I got one LGTM and Darin didn't
have any additional comments.  If there are any changes I still need to make,
I'll do them ASAP.