[webkit-reviews] review denied: [Bug 50867] [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6 : [Attachment 77178] Proposed patch to make WebKitGtk+ build on Mac OS X

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Koan-Sin Tan
<koansin.tan at gmail.com>'s request for review:
Bug 50867: [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6
https://bugs.webkit.org/show_bug.cgi?id=50867

Attachment 77178: Proposed patch to make WebKitGtk+ build on Mac OS X
https://bugs.webkit.org/attachment.cgi?id=77178&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77178&action=review

> Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:38
> +#if OS(DARWIN) && PLATFORM(GTK)
> +#define Cursor	QD_Cursor 
> +#endif
>  #include "PluginObject.h"
> +#if OS(DARWIN) && PLATFORM(GTK)
> +#undef Cursor
> +#endif
>  #include "PluginTest.h"
>  
>  #include "npapi.h"

Where is Cursor referenced? It probably makes sense to do this closer to that
place.

> Tools/Scripts/webkitdirs.pm:600
> -	   if (-e $libraryDir . "libwebkitgtk-3.0.so") {
> +	   if (isDarwin() and -d "$libraryDir") {
> +	       return $libraryDir . "libwebkitgtk-1.0.dylib";
> +	   } elsif (-e $libraryDir . "libwebkitgtk-3.0.so") {
>	       return $libraryDir . "libwebkitgtk-3.0.so";

You don't properly handle the GTK+ 2.x case here.

> WebCore/ChangeLog:9
> +	   on Darwain, so that PlatformString knows CFString. 

Should be 'Darwin'. :)

> WebCore/ChangeLog:13
> +	   * config.h: (OS(DARWIN) && PLATFORM(GTK)) doesn't like ctypes

It's hard for me to understand exactly what the issue is here. Where does
OS(DARWIN) && PLATFORM(GTK) use ctypes, that it breaks the build to disallow
them.

> WebCore/config.h:136
> +#if !PLATFORM(QT) && !PLATFORM(WX) && !PLATFORM(CHROMIUM) &&
!PLATFORM(CHROMIUM) && !(OS(DARWIN) && PLATFORM(GTK))

You've added an extra !PLATFORM(CHROMIUM).

> WebCore/platform/UUID.cpp:35
> +#if OS(DARWIN) && PLATFORM(GTK)
> +#define WTF_PLATFORM_CF 1
> +#endif
>  #include "UUID.h"

It seems really unusual to do something like that and do it before the second
include. Maybe there's a cleaner way to do it?