Bug 67931 – [GTK] Add WebKitWebContext to GTK API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Add WebKitWebContext class to wrap WKContext

[Carlos Garcia Campos]

Created attachment 107042 [details]
Patch

This patch depends on bug #65178

[Martin Robinson]

Comment on attachment 107042 [details]
Patch

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

Looks good, but I have a few nits and a small request to fix some of the FIXMEs now that we are touching this code. I feel that if we don't fix them now, they'll never get fixed.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:96
> + * WEBKIT_CACHE_MODEL_WEB_BROWSER.

Default value -> The default value

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:134
> +WebKitCacheModel
> +webkit_web_context_get_cache_model(WebKitWebContext* context)

Extra newline here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:87
> +WK_EXPORT GType
> +webkit_web_context_get_type        (void);
> +
> +WK_EXPORT WebKitWebContext *
> +webkit_web_context_get_default     (void);
> +
> +WK_EXPORT void
> +webkit_web_context_set_cache_model (WebKitWebContext *context,
> +                                    WebKitCacheModel  cache_model);
> +WK_EXPORT WebKitCacheModel
> +webkit_web_context_get_cache_model (WebKitWebContext *context);
> +

Do you think it makes sense to stop lining the arguments up between functions? It's really annoyng when you finally add a longer function and have to realign the entire file.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:48
> +    // FIXME: Add disk cache handling when soup has the API
> +    unsigned long urlCacheMemoryCapacity = 0;
> +    unsigned long urlCacheDiskCapacity = 0;
> +    uint64_t diskFreeSize = 0;

Soup has the API now, but I'm not sure if it's public yet.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:52
> +    // FIXME: The Mac port calculates these values based on the amount of physical memory that's
> +    // installed on the system. Currently these values match the Mac port for users with more than
> +    // 512 MB and less than 1024 MB of physical memory.

Is it possible to do this now, at least for Linux?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 107042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107042&action=review
> 
> Looks good, but I have a few nits and a small request to fix some of the FIXMEs now that we are touching this code. I feel that if we don't fix them now, they'll never get fixed.

This bug is about the WebKit2 API, those FIXMEs are new features that could be impemented for both WebKit1 and WebKit2 and don't depend on the API. So we can file separate bug reports for them so that we don't forget it.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:96
> > + * WEBKIT_CACHE_MODEL_WEB_BROWSER.
> 
> Default value -> The default value

Done.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:134
> > +WebKitCacheModel
> > +webkit_web_context_get_cache_model(WebKitWebContext* context)
> 
> Extra newline here.

Done.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.h:87
> > +WK_EXPORT GType
> > +webkit_web_context_get_type        (void);
> > +
> > +WK_EXPORT WebKitWebContext *
> > +webkit_web_context_get_default     (void);
> > +
> > +WK_EXPORT void
> > +webkit_web_context_set_cache_model (WebKitWebContext *context,
> > +                                    WebKitCacheModel  cache_model);
> > +WK_EXPORT WebKitCacheModel
> > +webkit_web_context_get_cache_model (WebKitWebContext *context);
> > +
> 
> Do you think it makes sense to stop lining the arguments up between functions? It's really annoyng when you finally add a longer function and have to realign the entire file.

I like it, but I guess it's actually a matter of taste. 

> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:48
> > +    // FIXME: Add disk cache handling when soup has the API
> > +    unsigned long urlCacheMemoryCapacity = 0;
> > +    unsigned long urlCacheDiskCapacity = 0;
> > +    uint64_t diskFreeSize = 0;
> 
> Soup has the API now, but I'm not sure if it's public yet.

It's marked as LIBSOUP_USE_UNSTABLE_REQUEST_API, but we are currently used auth dialog feature too which is also considered unstable, I think.

> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:52
> > +    // FIXME: The Mac port calculates these values based on the amount of physical memory that's
> > +    // installed on the system. Currently these values match the Mac port for users with more than
> > +    // 512 MB and less than 1024 MB of physical memory.
> 
> Is it possible to do this now, at least for Linux?

I don't know, I'll look at it, but I'm more worried about the API in this moment.

[Martin Robinson]

