Bug 28882 – Merge chromium's config.h.in into WebCore's config.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yaar Schnitman]

As an initial step for the webkit chromium port, as described here:
http://trac.webkit.org/wiki/Chromium

[Mark Rowe (bdash)]

That URL doesn't describe this at all.

[Yaar Schnitman]

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]

Created attachment 39025 [details]
Adds chromium specific defines to WebCore/config.h

[David Levin]

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]

Created attachment 39296 [details]
New Patch

David's feedback taken in.

[Mark Rowe (bdash)]

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 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]

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]

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]

Created attachment 39307 [details]
New New Patch

further feedback incorporated.

[WebKit Commit Bot]

Comment on attachment 39307 [details]
New New Patch

Clearing flags on attachment: 39307

Committed r48231: <http://trac.webkit.org/changeset/48231>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.