Bug 24001

Summary:

[GTK] Cache control APIs

Product:

WebKit

Reporter:

Kalle Vahlman <zuh>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

adam, alex, christian, commit-queue, ht990332, jmalonzo, pnormand, pochu27, tonikitoo, webkit.review.bot, xan.lopez, xfdbse

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Attachments:

Description	Flags
This patch adds a boolean page-cache property to WebSettings for enabling/disabling the use of page cache	gustavo: review-
This patch adds API to clear the page and resource caches	none
This patch adds API to clear the page and resource caches and actually builds	gustavo: review-
Implement 3 cache models for gtk target	xan.lopez: review-
Patch upgraded and reviewed	gustavo: review-
New proposal of the patch	gustavo: review+, commit-queue: commit-queue-
Proposed patch for the cache models	none
Proposed patch	none
Proposed patch for cache models	none
Proposed patch cache models	none
Cache models proposed patch	none
Clearing cache API	none
Proposed patch	none
Cache models proposed patch	xan.lopez: review-
Cache models proposed patch	none

Kalle Vahlman

Reported 2009-02-18 04:15:16 PST

There's a certain amount of control applications want to have over the caches that WebKit has, most importantly clearing them (for forcing content re-loads for example). There is three kinds of caches at least: - In-memory cache for resources (images, stylesheets etc) - On-disk cache for resources - Page cache for loaded documents As mentioned in bug #21239, the on-disk cache is planned to be handled through Soup for the GTK+ port so it needs no additional API (although it does mean that you need to clear it separately then...). The in-memory cache size is a bit tricky to expose and it probably makes more sense to be automatic or modified build-time for special cases anyway, so I don't have a patch for that. Setting the page cache size is also a questionable thing to expose directly, since applications can't have a good view to what is a good value really. What the mac port does is to have three caching models (small/medium/large) for different application profiles, and these are relative to the capacity of the machine. So for example for a document viewer (no frequent navigation) the page cache is disabled, for a document (eg. help) browser the size is at most 3 and smaller if there's under 1GB of memory and a web browser gets 5 if there's more than 2GB of memory. I think this would be a sane thing to do for the GTK+ port too.

Attachments
This patch adds a boolean page-cache property to WebSettings for enabling/disabling the use of page cache (6.10 KB, patch) 2009-02-18 04:19 PST, Kalle Vahlman	gustavo: review-	Details Formatted Diff Diff
This patch adds API to clear the page and resource caches (2.26 KB, patch) 2009-02-18 04:25 PST, Kalle Vahlman	no flags	Details Formatted Diff Diff
*This patch adds API to clear the page and resource caches and* actually builds** (2.67 KB, patch) 2009-02-18 05:01 PST, Kalle Vahlman	gustavo: review-	Details Formatted Diff Diff
Implement 3 cache models for gtk target (17.47 KB, patch) 2009-03-07 18:44 PST, Bobby Powers	xan.lopez: review-	Details Formatted Diff Diff
Patch upgraded and reviewed (7.36 KB, patch) 2009-12-04 05:29 PST, Alejandro G. Castro	gustavo: review-	Details Formatted Diff Diff
New proposal of the patch (7.28 KB, patch) 2009-12-04 08:00 PST, Alejandro G. Castro	gustavo: review+ commit-queue: commit-queue-	Details Formatted Diff Diff
Proposed patch for the cache models (5.40 KB, patch) 2009-12-17 10:15 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Proposed patch (7.25 KB, patch) 2009-12-18 03:15 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Proposed patch for cache models (6.27 KB, patch) 2009-12-18 04:01 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Proposed patch cache models (6.33 KB, patch) 2009-12-19 01:51 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Cache models proposed patch (7.09 KB, patch) 2009-12-19 02:27 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Clearing cache API (2.90 KB, patch) 2009-12-19 03:21 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Proposed patch (6.36 KB, patch) 2009-12-20 01:42 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Cache models proposed patch (6.36 KB, patch) 2009-12-20 01:56 PST, Alejandro G. Castro	xan.lopez: review-	Details Formatted Diff Diff
Cache models proposed patch (6.23 KB, patch) 2009-12-20 07:37 PST, Alejandro G. Castro	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Kalle Vahlman

Comment 1 2009-02-18 04:19:24 PST

