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
Ada Chan
2010-04-16 11:39:21 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 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.)
Some WebKit devices may want to constrain session storage just like they do for local storage. I'll mention that in the ChangeLog. 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... Created attachment 53583 [details]
Revised patch with additional comments in Settings.h and ChangeLog.
> 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 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 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)
(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. (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. 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. If anything http://trac.webkit.org/browser/trunk/WebKit/chromium/src/StorageNamespaceProxy.cpp will need some massaging. Probably by explicitly calling the parent constructor. You can also build locally with |webkit-build --chromium| too, which might be easier than iterating with the bot. Attachment 53583 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1695208 Created attachment 53596 [details]
Patch with chromium fix
Attachment 53596 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1614594 Created attachment 53598 [details]
Patch with chromium fix take 2 - forgot WebKitTools last time.
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!
> 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));
}
(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). > 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)
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 http://trac.webkit.org/changeset/57822 might have broken Qt Linux Release minimal Looks like this broke Qt Minimal and will likely need some #ifdef QSETTINGS guards. :( (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? 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. |