Bug 61801 – Add Glade catalog for WebKitGTK widgets

• Component: WebKitGTK
• Status: UNCONFIRMED

[Juan Pablo Ugarte]

Hello guys this is a basic implementation to add WebkitGTK widget support for glade and should be more that enough as a starting point.
It currently lets you embed a WebKitWebView and setup WebKitWebSettings object properties.

I also made a glade module that adds a URL property to WebView so that you can type in some address in glade to be able to see something.
In any case this new prop will never be saved to the builder file.

This is my first time I submit a patch for webkit and I had to touch the build system so that really needs review to make I made it the webkit way.
BTW we need icons for both classes 16x16 and 22x22 pixels

greets

Juan Pablo

Changelog:
	* configure.ac: added --enable-glade-catalog option

	* Source/WebKit/gtk/GNUmakefile.am: include Source/WebKit/gtk/GNUmakefile.am if --enable-glade-catalog was used

	* Source/WebKit/gtk/glade/GNUmakefile.am: rules to build libgladewebkit

	* Source/WebKit/gtk/glade/glade-webkit.c: glade module source.

	* Source/WebKit/gtk/glade/webkit.xml: Glade catalog (WebKitWebView and WebKitWebSettings support)

	* Source/WebKit/gtk/glade/icons/GNUmakefile.am: rules to install icons

[Juan Pablo Ugarte]

Created attachment 95470 [details]
first implementation

[Gustavo Noronha (kov)]

Comment on attachment 95470 [details]
first implementation

Hey, thanks for working on this! I don't know a lot about glade catalogs but I'll try to review this patch. Before I go into details though I'd like to ask you to review the style guide http://www.webkit.org/coding/coding-style.html. The names and declaration locations of variables, the position of the *, the sorting of #include statements, position of bracers are all wrong on the .c file (which, I think should be called webkitglade.c or gladewebkit.c to be consistent with the rest of the files under Sources/WebKit/gtk).

We also need a ChangeLog entry that you can generate with the Tools/Scripts/prepare-ChangeLog  script (use --help to see the relevant options).

[Juan Pablo Ugarte]

Created attachment 95541 [details]
Glade catalog/module implementation

[Martin Robinson]

Comment on attachment 95541 [details]
Glade catalog/module implementation

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.

[Juan Pablo Ugarte]

Created attachment 97120 [details]
Glade catalog/module implementation, 3rd try

Hi guys, this is a reviewed patch, The only thing i was not able to fix was to make the file a cpp since there is a bugs in glade headers that prevents us to include it in c++.
I will fix the headers and post a new patch that move the file to c++ once a new glade release is made.
Does it sounds like a plan?

[Martin Robinson]

(In reply to comment #5)
> Created an attachment (id=97120) [details]
> Glade catalog/module implementation, 3rd try
> 
> Hi guys, this is a reviewed patch, The only thing i was not able to fix was to make the file a cpp since there is a bugs in glade headers that prevents us to include it in c++.
> I will fix the headers and post a new patch that move the file to c++ once a new glade release is made.
> Does it sounds like a plan?

What keyword does the header use? We may be able to use the C preprocessor to get around this issue.

[Martin Robinson]

For instance, see this question on StackOverflow: http://stackoverflow.com/questions/2841204/use-the-keyword-class-as-a-variable-name-in-c

[Juan Pablo Ugarte]

One function has a parameter called virtual, but that can be fixed with the preprocessor.

the real problem is that we are defining an enum like this in glade-project.h

typedef enum   _GladePointerMode     GladePointerMode;

enum _GladePointerMode
{
  GLADE_POINTER_SELECT = 0,
  GLADE_POINTER_ADD_WIDGET,
  GLADE_POINTER_DRAG_RESIZE
};

so g++ complains about glade-project.h:22:16: error: use of enum ‘_GladePointerMode’ without previous declaration

[Eric Seidel (no email)]

Ping gtk reviewers?

[Martin Robinson]

(In reply to comment #8)
> One function has a parameter called virtual, but that can be fixed with the preprocessor.
> 
> the real problem is that we are defining an enum like this in glade-project.h
> 
> typedef enum   _GladePointerMode     GladePointerMode;
> 
> enum _GladePointerMode
> {
>   GLADE_POINTER_SELECT = 0,
>   GLADE_POINTER_ADD_WIDGET,
>   GLADE_POINTER_DRAG_RESIZE
> };

It seems you've fixed these issues upstream.  Thanks! Could you simply conditionalize Glade support on the new version?

[Julien Chaffraix]

Comment on attachment 97120 [details]
Glade catalog/module implementation, 3rd try

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

This patch has rotten pretty badly. If you choose to revive it, I would advise to ask the friendly people on #webkitgtk+ so that your patch doesn't get stuck again.

> Source/WebKit/gtk/ChangeLog:3
> +        Reviewed by Gustavo Noronha Silva.

That's weird as nobody reviewed it in the bug.