Bug 50430 – [GTK] The webkit cache model needs to be set when WebFrameLoaderClient::didPerformFirstNavigation() is called

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Joone Hur]

Currently, the webkit cache model is set when webkit_init() is called in WebKitGtk+, but other ports set the cache model when didPerformFirstNavigation() callback is called. I think that it seems better to set the cache model at the explicit time like other ports.

Also, we need to consider adding a new property(webview-cache-model) to WebKitWebSettings instead of calling the cache model APIs directly.

[Joone Hur]

Created attachment 75449 [details]
Proposed Patch

[Martin Robinson]

Comment on attachment 75449 [details]
Proposed Patch

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

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:696
> +    if (cacheModel != WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER && cacheModel != WEBKIT_CACHE_MODEL_WEB_BROWSER)

I think it makes sense to expalin this logic with a comment. The Mac port explictly tracks whether or not an API consumer has set a cache model via the  automaticallyDetectsCacheModel setting. That might provide a self-documenting alternative.

[Joone Hur]

Created attachment 75656 [details]
Proposed Patch2

Martin suggested me to add WEBKIT_CACHE_MODEL_DEFAULT enumeration value in order to check if the cache model is not determined, so I applied this idea to the patch. Thanks!

[Martin Robinson]

Comment on attachment 75656 [details]
Proposed Patch2

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

I think some of the comments should be shuffled around and expanded.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:698
> +    // Page cache capacity (in pages). Comment from Mac port:
> +    // (Research indicates that value / page drops substantially after 3 pages.)

This can should probably move to the relevant line in webkit_set_cache_model (the one that sets 3 cache size to 3 pages for WEBKIT_CACHE_MODEL_WEB_BROWSER).

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:699
> +    // FIXME: Expose this with an API and/or calculate based on available resources

I think you're right that we can just remove this line now.

> WebKit/gtk/webkit/webkitwebview.h:54
> +    WEBKIT_CACHE_MODEL_DEFAULT,

We should include some documentation with webkit_web_view_set_cache_model explaining that the default is WEB_BROWSER.

[Martin Robinson]

Alternatively, it makes sense to use an enum comment block: http://library.gnome.org/devel/gtk-doc-manual/stable/documenting_symbols.html.en

[Joone Hur]

Created attachment 75660 [details]
Proposed Patch3

The enum comment block has been added. Thanks!

[Martin Robinson]

Committed r73353: <http://trac.webkit.org/changeset/73353>

[Martin Robinson]

Comment on attachment 75660 [details]
Proposed Patch3

Landed after some minor changes.