Bug 32401 – ifdefs for FreeBSD/OpenBSD

• Component: Platform
• Status: RESOLVED INVALID

Simple ifdefs for WebKit to compile on FreeBSD/OpenBSD, based on patches by Ben Laurie and <[e-mail]>

Created attachment 44651 [details]
ifdefs for FreeBSD/OpenBSD

[Evan Martin]

Comment on attachment 44651 [details]
ifdefs for FreeBSD/OpenBSD

> +++ JavaScriptCore/ChangeLog	(working copy)
> @@ -1,3 +1,9 @@
> [phone]  Peter Valchev <[e-mail]>
> +        Add OpenBSD ifdefs

You need a newline between the above two lines, here and elsewhere.
You should put a reference to this bug into your changelogs.

> +++ WebCore/page/EventHandler.cpp	(working copy)
> @@ -1791,7 +1791,7 @@
>      return swallowEvent;
>  }
>  
> -#if !PLATFORM(GTK) && !(PLATFORM(CHROMIUM) && PLATFORM(LINUX))
> +#if !PLATFORM(GTK) && !(PLATFORM(CHROMIUM) && (PLATFORM(LINUX) || PLATFORM(FREEBSD) || PLATFORM(OPENBSD)))

I fear we're going to need some sort of shared platform here.
The OS_NIX question again!


> -#elif defined(__linux__)
> +#elif defined(__linux__) || defined(__FreeBSD__) || defined(__OpenBSD__)
>  #include "FontPlatformDataLinux.h"

I guess this file needs to be renamed, huh?

Created attachment 44707 [details]
With bug reference in ChangeLog

(In reply to comment #2)
> (From update of attachment 44651 [details])
> > +++ JavaScriptCore/ChangeLog	(working copy)
> > @@ -1,3 +1,9 @@
> > [phone]  Peter Valchev <[e-mail]>
> > +        Add OpenBSD ifdefs
> 
> You need a newline between the above two lines, here and elsewhere.
> You should put a reference to this bug into your changelogs.

Done

> > +++ WebCore/page/EventHandler.cpp	(working copy)
> > @@ -1791,7 +1791,7 @@
> >      return swallowEvent;
> >  }
> >  
> > -#if !PLATFORM(GTK) && !(PLATFORM(CHROMIUM) && PLATFORM(LINUX))
> > +#if !PLATFORM(GTK) && !(PLATFORM(CHROMIUM) && (PLATFORM(LINUX) || PLATFORM(FREEBSD) || PLATFORM(OPENBSD)))
> 
> I fear we're going to need some sort of shared platform here.
> The OS_NIX question again!

Sure, although there aren't that many, so maybe this can be considered at a later point? :)

> > -#elif defined(__linux__)
> > +#elif defined(__linux__) || defined(__FreeBSD__) || defined(__OpenBSD__)
> >  #include "FontPlatformDataLinux.h"
> 
> I guess this file needs to be renamed, huh?

Posix? :) But that sort of brings back the previous problem...

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44707 [details] without any errors.

[Darin Adler]

Comment on attachment 44707 [details]
With bug reference in ChangeLog

Maybe some of these should be PLATFORM(UNIX) && !PLATFORM(DARWIN) instead?

[Evan Martin]

The pattern Darin suggests is similar to what we've been using in Chrome for the same problem.  We had a few discussions about a platform define that means "Unix but not OS X" that sorta wraps "everything else", since the long tail of free Unixes tend to match Linux rather than OS X.

[WebKit Review Bot]

Attachment 44707 [details] did not pass chromium-ews:
Full output: http://webkit-commit-queue.appspot.com/results/128520

[Adam Barth]

Sorry for the noise.  This appears to be a bug in chromium-ews.

[Laszlo Gombos]

+1 for Darin's suggestion.

There are few places with NETBSD specific code. I think this patch should also include NETBSD; Darin's suggestion will take care of that as well.

[Evan Martin]

Comment on attachment 44707 [details]
With bug reference in ChangeLog

Setting r- based on Darin's comments (and since this also touches Chromium code -- though I'm not a webkit reviewer I think it's not so unreasonable for me to r- it for the same reason...).

