Bug 24001 - [GTK] Cache control APIs
: [GTK] Cache control APIs
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-18 04:15 PST by Kalle Vahlman
Modified: 2010-02-26 10:11 PST (History)
12 users (show)

See Also:


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
gns: 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
gns: 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
gns: review-
Details | Formatted Diff | Diff
New proposal of the patch (7.28 KB, patch)
2009-12-04 08:00 PST, Alejandro G. Castro
gns: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Kalle Vahlman 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.
Comment 1 Kalle Vahlman 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
Comment 2 Kalle Vahlman 2009-02-18 04:25:38 PST
Created attachment 27746 [details]
This patch adds API to clear the page and resource caches
Comment 3 Kalle Vahlman 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
Comment 4 Bobby Powers 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.
Comment 5 Gustavo Noronha (kov) 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.
Comment 6 Gustavo Noronha (kov) 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
Comment 7 Xan Lopez 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.

>
Comment 8 Xan Lopez 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.
Comment 9 Gustavo Noronha (kov) 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!
Comment 10 Christian Dywan 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.
Comment 11 Emilio Pozuelo Monfort 2009-06-16 13:09:13 PDT
Debian stable has libgtop 2.22.3
Comment 12 Philippe Normand 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?
Comment 13 Xan Lopez 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.
Comment 14 Alejandro G. Castro 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.
Comment 15 WebKit Review Bot 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
Comment 16 Gustavo Noronha (kov) 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+.
Comment 17 Xan Lopez 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.
Comment 18 Christian Dywan 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.
Comment 19 Alejandro G. Castro 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.
Comment 20 WebKit Review Bot 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
Comment 21 Alejandro G. Castro 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?
Comment 22 Christian Dywan 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.
Comment 23 Alejandro G. Castro 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.
Comment 24 Alejandro G. Castro 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.
Comment 25 WebKit Review Bot 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
Comment 26 WebKit Commit Bot 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
Comment 27 Alejandro G. Castro 2009-12-18 03:15:10 PST
Created attachment 45137 [details]
Proposed patch

Updated patch, let's try it again.
Comment 28 WebKit Review Bot 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
Comment 29 Gustavo Noronha (kov) 2009-12-18 03:50:35 PST
Comment on attachment 45137 [details]
Proposed patch

ok!
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2009-12-18 04:00:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Alejandro G. Castro 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.
Comment 33 Christian Dywan 2009-12-18 04:44:06 PST
Re-opening since not all patches are done.
Comment 34 Christian Dywan 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?
Comment 35 Alejandro G. Castro 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.
Comment 36 Alejandro G. Castro 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.
Comment 37 Alejandro G. Castro 2009-12-19 02:27:35 PST
Created attachment 45230 [details]
Cache models proposed patch
Comment 38 Alejandro G. Castro 2009-12-19 03:21:07 PST
Created attachment 45231 [details]
Clearing cache API
Comment 39 Alejandro G. Castro 2009-12-20 01:42:06 PST
Created attachment 45261 [details]
Proposed patch

Small fix in the doc
Comment 40 WebKit Review Bot 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
Comment 41 Alejandro G. Castro 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.
Comment 42 Alejandro G. Castro 2009-12-20 01:56:15 PST
Created attachment 45262 [details]
Cache models proposed patch
Comment 43 WebKit Review Bot 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
Comment 44 Xan Lopez 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.
Comment 45 Alejandro G. Castro 2009-12-20 07:37:58 PST
Created attachment 45279 [details]
Cache models proposed patch

Reviewed the points.
Comment 46 WebKit Review Bot 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
Comment 47 Xan Lopez 2009-12-20 07:42:44 PST
Comment on attachment 45279 [details]
Cache models proposed patch

r=me
Comment 48 WebKit Commit Bot 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>
Comment 49 WebKit Commit Bot 2009-12-20 07:53:56 PST
All reviewed patches have been landed.  Closing bug.