Bug 50004 – [GTK] Need 'webview-cache-model' property in WebKitWebSettings

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Joone Hur]

Currently, WebKitGtk+ offers the following functions to set the webview cache model,
 - webkit_get_cache_model()
 - webkit_set_cache_mode()

They are not relevant to any gobjects, so these functions seem to be exceptional.
I think we had better add a new property(webview-cache-model) to WebKitWebSettings and use the didPerformFirstNavigation callback to set the cache model instead of using the above functions directly.

This is an example of the didPerformFirstNavigation callback.

void FrameLoaderClient::didPerformFirstNavigation() const
{
    WebKitWebSettings* settings = webkit_web_view_get_settings(getViewFromFrame(m_frame));

    WebKitCacheModel cacheModel;
    g_object_get(settings, "webview-cache-model", &cacheModel, NULL);
    webViewSetCacheModel(cacheModel);
}

Mac and Windows port also set the cache model like this.

[Joone Hur]

Created attachment 74727 [details]
Proposed patch

[Xan Lopez]

Seems there's two different tasks here. One is doing things on the first navigation method, and the other would be deprecating the methods and adding the setting. I believe they should be addressed separately. FWIW, I'm not so sure that the benefit of the setting is enough to deprecate the existing functions; they might be in the webkitwebview.cpp file but they do not have an object in their API (we could create webkitglobals.cpp and move them there with the default_session getter).

[Xan Lopez]

It also seems the cache instance and settings are completely global, so I'm not sure of why would you want to constantly set the same value on first navigation?

[Xan Lopez]

Comment on attachment 74727 [details]
Proposed patch

r- mostly because I think this can be done in two steps.

[Joone Hur]

(In reply to comment #2)
> Seems there's two different tasks here. One is doing things on the first navigation method, and the other would be deprecating the methods and adding the setting. I believe they should be addressed separately. 
Okay, I will divide the patch into two.

> FWIW, I'm not so sure that the benefit of the setting is enough to deprecate the existing functions; they might be in the webkitwebview.cpp file but they do not have an object in their API (we could create webkitglobals.cpp and move them there with the default_session getter).

It seems not neat that the webkit_get/set_cache_model functions are introduced at the same level with other gobjects in the reference manual. Also, if the user want to set WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER, the webkit_set_cache model function should be called twice.

[Xan Lopez]

(In reply to comment #5)
> It seems not neat that the webkit_get/set_cache_model functions are introduced at the same level with other gobjects in the reference manual.

This could be solved by just moving the functions to a new webkitglobals.cpp fil. I agree it's ugly.

> Also, if the user want to set WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER, the webkit_set_cache model function should be called twice.

I don't understand this; why do you have to call it twice?

[Joone Hur]

> > Also, if the users want to set WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER, the webkit_set_cache_model function should be called twice.
> 
> I don't understand this; why do you have to call it twice?

I mean that we don't need to call it twice in that case. Currently, the cache model is set to WEBKIT_CACHE_MODEL_WEB_BROWSER in webkit_init(), so if we want to set WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER, we have to call webkit_set_cache_model function again. However, if we can set the cache-model in the settings, we just call webkit_set_cache_model() once after getting the cache-model property in didPerformFirstNavigation() because the users can change the cache model through the settings before calling webkit_set_cache_model().

[Joone Hur]

(In reply to comment #5)
> (In reply to comment #2)
> > Seems there's two different tasks here. One is doing things on the first navigation method, and the other would be deprecating the methods and adding the setting. I believe they should be addressed separately. 
> Okay, I will divide the patch into two.

I tried to divide this patch into about the first navigation method and deprecating the methods, but it seems impossible to divide it, because we can't access the settings when webkit_init is called.
Therefore, I think didPerformFirstNavigation() is the best place to set the cache_model, because we can give a chance for the users to change the settings before calling webkit_set_cache_model().

[Martin Robinson]

Comment on attachment 74727 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=74727&action=review

I think this is a good change. I see an issue with this patch though. The Mac port sets the cache model in didPerformFirstNavigation only if it's autodetected. Otherwise it sets the model as soon as the preferences change. We should replicate that behavior.

> WebKit/gtk/webkit/webkitprivate.h:311
> +    void webViewSetCacheModel(WebKitCacheModel model);
> +

This only seems to be used in webkitwebview.cpp, so it should be static and private to that file.

> WebKit/gtk/webkit/webkitwebview.cpp:4863
> + * Deprecated: Use WebKitWebSettings:webview-cache-model property instead.

Should be "Use the"

[Joone Hur]

 I have filed a new bug #50430 to divide the issue into two such as setting the cache model in didPerformFirstNavigation and introducing the webview-cache-model property, as suggested by Xan.

[Joone Hur]

Created attachment 75800 [details]
Proposed Patch2

The new webview-cache-model property can coexist with the current cache-model APIs,so we should recommend the user agents to use the new property instead of the cache-model APIs if this patch lands.

[Joone Hur]

Comment on attachment 75800 [details]
Proposed Patch2

it has a style problem

[Joone Hur]

Created attachment 75803 [details]
Proposed Patch3

I have submitted the patch again...

[WebKit Review Bot]

Attachment 75803 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75803 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75803 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75803 [details] did not pass style-queue:

Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128
error: Could not write new index file.
fatal: Could not reset index file to revision 'refs/remotes/trunk'.


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 75803 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

The GTK+ port of WebKit1 has been removed.