Bug 135798 – [GTK] Subtle-crypto feature off by default and add a new configure flag to enable it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Eduardo Lima Mitev]

Now that patches for subtle crypto from bug 133122 are starting to land, it has been recommended to turn OFF the feature by default while the implementations for the different crypto algorithms are not there. Currently, the feature is ON by default and there are stubs for all algorithm implementations.

But we still want the build and tests bots to pick up this feature, so we need to add a new configure flag to enable it on specific builds.

[Eduardo Lima Mitev]

Created attachment 236365 [details]
Patch

[Philippe Normand]

Comment on attachment 236365 [details]
Patch

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

> Tools/Scripts/webkitperl/FeatureList.pm:380
> +      define => "ENABLE_SUBTLE_CRYPTO", default => 0, value => \$subtleCrypto },

Having a default value of 0 here will disable the feature unless --enable-subtle-crypto is explicitely passed. So I think you need to have isGtk() at least.
Is the feature enabled on Mac as well? If so the value would need to be (isGtk() || isMac())

[Eduardo Lima Mitev]

Philippe, good catch! Indeed, at least the Mac port has it on and also iOS (per Source/WebCore/Configurations/FeatureDefines.xcconfig). I think the default value is:

default => isAppleMacWebkit() || isIOSWebKit()

but it would be nice to have confirmation from someone more familiar with these ports. Neither EFL nor Window seem to implement the feature.

[Eduardo Lima Mitev]

Created attachment 236382 [details]
Patch

[Philippe Normand]

Comment on attachment 236382 [details]
Patch

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

> Tools/Scripts/webkitperl/FeatureList.pm:380
> +      define => "ENABLE_SUBTLE_CRYPTO", default => isAppleMacWebKit() || isIOSWebKit(), value => \$subtleCrypto },

I still don't understand why isGtk() is not there. Or you plan to enable it later?

[Eduardo Lima Mitev]

"default" is the default value for the flag. If the flag is not explicitly set in the command line of your build-webkit call (either --subtle-crypto or --no-subtle-crypto), that's the assumed value.

Now, we want the GTK port to have it off by default, hence isGtk() is not there.
And to activate it one must explicitly add --subtle-crypto, and then is enabled.

[Eduardo Lima Mitev]

Ok, I understood things wrongly. You meant having the flag on by default for GTK builds using build-webkit. Release builds don't use that so it is safe. The result is the same as I originally intended: The feature is available only to developers and build/test bots, but not present in releases.

Submitting new patch adding isGtk() :). Sorry for the noise.

[Eduardo Lima Mitev]

Created attachment 236432 [details]
Patch

[Philippe Normand]

Comment on attachment 236432 [details]
Patch

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

> Tools/Scripts/webkitperl/FeatureList.pm:380
> +      define => "ENABLE_SUBTLE_CRYPTO", default => isGtk() || isAppleMacWebKit() || isIOSWebKit(), value => \$subtleCrypto },

Sorry I should have mentioned that earlier, other options enclose the default value between parenthesis, check the video option for instance.
No idea if it's really needed but would be good to have it for consistency at least.

[Eduardo Lima Mitev]

Created attachment 236433 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 236433 [details]
Patch

Clearing flags on attachment: 236433

Committed r172439: <http://trac.webkit.org/changeset/172439>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.