RESOLVED FIXED Bug 28882
Merge chromium's config.h.in into WebCore's config.h
https://bugs.webkit.org/show_bug.cgi?id=28882
Summary Merge chromium's config.h.in into WebCore's config.h
Yaar Schnitman
Reported 2009-09-01 11:56:18 PDT
As an initial step for the webkit chromium port, as described here: http://trac.webkit.org/wiki/Chromium
Attachments
Adds chromium specific defines to WebCore/config.h (3.22 KB, patch)
2009-09-03 17:57 PDT, Yaar Schnitman
levin: review-
New Patch (2.18 KB, patch)
2009-09-09 13:15 PDT, Yaar Schnitman
levin: review-
New New Patch (2.20 KB, patch)
2009-09-09 15:16 PDT, Yaar Schnitman
no flags
Mark Rowe (bdash)
Comment 1 2009-09-01 13:24:57 PDT
That URL doesn't describe this at all.
Yaar Schnitman
Comment 2 2009-09-03 17:41:51 PDT
The link describes the overall webkit chromium port, but indeed it doesn't explain this bug. Chromium today has its own config.h (over here http://src.chromium.org/svn/trunk/src/webkit/config.h.in), which was forked a while back from webkit's config.h. As I'm preparing the ground for the webkit port, I'm depreciating chromium's config.h and moving its chromium-specific defines into the ifdef PLATFORM(CHROMIUM) section of webkit's config.h.
Yaar Schnitman
Comment 3 2009-09-03 17:57:29 PDT
Created attachment 39025 [details] Adds chromium specific defines to WebCore/config.h
David Levin
Comment 4 2009-09-03 18:45:26 PDT
Comment on attachment 39025 [details] Adds chromium specific defines to WebCore/config.h > diff --git a/WebCore/config.h b/WebCore/config.h > @@ -17,7 +17,6 @@ > * Boston, MA 02110-1301, USA. > * > */ > - Please leave this the way it was. Also add an appropriate copyright line at the top (presumably Google). > +// TODO(mark): This list will hopefully shrink but may also grow. s/TODO(mark)/FIXME/ I guess this is a perpetual FIXME. > +// run "nm libwebcore.a | grep -E '[atsATS] ([+-]\[|\.objc_class_name)'" and > +#define WebCoreControlTintObserver \ > + ChromiumWebCoreObjCWebCoreControlTintObserver I didn't see this name when I ran "nm libwebcore.a | grep -E '[atsATS] ([+-]\[|\.objc_class_name)'" > +#else // !PLATFORM(DARWIN) > + > +// Don't define SKIA on Mac. Undefine other things as well that might get set > +// as side-effects. I find this comment very confusing. It says not to define it but then defines it. Of course, this is the non OSX section. How about this? // Only use SKIA on non-Mac platforms. > +#define WTF_PLATFORM_SKIA 1 // Undefine other items that may get set as side-effects. As side-effects of what (using SKIA)? > +#undef WTF_PLATFORM_CG > +#undef WTF_PLATFORM_CF > +#endif // if PLATFORM(CHROMIUM) No "if" here.
Yaar Schnitman
Comment 5 2009-09-09 13:15:04 PDT
Created attachment 39296 [details] New Patch David's feedback taken in.
Mark Rowe (bdash)
Comment 6 2009-09-09 13:26:46 PDT
Comment on attachment 39296 [details] New Patch > +// WebCore should compile with WTF_CHANGES as WTF does. > +#define WTF_CHANGES 1 Why is this being added? This define is not used in WebCore. The define only exists for use in the implementation of FastMalloc.
David Levin
Comment 7 2009-09-09 13:55:27 PDT
Comment on attachment 39296 [details] New Patch r- for now to answer questions/concerns below. > diff --git a/WebCore/config.h b/WebCore/config.h > + * Copyright (C) 2009, Google Inc. All rights reserved. No comma after 2009. > +// WebCore should compile with WTF_CHANGES as WTF does. > +#define WTF_CHANGES 1 According to this define is only needed for building FastMalloc, so it sounds like this shouldn't be here. Consider can/should these items be moved into the gyp file since other defines were moved there? #if !PLATFORM(DARWIN) // Define SKIA on non-Mac. #define WTF_PLATFORM_SKIA 1 #endif /* !PLATFORM(DARWIN) */ #define WTF_USE_GOOGLEURL 1 /* Chromium uses V8 by default */ #if !defined(WTF_USE_V8) #define WTF_USE_V8 1 #endif I don't have a good criteria for deciding what defines should be in each but would love to hear your ideas on this.
Yaar Schnitman
Comment 8 2009-09-09 14:55:19 PDT
Thanks for reviewing this. Here are my answers: (In reply to comment #7) > (From update of attachment 39296 [details]) > r- for now to answer questions/concerns below. > > > diff --git a/WebCore/config.h b/WebCore/config.h > > + * Copyright (C) 2009, Google Inc. All rights reserved. > No comma after 2009. Will fix. > > > +// WebCore should compile with WTF_CHANGES as WTF does. > > +#define WTF_CHANGES 1 > > According to this define is only needed for building FastMalloc, so it sounds > like this shouldn't be here. > Chromium needs WTF_CHANGES is because chromiums uses the same WebCore/config.h for compiling both wtf and webcore. I plan to solve this once I get to the next steps of the chromium port (this patch is the 1st step). I want to keep the patches small. > Consider can/should these items be moved into the gyp file since other defines > were moved there? > #if !PLATFORM(DARWIN) > // Define SKIA on non-Mac. > #define WTF_PLATFORM_SKIA 1 > #endif /* !PLATFORM(DARWIN) */ > #define WTF_USE_GOOGLEURL 1 > > /* Chromium uses V8 by default */ > #if !defined(WTF_USE_V8) > #define WTF_USE_V8 1 > #endif > > I don't have a good criteria for deciding what defines should be in each but > would love to hear your ideas on this. We try to avoid non-feature defines/undefs in gyp files, since: 1) gyp can only define but cannot undef. 2) developers tend to miss gyp defines since not in source files but in command line args or don't really understand gyp. We only use gyp defines when absolutely necessary or temp solution.
David Levin
Comment 9 2009-09-09 15:05:44 PDT
Last thing. I'm looking at the comments before the defines and none of them seem to add any value (e.g. "// Chromium doesn't use CFNetwork"). They restate what it being done but don't add a reason (add a why?), so I think they could all be removed. However, I would add one before WTF_CHANGES to explain why chromium needs this when no other build does.
Yaar Schnitman
Comment 10 2009-09-09 15:16:45 PDT
Created attachment 39307 [details] New New Patch further feedback incorporated.
WebKit Commit Bot
Comment 11 2009-09-09 15:36:04 PDT
Comment on attachment 39307 [details] New New Patch Clearing flags on attachment: 39307 Committed r48231: <http://trac.webkit.org/changeset/48231>
WebKit Commit Bot
Comment 12 2009-09-09 15:36:08 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.