Bug 115867 – [GTK] [WebKit2] Use a template file for generated GObject enum files

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

Using a template file greatly simplifies the build rules for generating the enum files and makes it a lot easier to integrate these steps into the cmake build.

[Martin Robinson]

Created attachment 201383 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Comment on attachment 201383 [details]
Patch

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

This is great, a lot cleaner, but I think we can take advantage to improve the code.

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:22
> +#include <glib-object.h>

This is already included by the header.

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:34
> +static const G@Type@Value _@enum_name@_values[] = {

This is not actually a global variable but specific to the given get_type function, so we can move it inside the get_type(), and then you don't need the _@enum_types_ prefix, and can simply use values[].

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:49
> +    if (!type)

Her we could use G_UNLIKELY macro.

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:38
> +/*** BEGIN file-tail ***/
> +G_END_DECLS
> +
> +#endif
> +/*** END file-tail ***/

The tail goes at the end, file-production should go here. It's easier to read the template if it has the same order than the generated file.

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:43
> +/*** BEGIN file-production ***/
> +#include <webkit2/@basename@>
> +#
> +/*** END file-production ***/

I'm not sure we really need this, now that we have single header includes, when this file is included all others are already included too. Here you could add a comment like you are doing in the cpp file:

/* Enumerations from @filename@. */

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:46
> +#define WEBKIT_TYPE_@ENUMSHORT@ @enum_name@_get_type()

There should be a space between @enum_name@_get_type and ().

> Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:51
> +/*** END enumeration-production ***/

Here is where the tail goes.

[Martin Robinson]

Created attachment 201496 [details]
Patch

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 201383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201383&action=review
> 
> This is great, a lot cleaner, but I think we can take advantage to improve the code.

Sure. I was focused on the cmake build, but I don't mind circling back here to fix up the file.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:22
> > +#include <glib-object.h>
> 
> This is already included by the header.

Okay.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:34
> > +static const G@Type@Value _@enum_name@_values[] = {
> 
> This is not actually a global variable but specific to the given get_type function, so we can move it inside the get_type(), and then you don't need the _@enum_types_ prefix, and can simply use values[].

Okay.

> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.cpp.template:49
> > +    if (!type)
> 
> Her we could use G_UNLIKELY macro.

Makes sense.

> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:38
> > +/*** BEGIN file-tail ***/
> > +G_END_DECLS
> > +
> > +#endif
> > +/*** END file-tail ***/
> 
> The tail goes at the end, file-production should go here. It's easier to read the template if it has the same order than the generated file.

Okay. Fine by me.

> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:43
> > +/*** BEGIN file-production ***/
> > +#include <webkit2/@basename@>
> > +#
> > +/*** END file-production ***/
> 
> I'm not sure we really need this, now that we have single header includes, when this file is included all others are already included too. Here you could add a comment like you are doing in the cpp file:

Removing this necessitated the addition of webkit2/webkit2.h to the include list of WebKitEnumTypes.h. I could have experimented with including each header in the body of the file, but the global include seemed a lot cleaner.

> 
> /* Enumerations from @filename@. */
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:46
> > +#define WEBKIT_TYPE_@ENUMSHORT@ @enum_name@_get_type()
> 
> There should be a space between @enum_name@_get_type and ().

Okay.

> > Source/WebKit2/UIProcess/API/gtk/WebKitEnumTypes.h.template:51
> > +/*** END enumeration-production ***/
> 
> Here is where the tail goes.

Doesn't bother me to have it in order. :)

[Carlos Garcia Campos]

Comment on attachment 201496 [details]
Patch

Excellent!

[Martin Robinson]

Committed r150018: <http://trac.webkit.org/changeset/150018>