[webkit-reviews] review requested: [Bug 25376] Refactor localStorage code for use in multi-process browsers : [Attachment 31504] patch 1 - v5

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has asked  for review:
Bug 25376: Refactor localStorage code for use in multi-process browsers
https://bugs.webkit.org/show_bug.cgi?id=25376

Attachment 31504: patch 1 - v5
https://bugs.webkit.org/attachment.cgi?id=31504&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
(In reply to comment #17)
> >  {
> > -	 // If the path is empty, we know we're never going to be using the
thread for anything, so don't start it.
> > -	 // In the future, we might also want to consider removing it from the
DOM in that case - <rdar://problem/5960470>
> > -	 if (path.isEmpty())
> > -	     return;
> 
> Can you comment on why it makes sense to delete this comment?

The comment just doesn't really fit in anywhere now.  The patch check is gone
since the manager now handles that stuff and the comment doesn't reflect the
current thinking.  (https://bugs.webkit.org/show_bug.cgi?id=25894)

Once the re-factoring is done, it'll be easier to fix the behavior for WebKit
while allowing browsers (with different philosophies) to opt out.  Doing so
earlier will just make the diffs a lot more messy.  And given that this has
been a problem since the beginning, I don't think it's a big deal to wait a bit
on it.	:-)

> > @@ -92,54 +88,18 @@ PassRefPtr<StorageArea> LocalStorage::st
> ...
> > -	 storageArea = LocalStorageArea::create(origin, this);
> > +	 
> 
> nit: ^^^ looks like some extraneous white spaces on that new line.

The spacing is as it was originally...I just removed an extra tab it appears. 
I think the spacing is proper as it is now.

> 
> 
> > Index: WebCore/storage/LocalStorageArea.cpp
> ...
> >  void LocalStorageArea::scheduleFinalSync()
> >  {
> > +	 ASSERT(isMainThread());
> > +	 if (!m_syncManager)
> > +	     return;
> > +	 
> 
> nit: ^^^ looks like some extraneous white spaces on that new line.

I guess I could remove it, but that code is logically grouped together.  It's
doing sanity checks and deciding if it should bail completely...essentially
testing assumptions which are made throughout the rest of the function.  I
don't like how it reads when you remove the space, but I'll do it if you
insist.

> 
> 
> > Index: WebCore/storage/StorageSyncManager.cpp
> ...
> > +void StorageSyncManager::close()
> ...
> > +	 
> 
> nit: ^^^ looks like some extraneous white spaces on that new line.
> 
> 
> > +bool StorageSyncManager::scheduleImport(PassRefPtr<LocalStorageArea> area)

> >  {
> >	 ASSERT(isMainThread());
> > -
> > +	 
> 
> nit: ^^^ looks like some extraneous white spaces on that new line.
> 
> 
> > +void StorageSyncManager::scheduleSync(PassRefPtr<LocalStorageArea> area)
> >  {
> >	 ASSERT(isMainThread());
> > +	 
> 
> nit: ^^^ looks like some extraneous white spaces on that new line.

Same as above...I'm putting a space between where I'm testing assumptions and
where the real code is.  This was done often in the code I started with, but I
did add in a bunch of spaces to make it all consistent.

I went ahead and made all the changes you asked for because making progress is
more important than these nits, but I think they've decreased readability and
consistency.