Summary: | [Qt] Enable Netscape plugin metadata caching on Linux should not be set in QWebSettings::enablePersistentStorage | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Leong <david.leong> | ||||||||||||||||||
Component: | WebKit Qt | Assignee: | QtWebKit Unassigned <webkit-qt-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | ademar, commit-queue, david.leong, hausmann, kimmo.t.kinnunen, kling, laszlo.gombos | ||||||||||||||||||
Priority: | P1 | Keywords: | Qt, QtTriaged | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | Linux | ||||||||||||||||||||
Attachments: |
|
Description
David Leong
2010-09-22 11:59:00 PDT
Will post up a proposed patch. Created attachment 68434 [details]
Proposed changes.
Proposed change to move the netscape plugin path to a new private settings function.
Talked to Laszlo, will make some changes to move it to QWebPage as a dynamic property. Will re-submit the patch. Created attachment 68612 [details]
Proposed changes, v2
Updated to use QObject dynamic properties.
(In reply to comment #4) > Created an attachment (id=68612) [details] > Proposed changes, v2 > > Updated to use QObject dynamic properties. In the original patch there was API for setting the cache db dir, but it got rejected. Adding Simon to the loop, he can decide about APIs.. See: http://bugs.webkit.org/show_bug.cgi?id=43179#c12 IMHO, probably makes sense to call the API, if there will be one, QWebSettings::setPluginCachePath(const QString &location) instead of event->propertyName() == "_q_NetscapePluginCachePath" I think for most "common" use-cases enabling the PersistentMetadataCache in QWebSettings::enablePersistentStorage() make sense. For QtWRT the PersistentMetadataCache should be "system-wide" while enablePersistentStorage set on a per widget basis, which tells me that the separate API to set a the PersistentMetadataCache path is justified for the performance gain. I proposed the dynamic property as the new exported symbol idea was already shut down. Hmm, I see the point of sharing the plugin cache path between different run-time widgets, but this doesn't only apply to the plugin cache but to all other caches, too. Why don't we change the behaviour of enablePersistentStorage() to put persistent storage into the provided storage path (or the DataLocation fallback) and put all _caches_ into a (documented) directory under QDesktopServices::CacheLocation? Basically I'm questioning if we really need an API if we can choose a sane default already in WebKit :) Sounds like we want to keep the enable netscape plugin cache call in enablePersistantStorage() but we would like to use QDesktopServices to save the cache file to a shared location? I will put up an updated patch based on Simon's comments. (In reply to comment #7) > Why don't we change the behaviour of enablePersistentStorage() to put persistent storage into the provided storage path (or the DataLocation fallback) and put all _caches_ into a (documented) directory under QDesktopServices::CacheLocation? > > > Basically I'm questioning if we really need an API if we can choose a sane default already in WebKit :) Sounds very wise. Probably with "all _caches_" you mean all app-global caches. E.g. data caches like http caches, html5 app cache content, icon db, etc. probably should stay at datalocation, as they may leak app-specific info. Also need to probably hardcode the caches path to ~/.appname/caches or something if the qdesktopservices is not available (the ifdef part).. And hope that the cachelocation call produces desired results on symbian.. (In reply to comment #9) > (In reply to comment #7) > > Why don't we change the behaviour of enablePersistentStorage() to put persistent storage into the provided storage path (or the DataLocation fallback) and put all _caches_ into a (documented) directory under QDesktopServices::CacheLocation? > > > > > > Basically I'm questioning if we really need an API if we can choose a sane default already in WebKit :) > > Sounds very wise. Probably with "all _caches_" you mean all app-global caches. E.g. data caches like http caches, html5 app cache content, icon db, etc. probably should stay at datalocation, as they may leak app-specific info. > > Also need to probably hardcode the caches path to ~/.appname/caches or something if the qdesktopservices is not available (the ifdef part).. > > And hope that the cachelocation call produces desired results on symbian.. I guess it really depends on what kind of leakage we're afraid of with the wrt, but I'd say any kind of storage that is a cache that can safely be deleted could be stored in a common location. If there's a bug in WebKit leaking information when it shouldn't, then we should fix that :) Created attachment 69552 [details]
patch v3
Patch v3 based on Simon's idea.
Created attachment 69553 [details]
patch
Sorry, attached the wrong file. Updated patch based on Simon's idea.
Created attachment 69554 [details]
patch
Not having a good day with patches... fixed again.
Comment on attachment 69554 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=69554&action=review > WebKit/qt/Api/qwebsettings.cpp:1099 > + QString cachePath = QDesktopServices::storageLocation(QDesktopServices::DataLocation); Shouldn't this be QDesktopServices::CacheLocation instead ? I assume that DataLocation is not "system-wide" while CacheLocation would always point to the same location for all apps using QtWebKit. Created attachment 69645 [details]
Updated to use cache location
Works fine on my linux machine. Will try on meego to make sure too.
Confirmed QDesktopLocation::CachePath working fine on Meego as well. The database gets saved to /home/user/.cache/organization/appname. So each app will have a copy of the cache but it sharable for any QtWebkit instance in the app. Comment on attachment 69554 [details]
patch
Clearning review as David uploaded a newer patch
Comment on attachment 69645 [details] Updated to use cache location View in context: https://bugs.webkit.org/attachment.cgi?id=69645&action=review > WebKit/qt/Api/qwebsettings.cpp:1102 > +#ifndef QT_NO_DESKTOPSERVICES > + QString cachePath = QDesktopServices::storageLocation(QDesktopServices::CacheLocation); > +#else > + QString cachePath = WebCore::pathByAppendingComponent(QDir::homePath(), "QtWebkit"); > +#endif Should we really support QT_NO_DESKTOPSERVICES? IMHO we should not support persistent storage if this feature is not compiled into Qt. Created attachment 70024 [details]
Updated again as per Simon's suggstion.
Flag out enablePersistant storage completely if desktop services is not supported.
Comment on attachment 70024 [details]
Updated again as per Simon's suggstion.
Please tick the "patch" checkbox when uploading patches.
Comment on attachment 70024 [details] Updated again as per Simon's suggstion. View in context: https://bugs.webkit.org/attachment.cgi?id=70024&action=review > WebKit/qt/ChangeLog:6 > + should not be set in QWebSettings::enablePersistentStorage. What does this mean? > WebKit/qt/ChangeLog:11 > + QWebSettings::enablePersistentStorage will now store the netscape > + plugin cache to QDesktopServices::DataLocation. This is incorrect, should be QDesktopServices::CacheLocation. > WebKit/qt/Api/qwebsettings.cpp:1096 > + // Path is not configurable and uses QDesktop services by default. s/QDesktop services/QDesktopServices/ Created attachment 70142 [details]
Fixed according to comments.
Thanks for the quick review. I've fixed the patch as per the comments.
Comment on attachment 70142 [details]
Fixed according to comments.
Looks good to me, r+.
Comment on attachment 70142 [details] Fixed according to comments. Clearing flags on attachment: 70142 Committed r69636: <http://trac.webkit.org/changeset/69636> All reviewed patches have been landed. Closing bug. Revision r69636 cherry-picked into qtwebkit-2.1 with commit 1551146 <http://gitorious.org/webkit/qtwebkit/commit/1551146> |