Simple ifdefs for WebKit to compile on FreeBSD/OpenBSD, based on patches by Ben Laurie and <sprewell@jaggeri.com>
Created attachment 44651 [details] ifdefs for FreeBSD/OpenBSD
Comment on attachment 44651 [details] ifdefs for FreeBSD/OpenBSD > +++ JavaScriptCore/ChangeLog (working copy) > @@ -1,3 +1,9 @@ > +2009-12-10 Peter Valchev <pvalchev@google.com> > + 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 @@ > > +2009-12-10 Peter Valchev <pvalchev@google.com> > > + 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...
style-queue ran check-webkit-style on attachment 44707 [details] without any errors.
Comment on attachment 44707 [details] With bug reference in ChangeLog Maybe some of these should be PLATFORM(UNIX) && !PLATFORM(DARWIN) instead?
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.
Attachment 44707 [details] did not pass chromium-ews: Full output: http://webkit-commit-queue.appspot.com/results/128520
Sorry for the noise. This appears to be a bug in chromium-ews.
+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.
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.
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)))) ?
(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.
PLATFORM(LINUX) is no longer a thing in the Source/WebCore.