Bug 110291 – Move WebCore feature defines to the WebCore directory

• Component: Platform
• Status: RESOLVED WONTFIX

[Laszlo Gombos]

Move WTF/wtf/FeatureDefaults.h (which currently only has feature defaults for WebCore) out from WTF into WebCore.

[Alexey Proskuryakov]

Should probably do something with FeatureDefines.xcconfig in JavaScriptCore as well then.

I don't know if having these definitions in WTF was done on purpose.

[Laszlo Gombos]

Created attachment 189355 [details]
Move from WTF to WebCore

One less file to change when a new WebCore Feature define is introduced. Changing a value of a WebCore define will not trigger rebuilding WTF or JavaScriptCore.

[Laszlo Gombos]

David, can you help reviewing this ?

[Laszlo Gombos]

Created attachment 189898 [details]
rebase at r143820

[Laszlo Gombos]

Feedback on irc: <benjaminp> I like the idea of moving WebCore configuration in WebCore. I would prefer someone who fixes the build system to review it. Like Dave Kilzer for example.

[David Kilzer (:ddkilzer)]

Comment on attachment 189898 [details]
rebase at r143820

View in context: https://bugs.webkit.org/attachment.cgi?id=189898&action=review

I like the general direction where this patch is going, but I'm hesitant to give the r+.  :)

Sam Weinig looked at this patch over my shoulder and he was okay with the direction as well.  (His only comment was about "FeatureDefines.h" sounding too generic, similar to my comment below.)

> Source/WebCore/FeatureDefines.h:30
> -#ifndef WTF_FeatureDefines_h
> -#define WTF_FeatureDefines_h
> +#ifndef FeatureDefines_h
> +#define FeatureDefines_h

What about naming this WebCoreFeatureDefines.h instead?  "FeatureDefines.h" is a bit generic, and if there are feature macros for JavaScriptCore (or WTF), I'd hate to have multiple "FeatureDefines.h" headers running around that were only differentiated by their path.

I also realize that would make the include be <WebCore/WebCoreFeatureDefines.h>, so I could go either way on this.

[Laszlo Gombos]

Created attachment 191053 [details]
rename FeatureDefines to WebCoreFeatureDefines

Renamed WebCore/FeatureDefines.h to WebCore/WebCoreFeatureDefines.h as suggested by David.

[Adam Barth]

Comment on attachment 191053 [details]
rename FeatureDefines to WebCoreFeatureDefines

View in context: https://bugs.webkit.org/attachment.cgi?id=191053&action=review

> Tools/DumpRenderTree/chromium/config.h:34
> +#include <WebCore/WebCoreFeatureDefines.h>

DumpRenderTree is not allowed to depend on WebCore directly in Chromium.

[Laszlo Gombos]

Created attachment 191065 [details]
remove change from chromium/config.h

Remove change from Tools/DumpRenderTree/chromium/config.h as suggested by Adam. For Chromium the defaults for the ENABLE() feature defines are set in the build system.

[Adam Barth]

Comment on attachment 191065 [details]
remove change from chromium/config.h

View in context: https://bugs.webkit.org/attachment.cgi?id=191065&action=review

> Source/WTF/wtf/FeatureDefines.h:-332
> -#if !defined(ENABLE_CANVAS_PATH)
> -#define ENABLE_CANVAS_PATH 1
> -#endif

What about these where the defaults are non-zero?

> Source/WTF/wtf/FeatureDefines.h:-344
> -#if !defined(ENABLE_CONTEXT_MENUS)
> -#define ENABLE_CONTEXT_MENUS 1
> -#endif

For example, this one isn't in features.gypi.

This seems very fragile!

[Adam Barth]

Please let me review this patch before landing.  I'm worried that you're not going to handle the Chromium case correctly and that this patch will lead to subtle bugs in the future.

[David Kilzer (:ddkilzer)]

Comment on attachment 191065 [details]
remove change from chromium/config.h

View in context: https://bugs.webkit.org/attachment.cgi?id=191065&action=review

> Source/WebCore/WebCoreFeatureDefines.h:574
> +#if !defined(JSC_OBJC_API_ENABLED)
> +#define JSC_OBJC_API_ENABLED (defined(__clang__) && defined(__APPLE__) && defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 && !defined(__i386__))
> +#endif

