Bug 50430 - [GTK] The webkit cache model needs to be set when WebFrameLoaderClient::didPerformFirstNavigation() is called
Summary: [GTK] The webkit cache model needs to be set when WebFrameLoaderClient::didPe...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 50004
  Show dependency treegraph
Reported: 2010-12-02 18:13 PST by Joone Hur
Modified: 2010-12-06 03:20 PST (History)
3 users (show)

See Also:

Proposed Patch (2.38 KB, patch)
2010-12-02 18:51 PST, Joone Hur
no flags Details | Formatted Diff | Diff
Proposed Patch2 (3.44 KB, patch)
2010-12-06 01:18 PST, Joone Hur
mrobinson: review-
Details | Formatted Diff | Diff
Proposed Patch3 (3.87 KB, patch)
2010-12-06 02:56 PST, Joone Hur
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joone Hur 2010-12-02 18:13:50 PST
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.
Comment 1 Joone Hur 2010-12-02 18:51:21 PST
Created attachment 75449 [details]
Proposed Patch
Comment 2 Martin Robinson 2010-12-03 07:00:11 PST
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

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.
Comment 3 Joone Hur 2010-12-06 01:18:53 PST
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!
Comment 4 Martin Robinson 2010-12-06 01:30:52 PST
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

We should include some documentation with webkit_web_view_set_cache_model explaining that the default is WEB_BROWSER.
Comment 5 Martin Robinson 2010-12-06 01:31:56 PST
Alternatively, it makes sense to use an enum comment block: http://library.gnome.org/devel/gtk-doc-manual/stable/documenting_symbols.html.en
Comment 6 Joone Hur 2010-12-06 02:56:30 PST
Created attachment 75660 [details]
Proposed Patch3

The enum comment block has been added. Thanks!
Comment 7 Martin Robinson 2010-12-06 03:18:43 PST
Committed r73353: <http://trac.webkit.org/changeset/73353>
Comment 8 Martin Robinson 2010-12-06 03:19:31 PST
Comment on attachment 75660 [details]
Proposed Patch3

Landed after some minor changes.