Bug 21051 – Databases panel should turn into a general Storage panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Timothy Hatcher]

The Databases panel should turn into a general storage panel and include Local and Session storage as well as Cookies.

[Keishi Hattori]

*** Bug 19882 has been marked as a duplicate of this bug. ***

[Yael]

Created attachment 26293 [details]
Add localStorage and sessionStorage to databases panel.

Add localStorage and sessionStorage to databases panel.

[Brady Eidson]

Comment on attachment 26293 [details]
Add localStorage and sessionStorage to databases panel.

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.

[Yael]

Created attachment 26312 [details]
This patch is addressing comment #2.

This patch is addressing comment #2 by moving the call to InspectorController from sessionStorage and localStorage to DomWindow,

[Sam Weinig]

Just a small nit, we prefer the spelling DOM to Dom.

[Yael]

My patch below was of course related to comment #3, not comment #2,
Brady, others, do you have some time to review the rest of the patch? thanks!

[Timothy Hatcher]

Comment on attachment 26312 [details]
This patch is addressing comment #2.

This patch is looking great! Sorry it took so long to get a review. Marking as r- for now to get some things fixed based on the comments below.

> +    page->inspectorController()->didUseDomStorage(storageArea.get(), false, m_frame);

As Sam mentioned, we prefer the spelling DOM to Dom when in the middle of name, or dom if it is at the beginning. There are a lot of occurrences of this that should be changed to DOM.

> +    this.localStorageListTreeElement = new WebInspector.SidebarSectionTreeElement(WebInspector.UIString("LOCAL-STORAGE"), {}, true);
> +    this.sessionStorageListTreeElement = new WebInspector.SidebarSectionTreeElement(WebInspector.UIString("SESSION-STORAGE"), {}, true);

I think these would read better without the hyphen, so "LOCAL STORAGE" and "SESSION STORAGE".

> +    content: url(Images/domStorage.png);

What does this icon look like? Who created it? Can it be freely added to WebKit under the BSD license?

> -.database-view {
> +.database-view, .domstorage-view {

I think it might be better to rename .database-view to something generic like .storage-view, so all the selectors don't need to be complex multi-selectors.

You also need to add the new files to the WebCore.vcproj.

[Yael]

Created attachment 27321 [details]
Addressing comment #7

This patch is addressing comment #7. 
WRT the image, I drew it based on databasetable.png. Nobody should claim ownership for it and it should be safe to add the image to the project.

[Yael]

Created attachment 27322 [details]
The new image

[Yael]

Thank you for approving this patch. It was real fun working on the Web Inspector :-)

Now I need help committing this patch :-)

[Brent Fulgham]

Committed in http://trac.webkit.org/changeset/40768.