Bug 28682 – [Qt] QtWebKit Persistent API

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Laszlo Gombos]

This bug is to track and review QtWebKit persistent API - as it has been discussed on #qtwebkit.

Directions from Simon regarding QtWebKit persistency 
 - By default QtWebKit should not store any data persistently
 - There should be an easy way (that is one call) to enable WebKit persistency - that is enable all the features that might create persistent data
 - Would be nice to have a function which sets up the base directory path for all persistent data
 - Would be nice to have a default for the base directory path so that clients can just enable persistency, without worrying about the path

[Laszlo Gombos]

Created attachment 38500 [details]
first try.

Putting this up for review to collect feedback. Patch implements the followings:

- Disable LocalStorage, ApplicationCache, HTML5 offline storage by default
- If persistency is enabled the default directory for LocalStorage and ApplicationCache is now based on QDesktopServices::DataLocation and not QDesktopServices::CacheLocation
- If persistency is enabled initialize HTML5 offline storage as well - this fixed offline Storage for QtLauncher (it is crashing now)

Looking for feedback on
 - function name and signature for enablePersistentStorage
 - Is there a need/can we maintain a disablePersistentStorage function (other WebKit ports does not seems to have this option)

[Eric Seidel (no email)]

Comment on attachment 38500 [details]
first try.

Your changelog doesn't really have nearly enough description of what you're doing.

Doesn't QtWebKit have a policy of adding unit tests for changes?

[Laszlo Gombos]

Just to capture some of the discussion (in person/from IRC) with Simon and Kenneth.

One idea was to add an enum argument so that this function can be used to configure not only the default for all persistent data but also for each individual persistent feature as well. One issue with that is that some persistent features are per page (more precisely per pageGroup) some other are static for the QtWebKit instance. 

I think the function that initializes persistent directory for all persistent features needs to be static.

Also I would like to highlight that most of these persistent features - once enabled - can not be easily turned off. The test case that I proposed in https://bugs.webkit.org/show_bug.cgi?id=28836 highlight some of the specifics of the problem.

I would propose that we "position" this API as some sort of initialization API that is expected to be called only once of a lifetime of a QtWebKit instance. In fact we might want to actually enforce that so the function is a no-op for all the subsequent calls - as far as I can tell most of all the other WebKit ports already behave that way.

[Laszlo Gombos]

Created attachment 39003 [details]
second try

Filled out ChangeLog properly as Simon indicated this API looks quite good already. I'm not sure if this needs a test case as this API does not add new functionality. It is merely a convenience function that server a use-case that most QtWebKit client would want - that is to enable all persistent features.

[Ariya Hidayat]

Comment on attachment 39003 [details]
second try

Looks good, except that we need a unit test (no matter how simple it is). It's a matter of checking all settings before calling enablePersistentStorage, and then checking everything again after. Sanity checks only, but hey, it's better to do it than to regret it later :)

r- until the test is added.

[Laszlo Gombos]

Created attachment 39090 [details]
3rd. try

Added autotest as requested by Eric and Ariya.

[Simon Hausmann]

Comment on attachment 39090 [details]
3rd. try

r=me

> +++ WebKit/qt/QtLauncher/main.cpp	(working copy)
> @@ -443,7 +443,7 @@ int main(int argc, char **argv)
>  
>      QWebSettings::globalSettings()->setAttribute(QWebSettings::PluginsEnabled, true);
>      QWebSettings::globalSettings()->setAttribute(QWebSettings::DeveloperExtrasEnabled, true);
> -    QWebSettings::globalSettings()->setAttribute(QWebSettings::LocalStorageEnabled, true);
> +    QWebSettings::globalSettings()->enablePersistentStorage();
>  

> @@ -152,6 +151,7 @@ DumpRenderTree::DumpRenderTree()
>      , m_notifier(0)
>  {
>      qt_drt_overwritePluginDirectories();
> +    QWebSettings::globalSettings()->enablePersistentStorage();
>  
>      m_controller = new LayoutTestController(this);
>      connect(m_controller, SIGNAL(done()), this, SLOT(dump()));

When landing, please change these to use QWebSettings::enablePersistentStorage() instead of QWebSettings::globalSettings()->enablePersistentStorage().

One could argue that DumpRenderTree should not share the persistent storage files/path with the default browser location,
to avoid possible side-effects (towards either side :). But that's a minor detail :)

[Laszlo Gombos]

Comment on attachment 39090 [details]
3rd. try

Committed as http://trac.webkit.org/changeset/48151