Bug 28882 - Merge chromium's config.h.in into WebCore's config.h
Summary: Merge chromium's config.h.in into WebCore's config.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yaar Schnitman
URL:
Keywords:
Depends on:
Blocks: 28396
  Show dependency treegraph
 
Reported: 2009-09-01 11:56 PDT by Yaar Schnitman
Modified: 2009-10-02 11:22 PDT (History)
4 users (show)

See Also:


Attachments
Adds chromium specific defines to WebCore/config.h (3.22 KB, patch)
2009-09-03 17:57 PDT, Yaar Schnitman
levin: review-
Details | Formatted Diff | Diff
New Patch (2.18 KB, patch)
2009-09-09 13:15 PDT, Yaar Schnitman
levin: review-
Details | Formatted Diff | Diff
New New Patch (2.20 KB, patch)
2009-09-09 15:16 PDT, Yaar Schnitman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yaar Schnitman 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
Comment 1 Mark Rowe (bdash) 2009-09-01 13:24:57 PDT
That URL doesn't describe this at all.
Comment 2 Yaar Schnitman 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.
Comment 3 Yaar Schnitman 2009-09-03 17:57:29 PDT
Created attachment 39025 [details]
Adds chromium specific defines to WebCore/config.h
Comment 4 David Levin 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.
Comment 5 Yaar Schnitman 2009-09-09 13:15:04 PDT
Created attachment 39296 [details]
New Patch

David's feedback taken in.
Comment 6 Mark Rowe (bdash) 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.
Comment 7 David Levin 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.
Comment 8 Yaar Schnitman 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.
Comment 9 David Levin 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.
Comment 10 Yaar Schnitman 2009-09-09 15:16:45 PDT
Created attachment 39307 [details]
New New Patch

further feedback incorporated.
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2009-09-09 15:36:08 PDT
All reviewed patches have been landed.  Closing bug.