[Webkit-unassigned] [Bug 24001] [GTK] Cache control APIs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=24001


xan.lopez at gmail.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #28394|review?                     |review-
               Flag|                            |




------- Comment #7 from xan.lopez at gmail.com  2009-05-04 05:35 PDT -------
(From update of attachment 28394)
> diff --git a/ChangeLog b/ChangeLog
> index 1af6e4a..b4ef3b0 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,16 @@
> +2009-03-07  Bobby Powers  <bobby at 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.

>


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.