Bug 115867 - [GTK] [WebKit2] Use a template file for generated GObject enum files
Summary: [GTK] [WebKit2] Use a template file for generated GObject enum files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords:
Depends on:
Blocks: 115966
  Show dependency treegraph
 
Reported: 2013-05-09 11:45 PDT by Martin Robinson
Modified: 2013-05-13 10:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (7.69 KB, patch)
2013-05-10 13:02 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (8.41 KB, patch)
2013-05-12 10:37 PDT, Martin Robinson
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2013-05-09 11:45:47 PDT
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.
Comment 1 Martin Robinson 2013-05-10 13:02:43 PDT
Created attachment 201383 [details]
Patch
Comment 2 WebKit Commit Bot 2013-05-10 13:05:14 PDT
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
Comment 3 Carlos Garcia Campos 2013-05-12 01:46:27 PDT
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.
Comment 4 Martin Robinson 2013-05-12 10:37:18 PDT
Created attachment 201496 [details]
Patch
Comment 5 Martin Robinson 2013-05-12 10:40:22 PDT
(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. :)
Comment 6 Carlos Garcia Campos 2013-05-13 00:09:31 PDT
Comment on attachment 201496 [details]
Patch

Excellent!
Comment 7 Martin Robinson 2013-05-13 10:35:07 PDT
Committed r150018: <http://trac.webkit.org/changeset/150018>