Bug 46287

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 QtAssignee: 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 Flags
Proposed changes.
none
Proposed changes, v2
none
patch v3
none
patch
none
patch
none
Updated to use cache location
none
Updated again as per Simon's suggstion.
kling: review-
Fixed according to comments. none

Description David Leong 2010-09-22 11:59:00 PDT
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.
Comment 1 David Leong 2010-09-22 11:59:30 PDT
Will post up a proposed patch.
Comment 2 David Leong 2010-09-22 13:56:31 PDT
Created attachment 68434 [details]
Proposed changes.

Proposed change to move the netscape plugin path to a new private settings function.
Comment 3 David Leong 2010-09-22 14:04:54 PDT
Talked to Laszlo, will make some changes to move it to QWebPage as a dynamic property. Will re-submit the patch.
Comment 4 David Leong 2010-09-23 16:03:42 PDT
Created attachment 68612 [details]
Proposed changes, v2

Updated to use QObject dynamic properties.
Comment 5 Kimmo Kinnunen 2010-09-28 04:12:24 PDT
(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"
Comment 6 Laszlo Gombos 2010-09-28 04:28:00 PDT
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.
Comment 7 Simon Hausmann 2010-09-29 08:15:49 PDT
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 :)
Comment 8 David Leong 2010-09-29 11:08:40 PDT
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.
Comment 9 Kimmo Kinnunen 2010-09-29 21:56:13 PDT
(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..
Comment 10 Simon Hausmann 2010-09-30 01:16:36 PDT
(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 :)
Comment 11 David Leong 2010-10-01 18:06:01 PDT
Created attachment 69552 [details]
patch v3

Patch v3 based on Simon's idea.
Comment 12 David Leong 2010-10-01 18:09:15 PDT
Created attachment 69553 [details]
patch

Sorry, attached the wrong file. Updated patch based on Simon's idea.
Comment 13 David Leong 2010-10-01 18:16:32 PDT
Created attachment 69554 [details]
patch

Not having a good day with patches... fixed again.
Comment 14 Laszlo Gombos 2010-10-04 09:01:23 PDT
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.
Comment 15 David Leong 2010-10-04 10:06:58 PDT
Created attachment 69645 [details]
Updated to use cache location

Works fine on my linux machine. Will try on meego to make sure too.
Comment 16 David Leong 2010-10-04 10:21:55 PDT
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 17 Simon Hausmann 2010-10-05 01:28:23 PDT
Comment on attachment 69554 [details]
patch

Clearning review as David uploaded a newer patch
Comment 18 Simon Hausmann 2010-10-05 01:30:06 PDT
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.
Comment 19 David Leong 2010-10-06 18:35:14 PDT
Created attachment 70024 [details]
Updated again as per Simon's suggstion.

Flag out enablePersistant storage completely if desktop services is not supported.
Comment 20 Andreas Kling 2010-10-06 19:07:21 PDT
Comment on attachment 70024 [details]
Updated again as per Simon's suggstion.

Please tick the "patch" checkbox when uploading patches.
Comment 21 Andreas Kling 2010-10-06 22:42:46 PDT
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/
Comment 22 David Leong 2010-10-07 13:27:34 PDT
Created attachment 70142 [details]
Fixed according to comments.

Thanks for the quick review. I've fixed the patch as per the comments.
Comment 23 Laszlo Gombos 2010-10-12 21:13:03 PDT
Comment on attachment 70142 [details]
Fixed according to comments.

Looks good to me, r+.
Comment 24 WebKit Commit Bot 2010-10-12 21:29:32 PDT
Comment on attachment 70142 [details]
Fixed according to comments.

Clearing flags on attachment: 70142

Committed r69636: <http://trac.webkit.org/changeset/69636>
Comment 25 WebKit Commit Bot 2010-10-12 21:29:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Ademar Reis 2010-10-15 07:01:28 PDT
Revision r69636 cherry-picked into qtwebkit-2.1 with commit 1551146 <http://gitorious.org/webkit/qtwebkit/commit/1551146>