WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED INVALID
32401
ifdefs for FreeBSD/OpenBSD
https://bugs.webkit.org/show_bug.cgi?id=32401
Summary
ifdefs for FreeBSD/OpenBSD
pvalchev
Reported
2009-12-10 17:17:39 PST
Simple ifdefs for WebKit to compile on FreeBSD/OpenBSD, based on patches by Ben Laurie and <
sprewell@jaggeri.com
>
Attachments
ifdefs for FreeBSD/OpenBSD
(13.79 KB, patch)
2009-12-10 17:18 PST
,
pvalchev
no flags
Details
Formatted Diff
Diff
With bug reference in ChangeLog
(13.91 KB, patch)
2009-12-11 13:44 PST
,
pvalchev
evan
: review-
Details
Formatted Diff
Diff
Latest patch with PLATFORM(UNIX) && !PLATFORM(DARWIN)
(13.81 KB, patch)
2009-12-23 16:54 PST
,
pvalchev
no flags
Details
Formatted Diff
Diff
PLATFORM(UNIX) && !PLATFORM(DARWIN)
(13.31 KB, patch)
2009-12-23 17:04 PST
,
pvalchev
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
pvalchev
Comment 1
2009-12-10 17:18:40 PST
Created
attachment 44651
[details]
ifdefs for FreeBSD/OpenBSD
Evan Martin
Comment 2
2009-12-11 11:09:28 PST
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?
pvalchev
Comment 3
2009-12-11 13:44:06 PST
Created
attachment 44707
[details]
With bug reference in ChangeLog
pvalchev
Comment 4
2009-12-11 13:46:34 PST
(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...
WebKit Review Bot
Comment 5
2009-12-11 13:48:21 PST
style-queue ran check-webkit-style on
attachment 44707
[details]
without any errors.
Darin Adler
Comment 6
2009-12-11 13:54:59 PST
Comment on
attachment 44707
[details]
With bug reference in ChangeLog Maybe some of these should be PLATFORM(UNIX) && !PLATFORM(DARWIN) instead?
Evan Martin
Comment 7
2009-12-11 19:57:12 PST
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
Comment 8
2009-12-16 03:54:53 PST
Attachment 44707
[details]
did not pass chromium-ews: Full output:
http://webkit-commit-queue.appspot.com/results/128520
Adam Barth
Comment 9
2009-12-16 07:49:29 PST
Sorry for the noise. This appears to be a bug in chromium-ews.
Laszlo Gombos
Comment 10
2009-12-18 15:30:10 PST
+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 11
2009-12-20 03:39:34 PST
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...).
pvalchev
Comment 12
2009-12-23 16:52:50 PST
(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?
pvalchev
Comment 13
2009-12-23 16:54:00 PST
Created
attachment 45456
[details]
Latest patch with PLATFORM(UNIX) && !PLATFORM(DARWIN) Changed per Darin's suggestion.
pvalchev
Comment 14
2009-12-23 17:04:24 PST
Created
attachment 45457
[details]
PLATFORM(UNIX) && !PLATFORM(DARWIN) Fixed small conflict left over from merge.
Laszlo Gombos
Comment 15
2009-12-24 06:17:03 PST
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)))) ?
pvalchev
Comment 16
2009-12-28 11:33:01 PST
(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
Comment 17
2010-02-28 09:04:40 PST
(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
Comment 18
2021-02-05 01:19:12 PST
PLATFORM(LINUX) is no longer a thing in the Source/WebCore.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug