[webkit-reviews] review denied: [Bug 46630] [GTK] use ENABLE(GLIB_SUPPORT) : [Attachment 68923] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 27 10:26:54 PDT 2010


Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 46630: [GTK] use ENABLE(GLIB_SUPPORT)
https://bugs.webkit.org/show_bug.cgi?id=46630

Attachment 68923: proposed patch
https://bugs.webkit.org/attachment.cgi?id=68923&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.


More information about the webkit-reviews mailing list