Created attachment 27745 [details] This patch adds a boolean page-cache property to WebSettings for enabling/disabling the use of page cache

Kalle Vahlman

Comment 2 2009-02-18 04:25:38 PST

Created attachment 27746 [details] This patch adds API to clear the page and resource caches

Kalle Vahlman

Comment 3 2009-02-18 05:01:55 PST

Created attachment 27747 [details] This patch adds API to clear the page and resource caches *and* actually builds

Bobby Powers

Comment 4 2009-03-07 18:44:54 PST

Created attachment 28394 [details] Implement 3 cache models for gtk target this adds a dependency to libgtop. I couldn't find another way to find total system memory in a platform-agnostic way. libgtop releases seem synced to GNOME, but its only linked against glib (doesn't pull in GNOME dependencies). If this is really a problem, I suppose I could go through and pull out the platform dependent ways for mac, windows, {free|open}bsd, linux and create an internal webkit function to find the total memory, but it seems like a big hassle. The cache model implementation is almost line for line the same as the Mac implementation, except on the mac they also manipulate NSURLCache. The GTK equivalent would be libsoup, but I'm told libsoup doesn't do any caching yet.

Gustavo Noronha (kov)

Comment 5 2009-05-04 04:50:57 PDT

Comment on attachment 27745 [details] This patch adds a boolean page-cache property to WebSettings for enabling/disabling the use of page cache I believe this property should be called enable-page-cache, to match the other properties which are similar in behavior. I think it is good to improve the comment which describes the property, too, to explain what the page cache is, and that disabling it doesn't disable all caching, but looks otherwise good to me. I'll have Xan take a look, since this is public API.

Gustavo Noronha (kov)

Comment 6 2009-05-04 05:32:27 PDT

Comment on attachment 27747 [details] This patch adds API to clear the page and resource caches *and* actually builds > +/** > + * webkit_web_view_clear_cache: > + * @web_view: a #WebKitWebView This looks wrong to me. Notice that you use the webview for nothing, and that's because both caches are actually global, right? So what we want is a webkit_clear_caches() that explains that it is cleaning the PageCache as well as the more generic cache, similar to webkit_get_default_session, that we already have. > + * > + * Clears cached resources. Useful for browsers with such functionality > + * and for forcing a reload of all resources. > + * > + * Since: 1.1.1 > + */ > +void webkit_web_view_clear_cache(WebKitWebView* webView) > +{ > + g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView)); > + > + // Clear the page cache > + int pageCacheSize = pageCache()->capacity(); > + pageCache()->setCapacity(0); > + pageCache()->setCapacity(pageCacheSize); > + > + // Clear the global cache > + cache()->setDisabled(true); > + cache()->setDisabled(false); > + > +} > + > } > diff --git a/WebKit/gtk/webkit/webkitwebview.h b/WebKit/gtk/webkit/webkitwebview.h > index 860c0d0..412326b 100644 > --- a/WebKit/gtk/webkit/webkitwebview.h > +++ b/WebKit/gtk/webkit/webkitwebview.h > @@ -2,6 +2,7 @@ > * Copyright (C) 2007 Holger Hans Peter Freyther > * Copyright (C) 2007, 2008 Alp Toker <alp@atoker.com> > * Copyright (C) 2008 Collabora Ltd. > + * Copyright (C) 2009 Movial Creative Technologies Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Library General Public > @@ -304,6 +305,9 @@ WEBKIT_API void > webkit_web_view_set_full_content_zoom (WebKitWebView *web_view, > gboolean full_content_zoom); > > +WEBKIT_API void > +webkit_web_view_clear_cache (WebKitWebView *web_view); > + > G_END_DECLS > > #endif

Xan Lopez

Comment 7 2009-05-04 05:35:20 PDT

