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
Created attachment 95470 [details] first implementation
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).
Created attachment 95541 [details] Glade catalog/module implementation
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.
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?
(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.
For instance, see this question on StackOverflow: http://stackoverflow.com/questions/2841204/use-the-keyword-class-as-a-variable-name-in-c
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
Ping gtk reviewers?
(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?
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.