Bug 99271 - [GTK] Feature enabling/disabling should be possible through build-webkit
Summary: [GTK] Feature enabling/disabling should be possible through build-webkit
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:
Depends on:
Blocks: 87126
  Show dependency treegraph
 
Reported: 2012-10-14 13:11 PDT by Zan Dobersek
Modified: 2012-12-12 08:20 PST (History)
8 users (show)

See Also:


Attachments
WIP patch (16.31 KB, patch)
2012-10-16 06:07 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (25.25 KB, patch)
2012-10-18 03:49 PDT, Zan Dobersek
gns: 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-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