[webkit-reviews] review denied: [Bug 61801] Add Glade catalog for WebKitGTK widgets : [Attachment 95541] Glade catalog/module implementation

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Juan Pablo Ugarte
<juanpablougarte at gmail.com>'s request for review:
Bug 61801: Add Glade catalog for WebKitGTK widgets
https://bugs.webkit.org/show_bug.cgi?id=61801

Attachment 95541: Glade catalog/module implementation
https://bugs.webkit.org/attachment.cgi?id=95541&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95541&action=review

Nice stuff. I have posted some comments below.

> configure.ac:1132
> +# WebkitGTK glade catalog

You can remove this comment.

> Source/WebKit/gtk/glade/GNUmakefile.am:5
> +
> +## Process this file with automake to produce Makefile.in
> +
> +# libgladewebkit
> +

You can remove these comments.

> Source/WebKit/gtk/glade/gladewebkit.c:3
> +/*
> + * Copyright (C) 2011 Juan Pablo Ugarte.
> + *

I'd prefer this to be a C++ file.

> Source/WebKit/gtk/glade/gladewebkit.c:32
> +void gladeWebkitWebViewSetProperty(GladeWidgetAdaptor *adaptor,
> +				      GObject *object,
> +				      const char *id,
> +				      const GValue *value)

In WebKit we generally put these all on one line.

> Source/WebKit/gtk/glade/gladewebkit.c:34
> +    if (!strcmp(id, "glade-url")) {

I think g_str_equal would be better here.

> Source/WebKit/gtk/glade/gladewebkit.c:45
> +	   char *scheme = g_uri_parse_scheme(url);
> +
> +	   char *fullURL = (scheme) ? (char*) url : g_strconcat("http://", url,
NULL);
> +
> +	   webkit_web_view_load_uri(WEBKIT_WEB_VIEW(object), fullURL);
> +

I don't think the newlines are necessary in this block. You should use GOwnPtr
for scheme.

I think that this would be less confusing written like:
GOwnPtr<gchar> scheme(g_uri_parse_scheme(url));
if (scheme) {
    webkit_web_view_load_uri(WEBKIT_WEB_VIEW(object), fullURL);
    return;
}

GOwnPtr<gchar> fullURL(g_strconcat("http://", url, NULL));
webkit_web_view_load_uri(WEBKIT_WEB_VIEW(object), fullURL.get());

> Source/WebKit/gtk/glade/gladewebkit.c:47
> +    } else if (!strcmp(id, "settings")) {

I'd use g_str_equal here too.

> Source/WebKit/gtk/glade/gladewebkit.c:51
> +	   GObject *settings = g_value_get_object(value);
> +
> +	   if (!settings)
> +	       return;

Extra newline here.

> Source/WebKit/gtk/glade/gladewebkit.c:54
> +	   webkit_web_view_set_settings(WEBKIT_WEB_VIEW(object),
> +					WEBKIT_WEB_SETTINGS(settings));

This can be one line. We usually don't break lines until they go over 120
characters.

> Source/WebKit/gtk/glade/icons/GNUmakefile.am:19
> +
> +## Process this file with automake to produce Makefile.in
> +
> +## Icons
> +icons16dir = $(GLADE_PIXMAP_DIR)/hicolor/16x16/actions
> +
> +icons16_DATA = \
> +	   Source/WebKit/gtk/glade/icons/16x16/widget-webkit-webview.png \
> +	   Source/WebKit/gtk/glade/icons/16x16/widget-webkit-websettings.png
> +
> +icons22dir = $(GLADE_PIXMAP_DIR)/hicolor/22x22/actions
> +
> +icons22_DATA = \
> +	   Source/WebKit/gtk/glade/icons/22x22/widget-webkit-webview.png \
> +	   Source/WebKit/gtk/glade/icons/22x22/widget-webkit-websettings.png
> +
> +EXTRA_DIST += $(icons16_DATA) \
> +		 $(icons22_DATA)
> +

You should combine this with the glade GNUMakefile.am.