Bug 46630 – [GTK] use ENABLE(GLIB_SUPPORT)

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

[Philippe Normand]

For GObject-related conditional includes we currently check PLATFORM(GTK) although GObject is used by other port(s) like EFL that have a ENABLE(GLIB_SUPPORT). Switching to that will simplify code for ports willing to use GObject in the future.

[Philippe Normand]

Created attachment 68923 [details]
proposed patch

[Martin Robinson]

Comment on attachment 68923 [details]
proposed patch

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

> ChangeLog:8
> +        [GTK] use ENABLE(GLIB_SUPPORT)
> +        https://bugs.webkit.org/show_bug.cgi?id=46630
> +
> +        * GNUmakefile.am: Enabled the GLIB_SUPPORT define.

We should have a bit more here about why this is a good thing. Something like "Enabling GLIB_SUPPORT on all ports that use GLib will simplify checks."

> JavaScriptCore/ChangeLog:9
> +        is explicitely enabled.

Small typo here explicitely versus explicitly.

> JavaScriptCore/wtf/Platform.h:1130
> -#if PLATFORM(GTK) || (PLATFORM(EFL) && ENABLE(GLIB_SUPPORT))
> +#if ENABLE(GLIB_SUPPORT)

I like this simplification.

> WebCore/platform/graphics/cairo/FontPlatformDataFreeType.cpp:114
> -#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#if ENABLE(GLIB_SUPPORT)
>      if (GdkScreen* screen = gdk_screen_get_default())
> -gdk_screen_get_font_options(screen);
> +        gdk_screen_get_font_options(screen);

This doesn't seem right to me. The macro doesn't only implies GLib, not GDK. :/ Perhaps it would be better to define a series of PLATFORM macros like PLATFORM(GLIB) and PLATFORM(GDK) which are more precise about what's enabled.

> WebCore/platform/graphics/cairo/FontPlatformDataFreeType.cpp:149
> -#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#if ENABLE(GLIB_SUPPORT)
>      if (GdkScreen* screen = gdk_screen_get_default())
>          options = gdk_screen_get_font_options(screen);

Same issue here.

[Philippe Normand]

Created attachment 68925 [details]
updated patch

[Martin Robinson]

Comment on attachment 68925 [details]
updated patch

Great. As I mentioned to Philippe via Jabber, we're probably going to remove the GDK-specific stuff from FontPlatformDataFreeType soon, so we can avoid the issue of adding a GDK-specific flag for now. I think I'd still like to see this transformed into either USE(GLIB) or PLATFORM(GLIB) in a later patch.

[Philippe Normand]

Created attachment 68926 [details]
updated patch

:)

[Philippe Normand]

Created attachment 68927 [details]
updated patch

[Martin Robinson]

Comment on attachment 68927 [details]
updated patch

Thanks.

[Philippe Normand]

Committed r68405: <http://trac.webkit.org/changeset/68405>

[WebKit Review Bot]

http://trac.webkit.org/changeset/68405 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/68403
http://trac.webkit.org/changeset/68404
http://trac.webkit.org/changeset/68405
http://trac.webkit.org/changeset/68406