[webkit-reviews] review denied: [Bug 50867] [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6 : [Attachment 77472] 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 77472: Proposed patch to make WebKitGtk+ build on Mac OS X
https://bugs.webkit.org/attachment.cgi?id=77472&action=review

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

Great updates. Sorry that I forgot to explain my webkitdirs.pm comment earlier.
I've included that below.

> Tools/Scripts/webkitdirs.pm:599
> -	   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") {

I still think this needs to handle the GTK 3.x case properly. It makes sense to
just make the library extension conditional. For example (untested):

my $extension = isDarwin() ? "dylib" : "so";
if (-e $libraryDir . "libwebkitgtk-3.0.$extension") {
    return $libraryDir . "libwebkitgtk-3.0.$extension";
}
return $libraryDir . "libwebkitgtk-1.0.$extension";

> WebCore/config.h:-136
>  // this breaks compilation of <QFontDatabase>, at least, so turn it off for
now
>  // Also generates errors on wx on Windows, presumably because these
functions
>  // are used from wx headers.
> -#if !PLATFORM(QT) && !PLATFORM(WX) && !PLATFORM(CHROMIUM)

Might want to update the comment to explain why this is necessary for GTK+ here
too.

> WebCore/platform/UUID.cpp:46
> +#elif OS(LINUX) && PLATFORM(CHROMIUM) || OS(DARWIN) && PLATFORM(GTK)

Is it possible to use parenthesis here to make this less ambiguous to the
reader?

> WebCore/platform/UUID.cpp:96
> +#elif OS(LINUX) && PLATFORM(CHROMIUM) || OS(DARWIN) && PLATFORM(GTK)

Same here.