Bug 46630 - [GTK] use ENABLE(GLIB_SUPPORT)
Summary: [GTK] use ENABLE(GLIB_SUPPORT)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-27 09:36 PDT by Philippe Normand
Modified: 2010-09-27 12:04 PDT (History)
4 users (show)

See Also:


Attachments
proposed patch (4.52 KB, patch)
2010-09-27 10:12 PDT, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
updated patch (2.21 KB, patch)
2010-09-27 10:32 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff
updated patch (2.29 KB, patch)
2010-09-27 10:37 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
updated patch (2.29 KB, patch)
2010-09-27 10:38 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2010-09-27 09:36:40 PDT
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.
Comment 1 Philippe Normand 2010-09-27 10:12:44 PDT
Created attachment 68923 [details]
proposed patch
Comment 2 Martin Robinson 2010-09-27 10:26:54 PDT
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.
Comment 3 Philippe Normand 2010-09-27 10:32:11 PDT
Created attachment 68925 [details]
updated patch
Comment 4 Martin Robinson 2010-09-27 10:37:15 PDT
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.
Comment 5 Philippe Normand 2010-09-27 10:37:50 PDT
Created attachment 68926 [details]
updated patch

:)
Comment 6 Philippe Normand 2010-09-27 10:38:18 PDT
Created attachment 68927 [details]
updated patch
Comment 7 Martin Robinson 2010-09-27 10:41:57 PDT
Comment on attachment 68927 [details]
updated patch

Thanks.
Comment 8 Philippe Normand 2010-09-27 10:55:55 PDT
Committed r68405: <http://trac.webkit.org/changeset/68405>
Comment 9 WebKit Review Bot 2010-09-27 12:04:14 PDT
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