Bug 87127 - [Gtk] Move feature defines processing into a GNUmakefile that's simple to autogenerate
Summary: [Gtk] Move feature defines processing into a GNUmakefile that's simple to aut...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
: 85753 (view as bug list)
Depends on: 87995 90693 90696
Blocks: 85456
  Show dependency treegraph
 
Reported: 2012-05-22 06:22 PDT by Zan Dobersek
Modified: 2012-10-05 12:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (25.05 KB, patch)
2012-05-31 11:17 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (26.12 KB, patch)
2012-07-06 12:14 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (27.72 KB, patch)
2012-08-15 05:01 PDT, Zan Dobersek
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2012-05-22 06:22:43 PDT
The idea is to process all the feature defines in a simple-to-generate GNUmakefile.features.am. The end goal is to simplify adding a new feature define to the project, with all the relevant files being autogenerated (covered by bug #85456).

In GNUmakefile.features.am (in Source/WebCore) all the feature defines would be added to the FEATURE_DEFINES and webcore_cppflags variables, set to the default value - 1 for enabled, 0 otherwise. In GNUmakefile.am the GNUmakefile.features.am would be included. The values of feature defines would then be overridden based on the configuration options used when running the configure script. Currently there are many configuration options, but they should be narrowed down (bug #87126) so this wouldn't be such a big burden to maintain (if any).
Comment 1 Martin Robinson 2012-05-22 11:45:31 PDT
One important usecase to consider is that we often enable things for build-webkit and disable them for normal configure runs. This allows us to have the bots test something and not ship it.
Comment 2 Zan Dobersek 2012-05-23 06:13:34 PDT
(In reply to comment #1)
> One important usecase to consider is that we often enable things for build-webkit and disable them for normal configure runs. This allows us to have the bots test something and not ship it.

I can't imagine a solution to this from the top of my head other than manually going through the list of the feature defines and switching their status if necessary after the release branch is formed.
Comment 3 Martin Robinson 2012-05-23 06:17:30 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > One important usecase to consider is that we often enable things for build-webkit and disable them for normal configure runs. This allows us to have the bots test something and not ship it.
> 
> I can't imagine a solution to this from the top of my head other than manually going through the list of the feature defines and switching their status if necessary after the release branch is formed.

Unfortunately, it's not just the release branch, but also in unstable releases as well.
Comment 4 Zan Dobersek 2012-05-31 11:17:54 PDT
Created attachment 145118 [details]
Patch
Comment 5 Zan Dobersek 2012-07-06 12:14:11 PDT
Created attachment 151109 [details]
Patch
Comment 6 Zan Dobersek 2012-08-15 05:01:09 PDT
Created attachment 158546 [details]
Patch
Comment 7 Martin Robinson 2012-08-23 10:10:40 PDT
Comment on attachment 158546 [details]
Patch

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

Okay, but definitely watch the bots on this one. It may also be a good idea to double-check "make distcheck"

> Source/WebCore/GNUmakefile.am:650
>  	ACCELERATED_COMPOSITING=1

This introduces an unused -DACCELERATED_COMPOSITING variable. I guess that's not a terrible side-effect.

> Source/WebCore/GNUmakefile.am:793
> +# Add the feature defines to webcore_cppflags in macro form

Nit: Missing a period on this comment.

> GNUmakefile.am:64
> +FEATURE_DEFINES_DEFAULTS :=
> +FEATURE_DEFINES_OVERRIDES :=
> +FEATURE_DEFINES :=

I think it'd make sense to use small_case here to match other variables.
Comment 8 Zan Dobersek 2012-08-23 10:35:52 PDT
Comment on attachment 158546 [details]
Patch

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

>> Source/WebCore/GNUmakefile.am:650
>>  	ACCELERATED_COMPOSITING=1
> 
> This introduces an unused -DACCELERATED_COMPOSITING variable. I guess that's not a terrible side-effect.

I'll remove this define as it's not used at all.
Comment 9 Zan Dobersek 2012-08-25 12:14:17 PDT
Committed as http://trac.webkit.org/changeset/126450.
Comment 10 Zan Dobersek 2012-10-05 12:12:41 PDT
*** Bug 85753 has been marked as a duplicate of this bug. ***