Comment on attachment 28394 [details] Implement 3 cache models for gtk target > diff --git a/ChangeLog b/ChangeLog > index 1af6e4a..b4ef3b0 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,16 @@ > +2009-03-07 Bobby Powers <bobby@laptop.org> > + > + Reviewed by NOBODY (OOPS!). > + > + Implement and expose cache models for GTK > + > + Based off the Mac implementation in WebView.mm. Expose this as > + a property in WebKitWebSettings, even though it is a global setting. > + Update GtkLauncher to use the 'web browser' cache model. I really don't like exposing global settings through WebKitWebSettings, as those should at least in theory only affect the views they are attached to. So my preference would be to have ad-hoc functions for this, I think. Gustavo? > libwebkit_1_0_la_CFLAGS = \ > $(libWebCore_la_CFLAGS) \ > - $(GNOMEKEYRING_CFLAGS) > + $(GNOMEKEYRING_CFLAGS) \ > + $(LIBGTOP_CFLAGS) A new library dependency for this seems a real pain. Do you think there might a be a chance to get this code into glib proper? If so, I'd favour copy&pasting it while that happens to later drop the code. > --- a/WebKit/gtk/webkit/webkitprivate.h > +++ b/WebKit/gtk/webkit/webkitprivate.h > @@ -196,6 +196,15 @@ extern "C" { > > WEBKIT_API void > webkit_web_settings_add_extra_plugin_directory (WebKitWebView *web_view, const gchar* directory); > + > + WEBKIT_API void > + webkit_web_view_set_cache_model (WebKitCacheModel cache_model); > + > + WEBKIT_API WebKitCacheModel > + webkit_web_view_cache_model (void); > + > + WEBKIT_API gboolean > + webkit_web_view_did_set_cache_model (void); So all three functions are not meant for public usage, only the property? > + /** > + * WebKitWebSettings:cache-model: > + * > + * Specifies a usage model for a WebView, which WebKit will use to > + * determine its caching behavior. > + * > + * Research indicates that users tend to browse within clusters of > + * documents that hold resources in common, and to revisit previously visited > + * documents. WebKit and the frameworks below it include built-in caches that take > + * advantage of these patterns, substantially improving document load speed in > + * browsing situations. The WebKit cache model controls the behaviors of all of > + * these caches, including NSURLCache and the various WebCore caches. Guess you want to drop the NSURLCache bit there, and mention libsoup in the future. > + * > + * Applications with a browsing interface can improve document load speed > + * substantially by specifying WebCacheModelDocumentBrowser. Applications without > + * a browsing interface can reduce memory usage substantially by specifying > + * WebCacheModelDocumentViewer. > + * > + * Since: 1.1.1 1.1.7 (same everywhere else) > + */ > + g_object_class_install_property(gobject_class, > + PROP_CACHE_MODEL, > + g_param_spec_int( > + "cache-model", > + "Cache Model", > + "The cache model for all WebViews to use.", > + 0, 2, 0, > + flags)); This property should be an enum, not an int, and should mark for translation the blurb and long description. > > +#include <glibtop/mem.h> > +#include "Cache.h" > +#include "PageCache.h" > + Please use the existing system to sort the headers in that file. > > + if (!webkit_web_view_did_set_cache_model()) > + { Brace is on the wrong line. > + default: > + ASSERT_NOT_REACHED(); g_assert_not_reached() > > +gboolean webkit_web_view_did_set_cache_model() > +{ > + return s_didSetCacheModel ? TRUE : FALSE; > +} Just return s_didSetCacheModel; ? > + g_object_set (G_OBJECT (settings), "cache-model", WEBKIT_CACHE_MODEL_WEB_BROWSER, NULL); g_object_set takes a gpointer. I'll r- this for now while we discuss the poins I have raised or others people might have. >

Xan Lopez

Comment 8 2009-05-04 05:39:57 PDT

(In reply to comment #5) > (From update of attachment 27745 [details] [review]) > I believe this property should be called enable-page-cache, to match the other > properties which are similar in behavior. I think it is good to improve the > comment which describes the property, too, to explain what the page cache is, > and that disabling it doesn't disable all caching, but looks otherwise good to > me. I'll have Xan take a look, since this is public API. > Yes, I think it would be nice to have some of the great explanation of the first comment in the property documentation. Other issues are the enable- bit asd Gustavo mentioned, changing 1.1.1 to 1.1.7 in the Since: tags and marking strings for translation.

Gustavo Noronha (kov)

Comment 9 2009-05-04 05:57:52 PDT

Comment on attachment 28394 [details] Implement 3 cache models for gtk target > +/* not having this produces a warning when including glibtop.h */ > +#define TIME_WITH_SYS_TIME 1 I would prefer having this right before the include, in this case, and undef'ed afterwards. > + // initialize glibtop so we can get memory and disk usage. > + glibtop_init(); I don't think we need to call this init here. We don't need all structs filled, anyway. I believe you can safely use the global server with the get function straight away, but do test please, because gtop's docs are way bad =(. Also, it looks like gtop doesn't work on Windows, so I think we'd need a fallback for this. > + WEBKIT_API void > + webkit_web_view_set_cache_model (WebKitCacheModel cache_model); > + > + WEBKIT_API WebKitCacheModel > + webkit_web_view_cache_model (void); These should not be webkit_web_view. They do not concern a specific webview, so make them webkit_{set,get}_cache_model. Notice the get, too. > + WEBKIT_API gboolean > + webkit_web_view_did_set_cache_model (void); I don't think we need this at all. > + PROP_CACHE_MODEL Nor a property, since this will be handled globally. > + if (!webkit_web_view_did_set_cache_model()) > + { > + // the Mac port checks the running app against a list of known > + // apps with preferred cache models, and then falls back on > + // WEBWEBKIT_CACHE_MODEL_DOCUMENT_VIEWER > + webkit_web_view_set_cache_model(WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER); > + } > + Move this to webkit_init. We should only do this once, really. > + if (s_didSetCacheModel && cacheModel == s_cacheModel) > + return; The didSetCacheModel check should go away everywhere, I think. > diff --git a/WebKitTools/GtkLauncher/main.c b/WebKitTools/GtkLauncher/main.c Put this in a different patch, for the testing purposes, please. Thanks for the patch!

Christian Dywan

Comment 10 2009-06-16 13:04:16 PDT

The patch looks quite nice, mind Gustavo's comments. Did you check if libgtop 2.22.3 is available on Debian stable and BSD? I'd say if this is going to be mandatory that should both be Yes. And please check if gtop is available on Win32, WebKitGTK almost works out of the box on Win32 these days and we don't want to change that again.

Emilio Pozuelo Monfort

Comment 11 2009-06-16 13:09:13 PDT

Debian stable has libgtop 2.22.3

Philippe Normand

Comment 12 2009-10-14 08:06:40 PDT

Ping? Can the media test media/restore-from-page-cache.html be skipped until this bug is fixed?

Xan Lopez

Comment 13 2009-10-20 08:32:48 PDT

(In reply to comment #12) > Ping? > > Can the media test media/restore-from-page-cache.html be skipped until this bug > is fixed? I've done this now.

Alejandro G. Castro

Comment 14 2009-12-04 05:29:55 PST

Created attachment 44307 [details] Patch upgraded and reviewed I've upgraded and reviewed the patch following the comments: reviewed the name of the property, add documentation, add i18n and I've added also some parts that were missing: check modification notifications of the property and include the property when copying the object.

WebKit Review Bot

Comment 15 2009-12-04 05:32:12 PST

Attachment 44307 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebsettings.cpp:103: enable_page_cache is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:2549: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 2

Gustavo Noronha (kov)

Comment 16 2009-12-04 06:23:01 PST

Comment on attachment 44307 [details] Patch upgraded and reviewed > @@ -2543,6 +2546,8 @@ static void webkit_web_view_settings_notify(WebKitWebSettings* webSettings, GPar > if (page) > page->setTabKeyCyclesThroughElements(g_value_get_boolean(&value)); > } > + else if (name == g_intern_string("enable-page-cache")) > + settings->setUsesPageCache(g_value_get_boolean(&value)); > else if (!g_object_class_find_property(G_OBJECT_GET_CLASS(webSettings), name)) > g_warning("Unexpected setting '%s'", name); > g_value_unset(&value); Like the bot already told us, this else if should be on the line with the }. The variable name is fine, though, leave it as is. Aside from this, the patch looks good, so I would say just fix this and reupload so we can set cq+.

Xan Lopez

Comment 17 2009-12-04 06:55:01 PST

(In reply to comment #16) > (From update of attachment 44307 [details]) > > @@ -2543,6 +2546,8 @@ static void webkit_web_view_settings_notify(WebKitWebSettings* webSettings, GPar > > if (page) > > page->setTabKeyCyclesThroughElements(g_value_get_boolean(&value)); > > } > > + else if (name == g_intern_string("enable-page-cache")) > > + settings->setUsesPageCache(g_value_get_boolean(&value)); > > else if (!g_object_class_find_property(G_OBJECT_GET_CLASS(webSettings), name)) > > g_warning("Unexpected setting '%s'", name); > > g_value_unset(&value); > > Like the bot already told us, this else if should be on the line with the }. > The variable name is fine, though, leave it as is. Aside from this, the patch > looks good, so I would say just fix this and reupload so we can set cq+. Actually there's no reason to not use camel case in those variables (and I'm sure we do that in other places). But it can go in a different patch. + * viewers. This setting only controls the Page Cache, note that + * the Page Cache is an end user feature that makes navigating the + * web much smoother, it is not a cache in the disk cache sense and + * it is not a cache in the traditional memory cache. Basically the + * Page Cache makes clicking the back button almost + * instantaneous. For details about the different types of caches + * and their purposes see: + * http://webkit.org/blog/427/webkit-page-cache-i-the-basics/ I think here you are making things a bit confusing by not quoting the page completely. The page cache is a memory cache, but what the blog means is that it's not a "memory cache" in the traditional sense where a resource is stored in memory to be used by any page. I think I'd only say that this cache is different than the disk-based or memory-based traditional resource caches, that its point is to make going back and forth between pages much faster, and that for more details you can see the mentioned link. Ideally in the future we'd have a full explanation somewhere in our docs instead of in an external link.

Christian Dywan

Comment 18 2009-12-04 07:07:42 PST

Just in case, please keep in mind that the "upgraded" patch is only part of the feature. We still need a way to control the capacities of the caches.

Alejandro G. Castro

Comment 19 2009-12-04 08:00:12 PST

Created attachment 44313 [details] New proposal of the patch Thanks for the comments, I've modified all the points.

WebKit Review Bot

Comment 20 2009-12-04 08:05:04 PST

Attachment 44313 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebsettings.cpp:103: enable_page_cache is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1

Alejandro G. Castro

Comment 21 2009-12-04 10:36:32 PST

I've checked the second patch and we have to make some decisions about it before creating the next proposal, the method is called webkit_web_view_clear_cache but it is not clearing the application storage, should we change the name to clear_memory_cache or add cacheStorage.empty(). I've also checked win port when clearing the WebCache is also clearing the cross origin preflight cache, I guess this causes the preflight for all the cross origin requests again, this is not a resources cache in memory but the list of the states regarding those requests. Does anyone have any insight about this point, should we add it?

Christian Dywan

Comment 22 2009-12-04 10:52:38 PST

(In reply to comment #21) > I've checked the second patch and we have to make some decisions about it > before creating the next proposal, the method is called > webkit_web_view_clear_cache but it is not clearing the application storage, > should we change the name to clear_memory_cache or add cacheStorage.empty(). > > I've also checked win port when clearing the WebCache is also clearing the > cross origin preflight cache, I guess this causes the preflight for all the > cross origin requests again, this is not a resources cache in memory but the > list of the states regarding those requests. Does anyone have any insight about > this point, should we add it? How about passing a WebKitCacheType to indicate which cache to clear? Then we can also extend it in the future, ie. we can start with memory and page cache.

Alejandro G. Castro

Comment 23 2009-12-17 04:25:43 PST

(In reply to comment #22) > > [...] > > How about passing a WebKitCacheType to indicate which cache to clear? Then we > can also extend it in the future, ie. we can start with memory and page cache. After checking the different implementations in the other ports I think we can do the same mac is doing, which is basically review the cache model patch and use that API to set the documentviewer cache model in order to clean the cache. And add application cache and preflight cache clearing to that.

Alejandro G. Castro

Comment 24 2009-12-17 10:15:37 PST

Created attachment 45083 [details] Proposed patch for the cache models First proposal for the cache models API, based in the last patch uploaded in the bug. I've created just one API: void webkit_set_cache_model(WebKitCacheModel cacheModel), and avoided the libgtop setting static values.

WebKit Review Bot

Comment 25 2009-12-17 10:19:38 PST

Attachment 45083 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitprivate.cpp:248: Extra space before ( in function call [whitespace/parens] [4] WebKit/gtk/webkit/webkitwebview.cpp:4089: webkit_set_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.h:379: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3

WebKit Commit Bot

Comment 26 2009-12-17 10:32:37 PST

Comment on attachment 44313 [details] New proposal of the patch Rejecting patch 44313 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Gustavo Noronha Silva', '--force']" exit_code: 1 Last 500 characters of output: D at 142. Hunk #4 succeeded at 736 with fuzz 2 (offset 46 lines). Hunk #5 succeeded at 964 with fuzz 2 (offset 52 lines). Hunk #6 FAILED at 1084. Hunk #7 FAILED at 1152. 4 out of 7 hunks FAILED -- saving rejects to file WebKit/gtk/webkit/webkitwebsettings.cpp.rej patching file WebKit/gtk/webkit/webkitwebview.cpp Hunk #2 FAILED at 2389. Hunk #3 FAILED at 2420. Hunk #4 FAILED at 2448. Hunk #5 FAILED at 2545. 4 out of 5 hunks FAILED -- saving rejects to file WebKit/gtk/webkit/webkitwebview.cpp.rej Full output: http://webkit-commit-queue.appspot.com/results/127969

Alejandro G. Castro

Comment 27 2009-12-18 03:15:10 PST

Created attachment 45137 [details] Proposed patch Updated patch, let's try it again.

WebKit Review Bot

Comment 28 2009-12-18 03:19:28 PST

Attachment 45137 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebsettings.cpp:105: enable_page_cache is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1

Gustavo Noronha (kov)

Comment 29 2009-12-18 03:50:35 PST

Comment on attachment 45137 [details] Proposed patch ok!

WebKit Commit Bot

Comment 30 2009-12-18 04:00:37 PST

Comment on attachment 45137 [details] Proposed patch Clearing flags on attachment: 45137 Committed r52306: <http://trac.webkit.org/changeset/52306>

WebKit Commit Bot

Comment 31 2009-12-18 04:00:43 PST

All reviewed patches have been landed. Closing bug.

Alejandro G. Castro

Comment 32 2009-12-18 04:01:29 PST

Created attachment 45142 [details] Proposed patch for cache models Added a variable to store the current cache model and API to get it, we are going to use it to clear the caches.

Christian Dywan

Comment 33 2009-12-18 04:44:06 PST

Re-opening since not all patches are done.

Christian Dywan

Comment 34 2009-12-18 05:24:48 PST

(In reply to comment #32) > Created an attachment (id=45142) [details] > Proposed patch for cache models > > Added a variable to store the current cache model and API to get it, we are > going to use it to clear the caches. Did you intentionally make "web browser" the default or is the documentation wrong by saying that you should specifiy it if you are writing a web browser? I would think "none" should be the default with non-optimized values just like it is at the moment?

Alejandro G. Castro

Comment 35 2009-12-18 05:59:27 PST

(In reply to comment #34) > > [...] > > Did you intentionally make "web browser" the default or is the documentation > wrong by saying that you should specifiy it if you are writing a web browser? The intention was explaining each option, as: if you you are writing a web browser you can do it specifying it. > I would think "none" should be the default with non-optimized values just like > it is at the moment? Currently we have page cache capacity to 3, and defaults in the resources cache values, so none is not exactly what we have now. Those default values are smaller than the smallest values set in Apple policies,that is why I thought we could increase them, it is true they are magic values agreed with kov and xan.

Alejandro G. Castro

Comment 36 2009-12-19 01:51:51 PST

Created attachment 45229 [details] Proposed patch cache models I've removed the none value and reviewed the documentation, thanks for the comments.

Alejandro G. Castro

Comment 37 2009-12-19 02:27:35 PST

Created attachment 45230 [details] Cache models proposed patch

Alejandro G. Castro

Comment 38 2009-12-19 03:21:07 PST

Created attachment 45231 [details] Clearing cache API

Alejandro G. Castro

Comment 39 2009-12-20 01:42:06 PST

Created attachment 45261 [details] Proposed patch Small fix in the doc

WebKit Review Bot

Comment 40 2009-12-20 01:46:40 PST

Attachment 45261 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:4100: webkit_set_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4105: Should have a space between // and comment [whitespace/comments] [4] WebKit/gtk/webkit/webkitwebview.cpp:4148: webkit_get_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.h:379: Extra space before ( in function call [whitespace/parens] [4] WebKit/gtk/webkit/webkitwebview.h:382: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5

Alejandro G. Castro

Comment 41 2009-12-20 01:52:15 PST

Clearing cache patch needs some discussion before applying it, the use case to clean the whole cache (disk and memory) is not clear. We will have the soup API to release the disk cache, so in which situation we would like to release memory and disk? Considering the application has picked the proper cache model.

Alejandro G. Castro

Comment 42 2009-12-20 01:56:15 PST

Created attachment 45262 [details] Cache models proposed patch

WebKit Review Bot

Comment 43 2009-12-20 01:57:10 PST

Attachment 45262 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:4100: webkit_set_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4148: webkit_get_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.h:379: Extra space before ( in function call [whitespace/parens] [4] WebKit/gtk/webkit/webkitwebview.h:382: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4

Xan Lopez

Comment 44 2009-12-20 02:41:39 PST

Comment on attachment 45262 [details] Cache models proposed patch >diff --git a/WebKit/gtk/ChangeLog b/WebKit/gtk/ChangeLog >index 1bc9b80..c4b48ff 100644 >--- a/WebKit/gtk/ChangeLog >+++ b/WebKit/gtk/ChangeLog >@@ -1,3 +1,21 @@ >+2009-12-18 Alejandro G. Castro <alex@igalia.com> >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ [GTK] Cache control APIs >+ https://bugs.webkit.org/show_bug.cgi?id=24001 >+ >+ Original patch by Bobby Powers <bobby@laptop.org> >+ >+ Added new API to specify cache models for GTK port. >+ >+ * webkit/webkitprivate.cpp: >+ (webkit_init): set a default cache model. >+ * webkit/webkitwebview.cpp: >+ * webkit/webkitwebview.h: >+ (webkit_set_cache_model): Added function. >+ (webkit_get_cache_model): Added function. >+ > 2009-12-18 Xan Lopez <xlopez@igalia.com> > > Reviewed by Gustavo Noronha. >diff --git a/WebKit/gtk/webkit/webkitprivate.cpp b/WebKit/gtk/webkit/webkitprivate.cpp >index 7b613ba..bf0b109 100644 >--- a/WebKit/gtk/webkit/webkitprivate.cpp >+++ b/WebKit/gtk/webkit/webkitprivate.cpp >@@ -246,10 +246,7 @@ void webkit_init() > JSC::initializeThreading(); > WebCore::InitializeLoggingChannelsIfNecessary(); > >- // Page cache capacity (in pages). Comment from Mac port: >- // (Research indicates that value / page drops substantially after 3 pages.) >- // FIXME: Expose this with an API and/or calculate based on available resources >- WebCore::pageCache()->setCapacity(3); >+ webkit_set_cache_model(WEBKIT_CACHE_MODEL_WEB_BROWSER); > > #if ENABLE(DATABASE) > gchar* databaseDirectory = g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL); >diff --git a/WebKit/gtk/webkit/webkitwebview.cpp b/WebKit/gtk/webkit/webkitwebview.cpp >index b87eeeb..a63b8cc 100644 >--- a/WebKit/gtk/webkit/webkitwebview.cpp >+++ b/WebKit/gtk/webkit/webkitwebview.cpp >@@ -9,6 +9,7 @@ > * Copyright (C) 2008, 2009 Collabora Ltd. > * Copyright (C) 2009 Igalia S.L. > * Copyright (C) 2009 Movial Creative Technologies Inc. >+ * Copyright (C) 2009 Bobby Powers > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public >@@ -41,6 +42,7 @@ > #include "AXObjectCache.h" > #include "NotImplemented.h" > #include "BackForwardList.h" >+#include "Cache.h" > #include "CString.h" > #include "ChromeClientGtk.h" > #include "ContextMenu.h" >@@ -65,6 +67,7 @@ > #include "FrameLoader.h" > #include "FrameView.h" > #include "MouseEventWithHitTestResults.h" >+#include "PageCache.h" > #include "Pasteboard.h" > #include "PasteboardHelper.h" > #include "PasteboardHelperGtk.h" >@@ -113,6 +116,7 @@ > */ > > static const double defaultDPI = 96.0; >+static WebKitCacheModel cacheModel; > > using namespace WebKit; > using namespace WebCore; >@@ -4067,3 +4071,81 @@ G_CONST_RETURN gchar* webkit_web_view_get_icon_uri(WebKitWebView* webView) > priv->iconURI = g_strdup(iconURL.utf8().data()); > return priv->iconURI; > } >+ >+/** >+ * webkit_web_view_set_cache_model: Wrong name. >+ * @cache_model: a #WebKitCacheModel >+ * >+ * Specifies a usage model for a WebView, which WebKit will use to for WebViews? >+ * determine its caching behavior. All web views follow the cache >+ * model. This cache model determines the RAM and disk space to use >+ * for caching previously viewed content . >+ * >+ * Research indicates that users tend to browse within clusters of I'm wonder if we'll ever see this research/study we all quote ;) >+ * documents that hold resources in common, and to revisit previously >+ * visited documents. WebKit and the frameworks below it include >+ * built-in caches that take advantage of these patterns, >+ * substantially improving document load speed in browsing >+ * situations. The WebKit cache model controls the behaviors of all of >+ * these caches, including disk and the various WebCore caches. Maybe we shouldn't mention the disk caches until we support them? >+ * >+ * Browsers can improve document load speed substantially by >+ * specifying WEBKIT_CACHE_MODEL_WEB_BROWSER. Applications without a >+ * browsing interface can reduce memory usage substantially by >+ * specifying WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER. Default value is >+ * WEBKIT_CACHE_MODEL_WEB_BROWSER. >+ * >+ * Since: 1.1.18 >+ */ >+void webkit_set_cache_model(WebKitCacheModel model) >+{ >+ if (cacheModel == model) >+ return; >+ >+ // FIXME: Add disk cache handling when soup has the API >+ guint cacheTotalCapacity; >+ guint cacheMinDeadCapacity; >+ guint cacheMaxDeadCapacity; >+ gdouble deadDecodedDataDeletionInterval; >+ guint pageCacheCapacity; >+ >+ switch (model) { >+ case WEBKIT_CACHE_MODEL_DOCUMENT_VIEWER: >+ pageCacheCapacity = 0; >+ cacheTotalCapacity = 0; >+ cacheMinDeadCapacity = 0; >+ cacheMaxDeadCapacity = 0; >+ deadDecodedDataDeletionInterval = 0; >+ break; >+ case WEBKIT_CACHE_MODEL_WEB_BROWSER: >+ pageCacheCapacity = 3; >+ cacheTotalCapacity = 32 * 1024 * 1024; >+ cacheMinDeadCapacity = cacheTotalCapacity / 4; >+ cacheMaxDeadCapacity = cacheTotalCapacity / 2; >+ deadDecodedDataDeletionInterval = 60; >+ break; >+ default: >+ g_assert_not_reached(); Use g_return_if_reached, otherwise we'll always crash at runtime which is bad. >+ } >+ >+ cache()->setCapacities(cacheMinDeadCapacity, cacheMaxDeadCapacity, cacheTotalCapacity); >+ cache()->setDeadDecodedDataDeletionInterval(deadDecodedDataDeletionInterval); >+ pageCache()->setCapacity(pageCacheCapacity); >+ cacheModel = model; >+} >+ >+/** >+ * webkit_web_view_get_cache_model: Wrong name.

Alejandro G. Castro

Comment 45 2009-12-20 07:37:58 PST

Created attachment 45279 [details] Cache models proposed patch Reviewed the points.

WebKit Review Bot

Comment 46 2009-12-20 07:41:17 PST

Attachment 45279 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKit/gtk/webkit/webkitwebview.cpp:4100: webkit_set_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.cpp:4148: webkit_get_cache_model is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebKit/gtk/webkit/webkitwebview.h:379: Extra space before ( in function call [whitespace/parens] [4] WebKit/gtk/webkit/webkitwebview.h:382: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4

Xan Lopez

Comment 47 2009-12-20 07:42:44 PST

Comment on attachment 45279 [details] Cache models proposed patch r=me

WebKit Commit Bot

Comment 48 2009-12-20 07:53:46 PST

Comment on attachment 45279 [details] Cache models proposed patch Clearing flags on attachment: 45279 Committed r52418: <http://trac.webkit.org/changeset/52418>

WebKit Commit Bot

Comment 49 2009-12-20 07:53:56 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.