Bug 99271

Summary: [GTK] Feature enabling/disabling should be possible through build-webkit
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, dpranke, gustavo, levin, mrobinson, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87126    
Attachments:
Description Flags
WIP patch
none
Patch gustavo: review+

Description Zan Dobersek 2012-10-14 13:11:58 PDT
Currently there is no way of enabling or disabling features by passing appropriate flags to the build-webkit script.
Comment 1 Zan Dobersek 2012-10-16 06:07:56 PDT
Created attachment 168933 [details]
WIP patch

A work-in-progress patch, using a very hack-ish approach.
Comment 2 Zan Dobersek 2012-10-18 03:49:51 PDT
Created attachment 169387 [details]
Patch
Comment 3 Zan Dobersek 2012-10-18 03:52:59 PDT
Hi Martin, kov, I'd appreciate a review of this.
Comment 4 Gustavo Noronha (kov) 2012-12-09 02:17:44 PST
Comment on attachment 169387 [details]
Patch

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

> Tools/Scripts/webkitdirs.pm:2032
> +    # Configurable features listed here should be kept in sync with the
> +    # features for which there exists a configuration option in configure.ac.
> +    my %configurableFeatures = (
> +        "filters" => 1,
> +        "gamepad" => 1,
> +        "geolocation" => 1,
> +        "indexed-database" => 1,
> +        "media-stream" => 1,
> +        "svg" => 1,
> +        "svg-fonts" => 1,
> +        "video" => 1,
> +        "webgl" => 1,
> +        "web-audio" => 1,
> +        "xslt" => 1,
> +    );
> +    my @overridableFeatures = ();
> +    foreach (@features) {
> +        if ($configurableFeatures{$_->{option}}) {
> +            push @buildArgs, autotoolsFlag(${$_->{value}}, $_->{option});;

In general I really like the way you're separating stuff that we allow configuration for and stuff build-webkit allows configuration for. I think it would be useful to have a comment around here explaining the overall architecture: that build-webkit parameters will in create a new GNUmakefile.features.am instead of passing parameters to autogen/configure. I think there's also the problem of we having to be more careful when generating the dist tarball - if we pass any parameters to build-webkit we'll get different defaults in the distributed tarball, so be good to make that explicit somewhere, this might be a good place for all that.
Comment 5 Zan Dobersek 2012-12-09 12:53:22 PST
Thanks for the review. If it's okay with you I'd also like to run a small test of creating the release tarball just to be sure things don't go fubar in that scenario.

Also, as discussed with Martin, this change opens the door for again using the build-webkit script (and the FeatureList module) as the preferred way of enabling and disabling the features in developer builds instead of using a configuration flag that overrides the feature defines in Source/WebCore/GNUmakefile.am
Comment 6 Zan Dobersek 2012-12-12 08:20:31 PST
(In reply to comment #4)
> In general I really like the way you're separating stuff that we allow configuration for and stuff build-webkit allows configuration for. I think it would be useful to have a comment around here explaining the overall architecture: that build-webkit parameters will in create a new GNUmakefile.features.am instead of passing parameters to autogen/configure. I think there's also the problem of we having to be more careful when generating the dist tarball - if we pass any parameters to build-webkit we'll get different defaults in the distributed tarball, so be good to make that explicit somewhere, this might be a good place for all that.

Announcing on the mailing list that people shouldn't use build-webkit for creating the tarballs.

The patch otherwise landed in r137270.
http://trac.webkit.org/changeset/137270