Bug 37717

Summary: Provide a way for clients to set session storage quota
Product: WebKit Reporter: Ada Chan <adachan>
Component: WebCore Misc.Assignee: Ada Chan <adachan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ddkilzer, dglazkov, dumi, eric, joepeck, jorlow, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to allow setting session storage quota
jorlow: review-
Revised patch with additional comments in Settings.h and ChangeLog.
none
Patch with chromium fix
none
Patch with chromium fix take 2 - forgot WebKitTools last time. jorlow: review+

Description Ada Chan 2010-04-16 11:39:21 PDT
Right now we have a way to set the quota for local storage but there's no way to specify a quota for session storage.  We should provide a way to set that quota for clients who care about limiting session storage size.
Comment 1 Ada Chan 2010-04-16 13:08:47 PDT
Created attachment 53554 [details]
Patch to allow setting session storage quota

Patch.  I have suspicion that this may break chromium.  Hopefully the trybot will tell me exactly what to fix.
Comment 2 Jeremy Orlow 2010-04-16 13:35:24 PDT
Comment on attachment 53554 [details]
Patch to allow setting session storage quota

Why should clients be able to set session storage quota?  If pages want to use up all the memory, they can.  And that memory goes away when the page goes away.  Are you scared about session storage being filled up and the user navigating to other origins but not closing the tab/page/window (and thus it stays used)?  I guess I could see that being a valid use case for this....but either way, the "why" should probably be explained better in the change log.  (The meat of the patch looks good tho.)
Comment 3 Ada Chan 2010-04-16 14:11:32 PDT
Some WebKit devices may want to constrain session storage just like they do for local storage.  I'll mention that in the ChangeLog.
Comment 4 Jeremy Orlow 2010-04-16 15:00:09 PDT
The reason why we constrain it for LocalStorage is because the effects last beyond just the browsing session.  With SessionStorage, all you have to do to kill any negative effects is close the browser window.  This is in line with the rest of the web.  My point is that if you're worried about memory consumption, limiting just SessionStorage doesn't do much for you.

The only thing that isn't in line with the rest of the web is that navigating to another page doesn't result in everything being cleaned up.

My point is that I'm not convinced this feature is actually useful and I'd (at very least) like these nuances to be clear in the ChangeLog.  Actually....maybe these should be comments in settings.h or something...
Comment 5 Ada Chan 2010-04-16 17:07:36 PDT
Created attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.
Comment 6 Ada Chan 2010-04-16 17:13:30 PDT
> My point is that I'm not convinced this feature is actually useful and I'd (at
> very least) like these nuances to be clear in the ChangeLog.  Actually....maybe
> these should be comments in settings.h or something...

I've added comments to Settings.h and ChangeLog.

Your comment is probably valid for the desktop environment.  However, a way to set a quota on session storage would be useful for devices that are more constrained in resources and therefore more concerned with memory consumption.
Comment 7 Jeremy Orlow 2010-04-16 17:14:12 PDT
Comment on attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.

I don't think your comments adequately explain the situation.  This helps one element of memory consumption.  And the only reason why it's interesting is that the memory used stays until the Page dies.  If that weren't true, I'd r- and mark the bug invalid.

LocalStorage and SessionStorage are often grouped together in peoples mind.  So I want to make sure that if someone's using LocalStorage quota they don't automatically think using SessionStorage quota is important to them.
Comment 8 Jeremy Orlow 2010-04-16 17:18:27 PDT
Comment on attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.

r=me

but i still think you can improve the comment a bit (per my other comment)
Comment 9 Ada Chan 2010-04-16 17:21:43 PDT
(In reply to comment #8)
> (From update of attachment 53583 [details])
> r=me
> 
> but i still think you can improve the comment a bit (per my other comment)

I've changed comment in Settings.h to be:

        // Allow clients concerned with memory consumption to set a quota on session storage
        // since the memory used won't be released until the Page is destroyed.
Comment 10 Jeremy Orlow 2010-04-16 17:24:04 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 53583 [details] [details])
> > r=me
> > 
> > but i still think you can improve the comment a bit (per my other comment)
> 
> I've changed comment in Settings.h to be:
> 
>         // Allow clients concerned with memory consumption to set a quota on
> session storage
>         // since the memory used won't be released until the Page is destroyed.

Perfect.
Comment 11 Ada Chan 2010-04-16 17:36:24 PDT
Thanks Jeremy for the review.

I really want to see the chromium trybot try this patch and make sure I don't cause any breakages.  Because there's a bug with trybots now not looking at r+ patches, I'll mark it back to ? for now.  When the trybot finishes, hopefully someone will r+ the patch again.
Comment 12 Jeremy Orlow 2010-04-16 17:49:28 PDT
If anything http://trac.webkit.org/browser/trunk/WebKit/chromium/src/StorageNamespaceProxy.cpp will need some massaging.  Probably by explicitly calling the parent constructor.
Comment 13 Jeremy Orlow 2010-04-16 17:50:52 PDT
You can also build locally with |webkit-build --chromium| too, which might be easier than iterating with the bot.
Comment 14 WebKit Review Bot 2010-04-16 18:19:26 PDT
Attachment 53583 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1695208
Comment 15 Ada Chan 2010-04-17 10:22:03 PDT
Created attachment 53596 [details]
Patch with chromium fix
Comment 16 WebKit Review Bot 2010-04-17 11:10:04 PDT
Attachment 53596 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1614594
Comment 17 Ada Chan 2010-04-17 11:19:27 PDT
Created attachment 53598 [details]
Patch with chromium fix take 2 - forgot WebKitTools last time.
Comment 18 Jeremy Orlow 2010-04-18 01:41:05 PDT
Comment on attachment 53598 [details]
Patch with chromium fix take 2 - forgot WebKitTools last time.

