Bug 46287 – [Qt] Enable Netscape plugin metadata caching on Linux should not be set in QWebSettings::enablePersistentStorage

• Component: WebKit Qt
• Status: RESOLVED FIXED

[David Leong]

Currently calling QWebSettings::enablePersistentStorage() function also calls setPersistentMetadataCacheEnabled(true); and setPersistentMetadataCachePath(storagePath); This functionality ties the persistant storage path with the plugin database path. 

Users may want to set a unique persistant path for different origins. Doing so will nullify the usefulness of the plugin cache. The plugin cache should be unique and sharable across the entire system.

Can we change the plugin cache functionality from the persistent storage path and add a new internal API to set the plugin database path.

[David Leong]

Will post up a proposed patch.

[David Leong]

Created attachment 68434 [details]
Proposed changes.

Proposed change to move the netscape plugin path to a new private settings function.

[David Leong]

Talked to Laszlo, will make some changes to move it to QWebPage as a dynamic property. Will re-submit the patch.

[David Leong]

Created attachment 68612 [details]
Proposed changes, v2

Updated to use QObject dynamic properties.

[Kimmo Kinnunen]

(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"

[Laszlo Gombos]

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.

[Simon Hausmann]

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 :)

[David Leong]

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.

[Kimmo Kinnunen]

(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..

[Simon Hausmann]

(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 :)

[David Leong]

Created attachment 69552 [details]
patch v3

Patch v3 based on Simon's idea.

[David Leong]

Created attachment 69553 [details]
patch

Sorry, attached the wrong file. Updated patch based on Simon's idea.

[David Leong]

Created attachment 69554 [details]
patch

Not having a good day with patches... fixed again.

[Laszlo Gombos]

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.

[David Leong]

Created attachment 69645 [details]
Updated to use cache location

Works fine on my linux machine. Will try on meego to make sure too.

[David Leong]

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.

[Simon Hausmann]

Comment on attachment 69554 [details]
patch

Clearning review as David uploaded a newer patch

[Simon Hausmann]

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.

[David Leong]

Created attachment 70024 [details]
Updated again as per Simon's suggstion.

Flag out enablePersistant storage completely if desktop services is not supported.

[Andreas Kling]

Comment on attachment 70024 [details]
Updated again as per Simon's suggstion.

Please tick the "patch" checkbox when uploading patches.

[Andreas Kling]

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/

[David Leong]

Created attachment 70142 [details]
Fixed according to comments.

Thanks for the quick review. I've fixed the patch as per the comments.

[Laszlo Gombos]

Comment on attachment 70142 [details]
Fixed according to comments.

Looks good to me, r+.

[WebKit Commit Bot]

Comment on attachment 70142 [details]
Fixed according to comments.

Clearing flags on attachment: 70142

Committed r69636: <http://trac.webkit.org/changeset/69636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r69636 cherry-picked into qtwebkit-2.1 with commit 1551146 <http://gitorious.org/webkit/qtwebkit/commit/1551146>