[Webkit-unassigned] [Bug 32401] ifdefs for FreeBSD/OpenBSD

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=32401





--- Comment #17 from Evan Martin <evan at chromium.org>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.