This will still break Chromium since it consumes the API (even though it completely compiles now).  Could you please leave the default constructor and have it call the other one with noQuota?  In fact, you could even skip adding the constructor that allows a quota to be passed in since we don't currently have any plans to add it (and let someone working on Chromium change it if we change our minds).  Thanks!
Comment 19 Ada Chan 2010-04-18 21:50:44 PDT
> Could you please leave the default constructor and
> have it call the other one with noQuota?

I'd like to get clarification on your suggestion.  Are you saying instead of changing:

PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*)

to take in a new parameter, I should just make another version that also takes in a quota

PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*, unsigned quota)

and have the original version's implementation just call the new version with noQuota?  Then the only chromium change that I need to make is passing noQuota to StorageNamespaceImpl::sessionStorageNamespace here:

WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
{
    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
}
Comment 20 Jeremy Orlow 2010-04-18 23:22:27 PDT
(In reply to comment #19)
> > Could you please leave the default constructor and
> > have it call the other one with noQuota?
> 
> I'd like to get clarification on your suggestion.  Are you saying instead of
> changing:
> 
> PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*)
> 
> to take in a new parameter, I should just make another version that also takes
> in a quota
> 
> PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page*,
> unsigned quota)
> 
> and have the original version's implementation just call the new version with
> noQuota?  Then the only chromium change that I need to make is passing noQuota
> to StorageNamespaceImpl::sessionStorageNamespace here:
> 
> WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
> {
>     return new
> WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
> }

I think what you've done in WebCore is great and shouldn't be changed.  I believe you can simply do this last part (passing in noQuota when creating the session storage StorageNamespaceImpl) and you're done.

If that doesn't make sense, it's probably because I'm missing something obvious.  If so, I'm happy to take a shot at the Chromium side of this and upload it.  Or you're free to land as is and we'll get our own build working again (since it really isn't your problem).
Comment 21 Ada Chan 2010-04-19 07:46:20 PDT
> I think what you've done in WebCore is great and shouldn't be changed.  I
> believe you can simply do this last part (passing in noQuota when creating the
> session storage StorageNamespaceImpl) and you're done.

If I don't change what I have in WebCore, I'll also need to fix:
PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page)
to take in a quota argument since I've changed the signature of StorageNamespace:;sessionStorageNamespace in WebCore.

So the entire change for chromium will look like:

--- WebKit/chromium/src/StorageNamespaceProxy.cpp	(revision 57799)
+++ WebKit/chromium/src/StorageNamespaceProxy.cpp	(working copy)
@@ -47,7 +47,7 @@ PassRefPtr<StorageNamespace> StorageName
     return adoptRef(new StorageNamespaceProxy(WebKit::webKitClient()->createLocalStorageNamespace(path, quota), LocalStorage));
 }
 
-PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page)
+PassRefPtr<StorageNamespace> StorageNamespace::sessionStorageNamespace(Page* page, unsigned quota)
 {
     WebKit::ChromeClientImpl* chromeClientImpl = static_cast<WebKit::ChromeClientImpl*>(page->chrome()->client());
     WebKit::WebViewClient* webViewClient = chromeClientImpl->webView()->client();
Index: WebKit/chromium/src/WebStorageNamespaceImpl.cpp
===================================================================
--- WebKit/chromium/src/WebStorageNamespaceImpl.cpp	(revision 57799)
+++ WebKit/chromium/src/WebStorageNamespaceImpl.cpp	(working copy)
@@ -47,7 +47,7 @@ WebStorageNamespace* WebStorageNamespace
 
 WebStorageNamespace* WebStorageNamespace::createSessionStorageNamespace()
 {
-    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace());
+    return new WebStorageNamespaceImpl(WebCore::StorageNamespaceImpl::sessionStorageNamespace(noQuota));
 }
 
 WebStorageNamespaceImpl::WebStorageNamespaceImpl(PassRefPtr<WebCore::StorageNamespace> storageNamespace)
Comment 22 Ada Chan 2010-04-19 11:40:56 PDT
Checked in r57822.  Checked in the minimum changes needed to fix chromium build as described in the last comment.

http://trac.webkit.org/changeset/57822
Comment 23 WebKit Review Bot 2010-04-19 11:48:56 PDT
http://trac.webkit.org/changeset/57822 might have broken Qt Linux Release minimal
Comment 24 Eric Seidel (no email) 2010-04-19 11:57:47 PDT
Looks like this broke Qt Minimal and will likely need some #ifdef QSETTINGS guards.  :(
Comment 25 Joseph Pecoraro 2010-04-19 12:01:12 PDT
(In reply to comment #24)
> Looks like this broke Qt Minimal and will likely need some #ifdef QSETTINGS
> guards.  :(

This could be that Qt on the build bot uses: -DENABLE_DOM_STORAGE=0
StorageMap::noQuota is inside a ENABLE(DOM_STORAGE) guard in StorageMap.h.

Eric, any way to tell if the "try" bot for Qt compiles with this flag on or off?
Comment 26 Eric Seidel (no email) 2010-04-19 12:13:11 PDT
The "try" bot (EWS) just uses "build-webkit".  The code is here:
http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/ports.py#L152

Basically results in:
build-webkit --release --qt

We may want to remove the Qt MINIMAL builder from core, as personally I find trying to support an "everything off" configuration somewhat confusing. :)  I guess it's useful to know that our #ifdefs are clean or not.