This is already defined in JSBase.h.  It should definitely not move to WebCore, and should probably just be removed from FeatureDefines.h (separate from this effort) since JSBase.h should be included wherever JSC_OBJC_API_ENABLED is used.  Tracking down a build failure because JSC_OBJC_API_ENABLED was defined differently in two places (and not knowing it was defined in FeatureDefines.h) required a lot of time.

[Mark Hahnenberg]

> This is already defined in JSBase.h.  It should definitely not move to WebCore, and should probably just be removed from FeatureDefines.h (separate from this effort) since JSBase.h should be included wherever JSC_OBJC_API_ENABLED is used.  Tracking down a build failure because JSC_OBJC_API_ENABLED was defined differently in two places (and not knowing it was defined in FeatureDefines.h) required a lot of time.

This flag was originally added to FeatureDefines.h so that it could be used in WebCore.exp.in to determine whether or not we should be exporting certain symbols based on whether the WebKit-related portion of the JSC ObjC API was enabled, but it turned out conditionally including certain symbols didn't work when building fat binaries, so it's not even necessary any more.

tl;dr: removing JSC_OBJC_API_ENABLED from FeatureDefines.h is a good idea.

[Mark Hahnenberg]

> tl;dr: removing JSC_OBJC_API_ENABLED from FeatureDefines.h is a good idea.

I filed bug 111269 for this.

[Laszlo Gombos]

(In reply to comment #10)
> (From update of attachment 191065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191065&action=review

> > Source/WTF/wtf/FeatureDefines.h:-344
> > -#if !defined(ENABLE_CONTEXT_MENUS)
> > -#define ENABLE_CONTEXT_MENUS 1
> > -#endif
> 
> For example, this one isn't in features.gypi.

Adam, thanks for spotting this. Do you have a suggestion how to export/forward this one particular header to DumpRenderTree for Chromium (without exposing WebCore entirely) ?

[Adam Barth]

I don't really understand what concrete problem this patch is try to solve.  I would just leave the header in WTF where it is at the base of the dependency diagram.

[Laszlo Gombos]

(In reply to comment #16)

> I don't really understand what concrete problem this patch is try to solve.  I would just leave the header in WTF where it is at the base of the dependency diagram.

The goal is to have feature defines follow the same encapsulation/layering as the rest of the code. WebCore feature flags should not be exposed to WTF and/or JSC, yet they are defined in WTF. I believe the change increases code readability.  

2 benefits are listed in comment 2
 - One less file to maintain when a WebCore feature flags is added (FeatureDefines.xcconfig)
 - Changing WebCore feature-flags does not trigger a build for JSC or WTF.

It seems that there is an agreement to have a separate WebCoreFeatureDefines.h from the rest of the feature defines (PlatformFeatureDefines.h?).

We could certainly have some/most of the benefits and still leave the WebCoreFeatureDefines.h in WTF.

Would it make sense to move WebCoreFeatureDefines.h to Source/Platform instead ?

[Adam Barth]

> Would it make sense to move WebCoreFeatureDefines.h to Source/Platform instead ?

That wouldn't help.

Chromium's DumpRenderTree is allowed to depend on WTF and Chromium's public API for WebKit.  It cannot see any other implementation details of the Source directory.  You're trying to make this header an implementation detail of WebCore, but Chromium's DumpRenderTree doesn't even know that WebCore exists.

[Maciej Stachowiak]

I think it's better to have the feature defines all in one place. Trying to define them in multiple different places increases the risk of an accidental mismatch. I think configuring the feature flags for WebKit is something that we should treat as having global scope to all of WebKit, even if some of them only affect some subdirectories of the tree.

[Laszlo Gombos]

(In reply to comment #19)

Thanks for the feedback.

Even though it seems that the idea was partially supported by a few, I do not feel strongly about this change, so I will close this bug as WONTFIX. I think having consensus on this matter that we as a project consistently follow going forward is more important.

> I think it's better to have the feature defines all in one place.

Maciej, I assume what you meant here is that there is a single entry point (FeatureDefines.h) for all WebKit configuration flags, but it does not necessary mean that all the configuration for all the ports are in a single header file. 

As described previously (https://lists.webkit.org/pipermail/webkit-dev/2013-January/023542.html) I assume that separating the port specific rules into port specific files are still a good idea.

FWIW, I have a WIP patch for EFL for how a port specific header file would look like - see bug 110717 .

My interpretation of the outcome of this discussion is that there is a consensus that the WTF directory is a good home for the WebKit wide configuration information.

I will file individual bugs for the work above, but please stop me now if you disagree with the direction.