(In reply to comment #3)

> > > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:52
> > > +    // FIXME: The Mac port calculates these values based on the amount of physical memory that's
> > > +    // installed on the system. Currently these values match the Mac port for users with more than
> > > +    // 512 MB and less than 1024 MB of physical memory.
> > 
> > Is it possible to do this now, at least for Linux?
> 
> I don't know, I'll look at it, but I'm more worried about the API in this moment.

I think properly implementing this function is an important part of exposing the API. It's a little bit broken as it is now.

[Carlos Garcia Campos]

Created attachment 107991 [details]
New patch

I've splitted the patch, because other patches depend on this one, and I don't want the cache model implementation blocks other API patches.

[Martin Robinson]

Comment on attachment 107991 [details]
New patch

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

Looks good to me. There's one small thing below and we just need another reviwer to review the API.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:58
> +    WebKitWebContext* webContext = WEBKIT_WEB_CONTEXT(g_object_new(WEBKIT_TYPE_WEB_CONTEXT, 0));

Sentinals have to be NULL,  because 0 is a different size than NULL on 64-bit.

[Carlos Garcia Campos]

Created attachment 108001 [details]
Use NULL instead of 0 in g_object_new()

[Martin Robinson]

Comment on attachment 108001 [details]
Use NULL instead of 0 in g_object_new()

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:74
> +    static GOnce onceInit = G_ONCE_INIT;
> +    return WEBKIT_WEB_CONTEXT(g_once(&onceInit, getDefaultWebContext, 0));

Out of curiosity does GOnce require that GObject be created in another function. I'm not sure I understand the benefit over something like this:

static WebKitWebContext* webContext = 0;
if (webContext)
    return webContext;

webContext = WEBKIT_WEB_CONTEXT(g_object_new(WEBKIT_TYPE_WEB_CONTEXT, NULL));
webContext->priv->context = WKContextGetSharedProcessContext();
WKContextSetCacheModel(webContext->priv->context, kWKCacheModelPrimaryWebBrowser);
return webContext;

It's fewer lines and clearer.

[Carlos Garcia Campos]

(In reply to comment #8)
> (From update of attachment 108001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108001&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:74
> > +    static GOnce onceInit = G_ONCE_INIT;
> > +    return WEBKIT_WEB_CONTEXT(g_once(&onceInit, getDefaultWebContext, 0));
> 
> Out of curiosity does GOnce require that GObject be created in another function.

Yes, GOnce calls the function the first time is called and returns the stored value for subsequent calls.

> I'm not sure I understand the benefit over something like this:

The only benefict is that GOnce ensures that the initialization is serialized across multiple threads.

> static WebKitWebContext* webContext = 0;
> if (webContext)
>     return webContext;
> 
> webContext = WEBKIT_WEB_CONTEXT(g_object_new(WEBKIT_TYPE_WEB_CONTEXT, NULL));
> webContext->priv->context = WKContextGetSharedProcessContext();
> WKContextSetCacheModel(webContext->priv->context, kWKCacheModelPrimaryWebBrowser);
> return webContext;
> 
> It's fewer lines and clearer.

[Martin Robinson]

Comment on attachment 108001 [details]
Use NULL instead of 0 in g_object_new()

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

Looks good to me with a minor tweak.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:56
> +static gpointer getDefaultWebContext(gpointer)

You should probably call this createDefaultWebContext.

[Philippe Normand]

Comment on attachment 108001 [details]
Use NULL instead of 0 in g_object_new()

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

Looks good to me API wise, just a nit in the test!

> Source/WebKit2/UIProcess/API/gtk/tests/testwebcontext.c:27
> +    g_assert(webkit_web_context_get_default(context) == webkit_web_context_get_default(context));

Hum webkit_web_context_get_default() doesn't take any argument, did I miss something?

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 108001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108001&action=review
> 
> Looks good to me API wise, just a nit in the test!
> 
> > Source/WebKit2/UIProcess/API/gtk/tests/testwebcontext.c:27
> > +    g_assert(webkit_web_context_get_default(context) == webkit_web_context_get_default(context));
> 
> Hum webkit_web_context_get_default() doesn't take any argument, did I miss something?

Nop, I changed that and it seems I forgot to add the changes before making the patch :-P

[Carlos Garcia Campos]

Created attachment 108283 [details]
Patch with suggested changes

[Philippe Normand]

Comment on attachment 108283 [details]
Patch with suggested changes

As Martin didn't object on the API and neither did I, this is good to go. Thanks Carlos!

[Carlos Garcia Campos]

Committed r96133: <http://trac.webkit.org/changeset/96133>