[webkit-reviews] review denied: [Bug 25376] Refactor localStorage code for use in multi-process browsers : [Attachment 31395] patch 1 - v4

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 25376: Refactor localStorage code for use in multi-process browsers
https://bugs.webkit.org/show_bug.cgi?id=25376

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
This refactoring seems like a good idea to me.	Reading over this change,
I spotted some stylistic things you'll want to fix.  Marking R- on account
of those.


> Index: WebCore/storage/LocalStorage.cpp
...
>  LocalStorage::LocalStorage(const String& path)
>      : m_path(path.copy())
> +    , m_syncManager(0)

nit: no need for this line since RefPtr's default constructor is equivalent.
it is commonplace in webcore to exclude lines like this.


>  {
> -    // 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?


> @@ -92,54 +88,18 @@ PassRefPtr<StorageArea> LocalStorage::st
...
> -    storageArea = LocalStorageArea::create(origin, this);
> +    

nit: ^^^ looks like some extraneous white spaces on that new line.


> 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.


> 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.


>      if (m_thread)
>	   m_thread->scheduleSync(area);
>  }

> Index: WebCore/storage/StorageSyncManager.h
...
> +    class StorageSyncManager: public ThreadSafeShared<StorageSyncManager> {

nit: need a space before the ":"