Bug 67931 - [GTK] Add WebKitWebContext to GTK API
Summary: [GTK] Add WebKitWebContext to GTK API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 65178
Blocks: 67990 68434
  Show dependency treegraph
 
Reported: 2011-09-12 05:45 PDT by Carlos Garcia Campos
Modified: 2011-09-27 11:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (17.22 KB, patch)
2011-09-12 05:50 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
New patch (10.73 KB, patch)
2011-09-20 06:09 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
Use NULL instead of 0 in g_object_new() (10.75 KB, patch)
2011-09-20 07:56 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch with suggested changes (10.78 KB, patch)
2011-09-21 23:52 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-09-12 05:45:56 PDT
Add WebKitWebContext class to wrap WKContext
Comment 1 Carlos Garcia Campos 2011-09-12 05:50:13 PDT
Created attachment 107042 [details]
Patch

This patch depends on bug #65178
Comment 2 Martin Robinson 2011-09-14 08:55:00 PDT
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?
Comment 3 Carlos Garcia Campos 2011-09-15 00:21:21 PDT
(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.
Comment 4 Martin Robinson 2011-09-15 00:29:29 PDT
(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.
Comment 5 Carlos Garcia Campos 2011-09-20 06:09:02 PDT
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.
Comment 6 Martin Robinson 2011-09-20 06:52:53 PDT
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.
Comment 7 Carlos Garcia Campos 2011-09-20 07:56:27 PDT
Created attachment 108001 [details]
Use NULL instead of 0 in g_object_new()
Comment 8 Martin Robinson 2011-09-20 15:15:07 PDT
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.
Comment 9 Carlos Garcia Campos 2011-09-20 23:49:38 PDT
(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.
Comment 10 Martin Robinson 2011-09-21 09:57:15 PDT
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.
Comment 11 Philippe Normand 2011-09-21 10:01:32 PDT
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?
Comment 12 Carlos Garcia Campos 2011-09-21 23:40:24 PDT
(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
Comment 13 Carlos Garcia Campos 2011-09-21 23:52:44 PDT
Created attachment 108283 [details]
Patch with suggested changes
Comment 14 Philippe Normand 2011-09-22 00:07:06 PDT
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!
Comment 15 Carlos Garcia Campos 2011-09-27 11:05:20 PDT
Committed r96133: <http://trac.webkit.org/changeset/96133>