Bug 50867 – [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6

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

[Koan-Sin Tan]

WebKitGtk+ doesn't build on Mac OS X 10.6 with MacPorts. "WebKitTools/Scripts/build-webkit --gtk" doesn't build, because
1. it requires geolocation, which is not in MacPorts, so "WebKitTools/Scripts/build-webkit --gtk --no-geolocation"
2. some files need to be modified, to run "build-webkit --gtk --no-geolocation", I'll submit a ptach
3. "WebKitTools/Scripts/run-launcher --gtk" doesn't launch the browser, because of wrong library path. Modification will be included in the path too

[Koan-Sin Tan]

Created attachment 76297 [details]
a quick patch to make WebKitGtk+ build on Mac OS X

[Mark Rowe (bdash)]

Comment on attachment 76297 [details]
a quick patch to make WebKitGtk+ build on Mac OS X

The changes in WebCore at the very least require explanation in the ChangeLog.

The change in UUID.cpp does not seem correct -- it looks as though it sends GTK on Mac OS X down the “not implemented” path.  Presumably if there is in fact a reason for this change then the ChangeLog entry would mention it.

[Koan-Sin Tan]

(In reply to comment #2)
> (From update of attachment 76297 [details])
> The changes in WebCore at the very least require explanation in the ChangeLog.
> 
> The change in UUID.cpp does not seem correct -- it looks as though it sends GTK on Mac OS X down the “not implemented” path.  Presumably if there is in fact a reason for this change then the ChangeLog entry would mention it.

Because CFString will be undeclared type with OS(DARWIN) && PLATFORM(GTK). Anyway, I know other ways to deal with this. will update the patch later. Thanks

[Mark Rowe (bdash)]

(In reply to comment #3)
> Because CFString will be undeclared type with OS(DARWIN) && PLATFORM(GTK). Anyway, I know other ways to deal with this. will update the patch later. Thanks

The file includes <CoreFoundation/CoreFoundation.h>.  CFString will be declared as a result of that.

[Koan-Sin Tan]

(In reply to comment #4)
> (In reply to comment #3)
> > Because CFString will be undeclared type with OS(DARWIN) && PLATFORM(GTK). Anyway, I know other ways to deal with this. will update the patch later. Thanks
> 
> The file includes <CoreFoundation/CoreFoundation.h>.  CFString will be declared as a result of that.

Yes, but I think I didn't explain it clear. The  problem is that CFString is not declared in the String class ("PlatformString"), which is included/declared in UUID.h. So #define WTF_PLATFORM_CF before including UUID.h is needed. If this sounds reasonable, I'll submit a modified patch.

[Koan-Sin Tan]

Created attachment 76319 [details]
proposed patch to make WebKitGtk+ build on Mac OS X

In UUID.cpp, let createCanonicalUUIDString be OS(DARWIN) one

[Early Warning System Bot]

Attachment 76319 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6964087

[WebKit Review Bot]

Attachment 76319 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/6896074

[Koan-Sin Tan]

Created attachment 76322 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

Oops, #endif is not in the right place

[Koan-Sin Tan]

Created attachment 77178 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

same as 76322, WebKitTools/ --> Toos/

[Martin Robinson]

Comment on attachment 77178 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

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?

[Koan-Sin Tan]

(In reply to comment #11)
> (From update of attachment 77178 [details])
> 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.

somewhere in #include "PluginObject.h", I'l dig it out.
And #include <X11/Xlib.h> will define Cursor again.
So, must #undef it before X11/Xlib.h
 
> > 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.
> 

I don't understand the GTK+ 2.x case, can you tell me more?

> 
> Should be 'Darwin'. :)
>

Thanks, will fix it.
 
> > 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.
>

I forgot the detail, will dig it out.
 
> 
> You've added an extra !PLATFORM(CHROMIUM).
>

I'll fix it, thanks.
 
> > 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?

move it into UUID.h? must define this before #include "PlatformString.h", which is the first include in UUID.h

[Koan-Sin Tan]

(In reply to comment #11)
> > 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.
> 

This is a bit tricky. Any Gtk file that includes <libintl.h>or <glib/gi18n-lib.h> will have trouble.
On Mac, libintl.h will include <xlocale.h>

<xclocale.h> includes <xlocale/_ctype.h>, which uses isacii(). If we use "DisallowCType.h"  (isascii() is disallowed), we'll see message shown below. Of course, we can do something like "#undef isascii" before including libintl.h or  glib/gi18n-lib.h, but don't use "DisallowCType.h" is easier.

In file included from /usr/include/xlocale.h:77,
                 from /usr/local/include/libintl.h:24,
                 from /opt/local/include/glib-2.0/glib/gi18n-lib.h:25,
                 from ../../WebKit/gtk/webkit/webkitnetworkresponse.cpp:30:
/usr/include/xlocale/_ctype.h: In function ‘int __istype_l(__darwin_ct_rune_t, long unsigned int, _xlocale*)’:
/usr/include/xlocale/_ctype.h:47: error: ‘isascii_WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h’ was not declared in this scope
/usr/include/xlocale/_ctype.h: In function ‘__darwin_ct_rune_t __toupper_l(__darwin_ct_rune_t, _xlocale*)’:
/usr/include/xlocale/_ctype.h:54: error: ‘isascii_WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h’ was not declared in this scope
/usr/include/xlocale/_ctype.h: In function ‘__darwin_ct_rune_t __tolower_l(__darwin_ct_rune_t, _xlocale*)’:
/usr/include/xlocale/_ctype.h:61: error: ‘isascii_WTF_Please_use_ASCIICType_instead_of_ctype_see_comment_in_ASCIICType_h’ was not declared in this scope

[Koan-Sin Tan]

(In reply to comment #11)
> (From update of attachment 77178 [details])
> 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.
> 

PluginObject.h will include WebCore/bridge/npapi.h and include QuickDraw stuff, which will typedef Cursor, and X11/Xlib.h wil #define Cursor again. That's the problem

[Koan-Sin Tan]

Created attachment 77307 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

add more explanations to Changelogs, fixed some typos

[Mark Rowe (bdash)]

Comment on attachment 77307 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

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

> WebCore/platform/UUID.h:37
>  #include "PlatformString.h"

This change to UUID.h is simply not correct.  It’s unsafe to alter the PLATFORM defines in individual files like this, even more so from a header file.

[Koan-Sin Tan]

(In reply to comment #16)
> (From update of attachment 77307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77307&action=review
> 
> > WebCore/platform/UUID.h:37
> >  #include "PlatformString.h"
> 
> This change to UUID.h is simply not correct.  It’s unsafe to alter the PLATFORM defines in individual files like this, even more so from a header file.

OK, how about #define PLATFORM_CF for  (OS(DARWIN) && PLATFORM(GTK)) in JavaScriptCore/wt/Platform.h? Will this be a correct change?

[Koan-Sin Tan]

Created attachment 77402 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

let OS(DARWIN) && PLATFORM(GTK) use the Chromium Linux one to generate UUID so that we can avoid the conflicts between CoreFoundation and Gtk/X11

[Koan-Sin Tan]

Created attachment 77407 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

same as 77402, #include necessary headers

[Eric Seidel (no email)]

Comment on attachment 77407 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

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

In general looks fine.  The Cursor re-def looks scary and fragile.

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

Why is this needed?

[Koan-Sin Tan]

(In reply to comment #20)
> (From update of attachment 77407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77407&action=review
> 
> In general looks fine.  The Cursor re-def looks scary and fragile.
> 

Yes. I know :-)

> > Tools/DumpRenderTree/unix/TestNetscapePlugin/TestNetscapePlugin.cpp:35
> > +#if OS(DARWIN) && PLATFORM(GTK)
> > +#define Cursor       QD_Cursor 
> > +#endif
> >  #include "PluginObject.h"
> > +#if OS(DARWIN) && PLATFORM(GTK)
> > +#undef Cursor
> > +#endif
> 
> Why is this needed?

To avoid typedef conflict. PluginObject.h will include WebCore/bridge/npapi.h and include QuickDraw stuff, which will typedef Cursor, and X11/Xlib.h wil typedef Cursor again. That's the problem.

I tried to avoid including QuickDraw without success. Because many headers in the<ApplicationServices/ApplicationServices.h>, which is used to include QuickDraw, depends on QuickDraw.

[Koan-Sin Tan]

Created attachment 77472 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

removed the scary Cursor re-def mentioned by Martin and Eric by adding -DXP_UNIX to TestNetscapePlugin_libtestnetscapeplugin_la_CPPFLAGS in Tools/GNUmakefile.am.

[Martin Robinson]

Comment on attachment 77472 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

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.

[Koan-Sin Tan]

(In reply to comment #23)
> (From update of attachment 77472 [details])
> 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.
> 

Thanks, I see what you mean this time, will do it.

> > 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.
>

OK.
 
> > 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?
> 

you mean
    #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && PLATFORM(GTK))
Yes, I agree. I like parenthesis. I did have parenthesis before, but Tools/Scripts/check-webkit-style doesn't like them. It told me
    WebCore/platform/UUID.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
I think that's a bug of check-webkit-style, since && has higher precedence than ||, I just removed parenthesis. To add them back, I have to find how to fix the check-webkit-style problem first :-)

[Koan-Sin Tan]

Created attachment 77533 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

updated 77472: added comments to WebCore/config.h and changed webkitdirs.pm as suggested by mrobinson

[David Levin]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 77472 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77472&action=review
> > > 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?
> > 
> 
> you mean
>     #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && PLATFORM(GTK))
> Yes, I agree. I like parenthesis. I did have parenthesis before, but Tools/Scripts/check-webkit-style doesn't like them. It told me
>     WebCore/platform/UUID.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
> I think that's a bug of check-webkit-style, since && has higher precedence than ||, I just removed parenthesis. To add them back, I have to find how to fix the check-webkit-style problem first :-)

Changes that fail check-webkit-style aren't disallowed. It is advisory that there is likely an issue there. In this case, it thinks there is a function call, but there isn't.



btw, if you'd like to fix check-webkit-style, the issue is in the regex below from Tools/Scripts/webkitpy/style/checkers/cpp.py (which I found by searching for the error text):

1340	        if (search(r'\w\s+\(', function_call)
1341	            and not search(r'#\s*define|typedef', function_call)):
1342	            error(line_number, 'whitespace/parens', 4,
1343	                  'Extra space before ( in function call')

[Koan-Sin Tan]

(In reply to comment #26)
> > 
> > you mean
> >     #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && PLATFORM(GTK))
> > Yes, I agree. I like parenthesis. I did have parenthesis before, but Tools/Scripts/check-webkit-style doesn't like them. It told me
> >     WebCore/platform/UUID.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
> > I think that's a bug of check-webkit-style, since && has higher precedence than ||, I just removed parenthesis. To add them back, I have to find how to fix the check-webkit-style problem first :-)
> 
> Changes that fail check-webkit-style aren't disallowed. It is advisory that there is likely an issue there. In this case, it thinks there is a function call, but there isn't.
> 
> 
> btw, if you'd like to fix check-webkit-style, the issue is in the regex below from Tools/Scripts/webkitpy/style/checkers/cpp.py (which I found by searching for the error text):
> 
> 1340            if (search(r'\w\s+\(', function_call)
> 1341                and not search(r'#\s*define|typedef', function_call)):
> 1342                error(line_number, 'whitespace/parens', 4,
> 1343                      'Extra space before ( in function call')


Thanks, adding a line like
        and not search(r'\A#elif', function_call)
a few lines before 1340 can avoid the problem. But this is a hack. It doesn't seem to be right . Any suggestion on how to do it properly? I'l file a bug on this

[Martin Robinson]

Comment on attachment 77533 [details]
Proposed patch to make WebKitGtk+ build on Mac OS X

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

> WebCore/config.h:139
> +// OS(DARWIN) && PLATFORM(GTK): Many Gtk related files include <libintl.h> or
> +// <glib/gi18n-lib.h>. On Mac, these two include <xlocale.h>. <xclocale.h>
> +// includes <xlocale/_ctype.h>, which uses isacii(). If we disallow ctype, 
> +// we'll have problem.

Small grammar issue here. I'll fix before landing myself.

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

Should have a parenthesis around these. I'll fix when landing.

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

Ditto.

[Mark Rowe (bdash)]

Why isn’t UUID.cpp looking at PLATFORM(CF) when deciding whether to use the CF-based implementation?  That seems a lot more obvious to me than the mess of conditionals in the patch.

[Martin Robinson]

Created attachment 77588 [details]
Version of the patch incorporating Sam's suggestion

[Martin Robinson]

(In reply to comment #29)
> Why isn’t UUID.cpp looking at PLATFORM(CF) when deciding whether to use the CF-based implementation?  That seems a lot more obvious to me than the mess of conditionals in the patch.

I don't have OS X here to test it, but I've uploaded a patch that takes this approach. Perhaps a Koan-Sin or Sam could test it.

[WebKit Review Bot]

Attachment 77588 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/GNUmakefile.am', u'Tools/Scripts/webkitdirs.pm', u'WebCore/ChangeLog', u'WebCore/config.h', u'WebCore/platform/UUID.cpp']" exit_code: 1
WebCore/platform/UUID.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/UUID.cpp:83:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Rowe (bdash)]

Comment on attachment 77588 [details]
Version of the patch incorporating Sam's suggestion

That appears to send Darwin-but-not-CF down the Linux path, which won’t work.

[Martin Robinson]

(In reply to comment #33)
> (From update of attachment 77588 [details])
> That appears to send Darwin-but-not-CF down the Linux path, which won’t work.

So it sounds like there may be no good way around more #ifdefs here. Now that I think of it, I'm curious why GTK+ on OS X cannot use the CF version.

[Koan-Sin Tan]

(In reply to comment #33)
> (From update of attachment 77588 [details])
> That appears to send Darwin-but-not-CF down the Linux path, which won’t work.

Yes, it won't work because OS X doesn't have /proc/sys/kernel/random/uuid, that why I added it to the OS(LINUX) && PLATFORM(CHROMIUM) part

[WebKit Review Bot]

Attachment 77588 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7274221

[Mark Rowe (bdash)]

it could be expressed more clearly as PLATFORM(DARWIN) && !PLATFORM(CF) on the correct #elif.

[Koan-Sin Tan]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 77588 [details] [details])
> > That appears to send Darwin-but-not-CF down the Linux path, which won’t work.
> 
> So it sounds like there may be no good way around more #ifdefs here. Now that I think of it, I'm curious why GTK+ on OS X cannot use the CF version.

To use CF version, we need PLATFORM(CF) to be true before including UUID.h, or before including PlatformString.h in UUID.h. That looks unsafe

[Mark Rowe (bdash)]

(In reply to comment #34)
> (In reply to comment #33)
> > (From update of attachment 77588 [details] [details])
> > That appears to send Darwin-but-not-CF down the Linux path, which won’t work.
> 
> So it sounds like there may be no good way around more #ifdefs here.

You didn’t implement what I suggested.  What I suggested would be both clearer and correct.

[Martin Robinson]

(In reply to comment #39)
> You didn’t implement what I suggested.  What I suggested would be both clearer and correct.

Sorry. I misunderstood your first comment. I was doing some minor cleanup on the patch while you posted your comment, so it seemed weird to land it without taking a shot at doing what you suggested. From your followup comment, I see that it was incorrect. I'll let Koan-Sin follow up, since he can actually test the change.

[Koan-Sin Tan]

Created attachment 77597 [details]
patch incorporated Martin's modifications and Mark's comments

using OS(DARWIN) and PLATFORM(CF)  to determine which UUID generation function we should use UUID.cpp


Note that  #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF)) will run into a bug of check-webkit-style and && has higher precedence than ||, so I just removed parentheses

[Mark Rowe (bdash)]

(In reply to comment #41)
> Note that  #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF)) will run into a bug of check-webkit-style and && has higher precedence than ||, so I just removed parentheses

It would be preferable to keep the more obvious style, file a bug about the bogus warning, and then simply ignore it for this patch.

[Koan-Sin Tan]

(In reply to comment #42)
> (In reply to comment #41)
> > Note that  #elif (OS(LINUX) && PLATFORM(CHROMIUM)) || (OS(DARWIN) && !PLATFORM(CF)) will run into a bug of check-webkit-style and && has higher precedence than ||, so I just removed parentheses
> 
> It would be preferable to keep the more obvious style, file a bug about the bogus warning, and then simply ignore it for this patch.

Done, bug 51695

[Koan-Sin Tan]

Created attachment 77981 [details]
proposed patch

updated patch, differences from 77597:
1. add parenthese in platform/UUID.cpp because bug 51695 is fixed
2. Tools/Scripts/webkitdirs.pm is updated because it was changed a bit

[WebKit Review Bot]

Attachment 77981 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Koan-Sin Tan]

Created attachment 78359 [details]
 proposed patch

rebased: WebCore -> Source/WebCore

[WebKit Review Bot]

Attachment 78359 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7411089

[Build Bot]

Attachment 78359 [details] did not build on win:
Build output: http://queues.webkit.org/results/7413099

[Martin Robinson]

Comment on attachment 78359 [details]
 proposed patch

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

Looks good, but I think there is a remaining issue below.

> Source/WebCore/platform/UUID.cpp:74
> -#elif OS(DARWIN)
> +elif OS(DARWIN) && PLATFORM(CF)

I don't think it's right here to remove the #

[Koan-Sin Tan]

Created attachment 78647 [details]
proposed patch

add the # back

[WebKit Review Bot]

Attachment 78647 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7431134

[Koan-Sin Tan]

Created attachment 78651 [details]
proposed patch

a #elif in 78647 is not in the right place, sorry about that

[Martin Robinson]

Comment on attachment 78651 [details]
proposed patch

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

> Tools/ChangeLog:9
> +        [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6
> +        https://bugs.webkit.org/show_bug.cgi?id=50867
> +
> +        [Gtk] WebKitGtk+ doesn't build on Mac OS X 10.6
> +        https://bugs.webkit.org/show_bug.cgi?id=50867

You have a double ChangeLog line here. I'll land this and fix that.

[Martin Robinson]

Committed r75619: <http://trac.webkit.org/changeset/75619>