[Webkit-unassigned] [Bug 21051] Databases panel should turn into a general Storage panel

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=21051





------- Comment #3 from beidson at apple.com  2008-12-29 16:18 PDT -------
(From update of attachment 26293)
Great to see someone tackling this!  I'm not going through all of the html/js
at this time, but have a few comments.  One conceptual, and one nit-picky.

> Index: WebCore/storage/SessionStorage.h
> ===================================================================
> --- WebCore/storage/SessionStorage.h	(revision 39498)
> +++ WebCore/storage/SessionStorage.h	(working copy)
> @@ -42,7 +42,7 @@
>          static PassRefPtr<SessionStorage> create(Page*);
>          PassRefPtr<SessionStorage> copy(Page*);
>          
> -        PassRefPtr<StorageArea> storageArea(SecurityOrigin*);
> +        PassRefPtr<StorageArea> storageArea(Frame*, SecurityOrigin*);

I'm admittedly not reading through the entire patch to determine this myself,
but is having the Frame necessary for the inspector's sake?  If we're modeling
closely after the inspector's interaction with Database storage, it seems the
frame argument isn't necessary.

LocalStorage::storageArea() takes the frame argument for the sake of a future
client call since saving stuff to the users disk is a policy concern. 
SessionStorage has no such concern, and I'd rather not see it cluttered like
this without need.


> Index: WebCore/storage/SessionStorage.cpp
> ===================================================================
> --- WebCore/storage/SessionStorage.cpp	(revision 39498)
> +++ WebCore/storage/SessionStorage.cpp	(working copy)
>  
> -PassRefPtr<StorageArea> SessionStorage::storageArea(SecurityOrigin* origin)
> +PassRefPtr<StorageArea> SessionStorage::storageArea(Frame* sourceFrame, SecurityOrigin* origin)
>  {
> +    const String domain = origin->domain();
>      RefPtr<SessionStorageArea> storageArea;
> -    if (storageArea = m_storageAreaMap.get(origin))
> +    if (storageArea = m_storageAreaMap.get(origin)) {
> +        sourceFrame->page()->inspectorController()->didUseDomStorage(storageArea.get(), domain, false, sourceFrame);
>          return storageArea.release();
> +    }
>          
>      storageArea = SessionStorageArea::create(origin, m_page);
> +    sourceFrame->page()->inspectorController()->didUseDomStorage(storageArea.get(), domain, false, sourceFrame);
>      m_storageAreaMap.set(origin, storageArea);
>      return storageArea.release();
>  }

The local copy of origin->domain() seems unnecessary here, not gaining anything
by reusing it.  May as well just call the method in the two places it's needed.

> Index: WebCore/storage/LocalStorage.cpp
> ===================================================================
> --- WebCore/storage/LocalStorage.cpp	(revision 39498)
> +++ WebCore/storage/LocalStorage.cpp	(working copy)
> -
> +    const String domain = origin->domain();
>      RefPtr<LocalStorageArea> storageArea;
> -    if (storageArea = m_storageAreaMap.get(origin))
> +    if (storageArea = m_storageAreaMap.get(origin)) {
> +        sourceFrame->page()->inspectorController()->didUseDomStorage(storageArea.get(), domain, true, sourceFrame);
>          return storageArea.release();
> +    }
>          
>      storageArea = LocalStorageArea::create(origin, this);
> +    sourceFrame->page()->inspectorController()->didUseDomStorage(storageArea.get(), domain, true, sourceFrame);
>      m_storageAreaMap.set(origin, storageArea);
>      return storageArea.release();
>  }

Same comment here.


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.