[webkit-reviews] review granted: [Bug 99271] [GTK] Feature enabling/disabling should be possible through build-webkit : [Attachment 169387] Patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 99271: [GTK] Feature enabling/disabling should be possible through
build-webkit
https://bugs.webkit.org/show_bug.cgi?id=99271

Attachment 169387: Patch
https://bugs.webkit.org/attachment.cgi?id=169387&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.