Bug 21051

Summary: Databases panel should turn into a general Storage panel
Product: WebKit Reporter: Timothy Hatcher <timothy>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, keishi, kmccullough, mike, rik, timothy, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Add localStorage and sessionStorage to databases panel.
none
This patch is addressing comment #2.
timothy: review-
Addressing comment #7
timothy: review+
The new image none

Description Timothy Hatcher 2008-09-24 02:03:48 PDT
The Databases panel should turn into a general storage panel and include Local and Session storage as well as Cookies.
Comment 1 Keishi Hattori 2008-09-27 23:30:34 PDT
*** Bug 19882 has been marked as a duplicate of this bug. ***
Comment 2 Yael 2008-12-29 11:43:48 PST
Created attachment 26293 [details]
Add localStorage and sessionStorage to databases panel.

Add localStorage and sessionStorage to databases panel.
Comment 3 Brady Eidson 2008-12-29 16:18:26 PST
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.
Comment 4 Yael 2008-12-30 10:45:49 PST
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,
Comment 5 Sam Weinig 2008-12-30 11:21:33 PST
Just a small nit, we prefer the spelling DOM to Dom.
Comment 6 Yael 2009-01-07 05:37:55 PST
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!
Comment 7 Timothy Hatcher 2009-02-03 13:18:00 PST
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.
Comment 8 Yael 2009-02-04 11:24:18 PST
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.
Comment 9 Yael 2009-02-04 11:25:25 PST
Created attachment 27322 [details]
The new image
Comment 10 Yael 2009-02-05 07:54:25 PST
Thank you for approving this patch. It was real fun working on the Web Inspector :-)

Now I need help committing this patch :-)
Comment 11 Brent Fulgham 2009-02-07 21:22:00 PST
Committed in http://trac.webkit.org/changeset/40768.