(In reply to comment #10)
> +1 for Darin's suggestion.
> 
> There are few places with NETBSD specific code. I think this patch should also
> include NETBSD; Darin's suggestion will take care of that as well.

I changed it per this suggestion. Uploading the new version now.

Can you please take another look?

Created attachment 45456 [details]
Latest patch with PLATFORM(UNIX) && !PLATFORM(DARWIN)

Changed per Darin's suggestion.

Created attachment 45457 [details]
PLATFORM(UNIX) && !PLATFORM(DARWIN)

Fixed small conflict left over from merge.

[Laszlo Gombos]

Comment on attachment 45457 [details]
PLATFORM(UNIX) && !PLATFORM(DARWIN)

The review flag is not set on the patch even though it seems like it is up for review.

> Index: WebCore/dom/SelectElement.cpp
> ===================================================================

> -#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && PLATFORM(LINUX))
> +#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(UNIX) && !PLATFORM(DARWIN)))

No need for the extra parenthesis - (PLATFORM(CHROMIUM) && PLATFORM(UNIX) && !PLATFORM(DARWIN))

For all changes under WebCore/platform/graphics/skia/*

> -#if defined(__linux__) || PLATFORM(WIN_OS)
> +#if (PLATFORM(UNIX) && !PLATFORM(DARWIN)) || PLATFORM(WIN_OS)

I'm not the best person to comment on this but it looks to me that Skia files aren't currently built on the Mac for Chromium (see WebCore/WebCore.gyp/WebCore.gyp and JavaScriptCore/wtf/Platform.h). This makes the !PLATFORM(DARWIN) guard redundant and potentially allows to get rid of all the guard conditions here.

> Index: WebCore/loader/CachedFont.cpp
> ===================================================================
> -#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || PLATFORM(LINUX))) || PLATFORM(HAIKU) || PLATFORM(WINCE)
> +#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || (PLATFORM(UNIX) && !PLATFORM(DARWIN)))) || PLATFORM(HAIKU) || PLATFORM(WINCE)

Similar comments as above - Could this be simplified to (PLATFORM(CHROMIUM) && !PLATFORM(DARWIN)) instead of (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || (PLATFORM(UNIX) && !PLATFORM(DARWIN)))) ?

(In reply to comment #15)
> (From update of attachment 45457 [details])
> The review flag is not set on the patch even though it seems like it is up for
> review.
> 
> > Index: WebCore/dom/SelectElement.cpp
> > ===================================================================
> 
> > -#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && PLATFORM(LINUX))
> > +#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(UNIX) && !PLATFORM(DARWIN)))
> 
> No need for the extra parenthesis - (PLATFORM(CHROMIUM) && PLATFORM(UNIX) &&
> !PLATFORM(DARWIN))
> 
> For all changes under WebCore/platform/graphics/skia/*

I thought they make it easier to read but of course feel free to remove them - or would you like me to upload a new patch without them? 

> > -#if defined(__linux__) || PLATFORM(WIN_OS)
> > +#if (PLATFORM(UNIX) && !PLATFORM(DARWIN)) || PLATFORM(WIN_OS)
> 
> I'm not the best person to comment on this but it looks to me that Skia files
> aren't currently built on the Mac for Chromium (see
> WebCore/WebCore.gyp/WebCore.gyp and JavaScriptCore/wtf/Platform.h). This makes
> the !PLATFORM(DARWIN) guard redundant and potentially allows to get rid of all
> the guard conditions here.

I'm not sure, I'd prefer if the Chromium guys make the call as I don't want to break the mac build.


> > Index: WebCore/loader/CachedFont.cpp
> > ===================================================================
> > -#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || PLATFORM(LINUX))) || PLATFORM(HAIKU) || PLATFORM(WINCE)
> > +#if PLATFORM(CG) || PLATFORM(QT) || PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) || (PLATFORM(UNIX) && !PLATFORM(DARWIN)))) || PLATFORM(HAIKU) || PLATFORM(WINCE)
> 
> Similar comments as above - Could this be simplified to (PLATFORM(CHROMIUM) &&
> !PLATFORM(DARWIN)) instead of (PLATFORM(CHROMIUM) && (PLATFORM(WIN_OS) ||
> (PLATFORM(UNIX) && !PLATFORM(DARWIN)))) ?

[Evan Martin]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 45457 [details] [details])
> > The review flag is not set on the patch even though it seems like it is up for
> > review.
> > 
> > > Index: WebCore/dom/SelectElement.cpp
> > > ===================================================================
> > 
> > > -#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && PLATFORM(LINUX))
> > > +#elif PLATFORM(GTK) || (PLATFORM(CHROMIUM) && (PLATFORM(UNIX) && !PLATFORM(DARWIN)))
> > 
> > No need for the extra parenthesis - (PLATFORM(CHROMIUM) && PLATFORM(UNIX) &&
> > !PLATFORM(DARWIN))
> > 
> > For all changes under WebCore/platform/graphics/skia/*
> 
> I thought they make it easier to read but of course feel free to remove them -
> or would you like me to upload a new patch without them? 

In WebKit-land, it's up to you to upload the perfect patch.


> > > -#if defined(__linux__) || PLATFORM(WIN_OS)
> > > +#if (PLATFORM(UNIX) && !PLATFORM(DARWIN)) || PLATFORM(WIN_OS)
> > 
> > I'm not the best person to comment on this but it looks to me that Skia files
> > aren't currently built on the Mac for Chromium (see
> > WebCore/WebCore.gyp/WebCore.gyp and JavaScriptCore/wtf/Platform.h). This makes
> > the !PLATFORM(DARWIN) guard redundant and potentially allows to get rid of all
> > the guard conditions here.
> 
> I'm not sure, I'd prefer if the Chromium guys make the call as I don't want to
> break the mac build.

WebKit style I've seen is to always build all cross-platform files on all platforms, then use ifdefs within the file to turn features on/off.  So I think the existing code is legit.


Peter, be sure to pick "details" and then set the review field to "?" to request someone to review your next patch.

[Martin Robinson]

PLATFORM(LINUX) is no longer a thing in the Source/WebCore.