[webkit-reviews] review denied: [Bug 67990] [GTK] Use WebKitWebContext in WebKitWebView : [Attachment 107986] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 67990: [GTK] Use WebKitWebContext in WebKitWebView
https://bugs.webkit.org/show_bug.cgi?id=67990

Attachment 107986: Updated patch
https://bugs.webkit.org/attachment.cgi?id=107986&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.