Bug 67990 – [GTK] Use WebKitWebContext in WebKitWebView

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Instead of using WKContextGetSharedProcessContext(), it should use webkit_web_context_get_default() and add API to create a new view with a context and return the current context.

[Carlos Garcia Campos]

Created attachment 107151 [details]
Patch

[Nayan Kumar K]

Seems like you missed adding WebKitWebContextPrivate.h?

[Carlos Garcia Campos]

Created attachment 107986 [details]
Updated patch

Add missing WebKitWebContextPrivate.h as pointed out by Nayan.

[Martin Robinson]

Comment on attachment 107986 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:153
> +/* Private API */

it should be apparent because it's in WebKit coding style.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:154
> +WKContextRef webkitWebContextGetContext(WebKitWebContext* context)

Probably better to call it something like: webkitWebContextGetWKContextRef to eliminate ambiguity.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49
> +    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetContext(webView->priv->context), 0);

Might as well do:
webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), 
                               webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0);

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:113
> + * Creates a new #WebKitWebView with the default context.
> + * See also webkit_web_view_new_with_context.
> + *

Somewhere we need to explain what a web context represents. I think it's sufficient to link to #WebKitWebContext here and ensure the documentation there is good.

i.e. Creates a new #WebKitWebView with the default #WebKitWebContext.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:127
> + * @context: the #WebKitWebContext to be used by the #WebKitWebView
> + *
> + * Creates a new #WebKitWebView with the given context.
> + *
> + * Returns: The newly created #WebKitWebView widget

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:140
> + * Gets the web context of @web_view.

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-63
>  };
>  
>  WK_EXPORT GType
> -webkit_web_view_get_type   (void);

This is why I think we should stop lining up the argument lists here. It's standard in GLib headers, but this kind of churn hurts patch readability and makes "git blame" useless.

> Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:40
> +    WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> +    g_object_ref_sink(view);
> +    g_assert(webkit_web_view_get_context(view) == webkit_web_context_get_default());
> +    g_object_unref(view);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_thread_init(NULL);
> +    gtk_test_init(&argc, &argv, NULL);
> +
> +    g_test_bug_base("https://bugs.webkit.org/");
> +    g_test_add_func("/webkit2/webview/default_context",
> +                    testWebViewDefaultContext);
> +

I'm loving these unit tests! The asterisks are on the wrong side here though.

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 107986 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107986&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:153
> > +/* Private API */
> 
> it should be apparent because it's in WebKit coding style.

It means the API is public inside webkit, but private for the users. It's still obvious because they are not static methods, but I added just to organize the source file, I can remove it anyway.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:154
> > +WKContextRef webkitWebContextGetContext(WebKitWebContext* context)
> 
> Probably better to call it something like: webkitWebContextGetWKContextRef to eliminate ambiguity.

Makes sense.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:49
> > +    webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), webkitWebContextGetContext(webView->priv->context), 0);
> 
> Might as well do:
> webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), 
>                                webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0);

I'm not sure I understand what you mean here, you are using a WebView cast for a WebContext.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:113
> > + * Creates a new #WebKitWebView with the default context.
> > + * See also webkit_web_view_new_with_context.
> > + *
> 
> Somewhere we need to explain what a web context represents. 

Yes, that would be part of the section for WebKitWebContext

> I think it's sufficient to link to #WebKitWebContext here and ensure the documentation there is good.
> 
> i.e. Creates a new #WebKitWebView with the default #WebKitWebContext.

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:127
> > + * @context: the #WebKitWebContext to be used by the #WebKitWebView
> > + *
> > + * Creates a new #WebKitWebView with the given context.
> > + *
> > + * Returns: The newly created #WebKitWebView widget
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:140
> > + * Gets the web context of @web_view.
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:-63
> >  };
> >  
> >  WK_EXPORT GType
> > -webkit_web_view_get_type   (void);
> 
> This is why I think we should stop lining up the argument lists here. It's standard in GLib headers, but this kind of churn hurts patch readability and makes "git blame" useless.

I think source code readability is more important than patches readability, and git blame in a C header file is not that useful, but if everybody else agrees it's fine with me to stop lining up the arguments.

> > Source/WebKit2/UIProcess/API/gtk/tests/testwebview.c:40
> > +    WebKitWebView *view = WEBKIT_WEB_VIEW(webkit_web_view_new());
> > +    g_object_ref_sink(view);
> > +    g_assert(webkit_web_view_get_context(view) == webkit_web_context_get_default());
> > +    g_object_unref(view);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > +    g_thread_init(NULL);
> > +    gtk_test_init(&argc, &argv, NULL);
> > +
> > +    g_test_bug_base("https://bugs.webkit.org/");
> > +    g_test_add_func("/webkit2/webview/default_context",
> > +                    testWebViewDefaultContext);
> > +
> 
> I'm loving these unit tests! The asterisks are on the wrong side here though.

Unit tests are C files, not C++.

[Carlos Garcia Campos]

Created attachment 108115 [details]
Updated patch

Updated patch fixing some of the comments, and it also adds WebKitPrivate.h with GParamFlags macros, since this patch is the first one adding properties.

[Martin Robinson]

(In reply to comment #5)

> > Might as well do:
> > webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), 
> >                                webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView->priv->context)), 0);
> 
> I'm not sure I understand what you mean here, you are using a WebView cast for a WebContext.

Sorry. I mean this:

webkitWebViewBaseCreateWebPage(WEBKIT_WEB_VIEW_BASE(webView), 
> >                                webkitWebContextGetContext(WEBKIT_WEB_VIEW(webView)->priv->context), 

> > I'm loving these unit tests! The asterisks are on the wrong side here though.
> Unit tests are C files, not C++.

Sorry. I always forget about the silly difference in style for C files.

This patch looks good to me. I think it should be r+ with another reviewer looking at it and a few minor tweaks.

[Martin Robinson]

Comment on attachment 108115 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitPrivate.h:35
> +#define WEBKIT_PARAM_READABLE  (static_cast<GParamFlags>(G_PARAM_READABLE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB))
> +#define WEBKIT_PARAM_WRITABLE  (static_cast<GParamFlags>(G_PARAM_WRITABLE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB))
> +#define WEBKIT_PARAM_READWRITE (static_cast<GParamFlags>(G_PARAM_READWRITE|G_PARAM_STATIC_NAME|G_PARAM_STATIC_NICK|G_PARAM_STATIC_BLURB))

You have some extra spaces after the macro names.  I don't think we typically line these up in WebKit.

[Philippe Normand]

Comment on attachment 108115 [details]
Updated patch

Looks good to me, API wise!

[Martin Robinson]

Comment on attachment 108115 [details]
Updated patch

Philippe has approved this so r+ with the few tweaks I suggested.

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 108115 [details])
> Philippe has approved this so r+ with the few tweaks I suggested.

Thanks, not that this still depends on bug https://bugs.webkit.org/show_bug.cgi?id=67931, which depends on headers patch.

[Carlos Garcia Campos]

Committed r96136: <http://trac.webkit.org/changeset/96136>