Bug 135798 - [GTK] Subtle-crypto feature off by default and add a new configure flag to enable it
Summary: [GTK] Subtle-crypto feature off by default and add a new configure flag to en...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-08-11 06:06 PDT by Eduardo Lima Mitev
Modified: 2014-08-12 02:35 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.07 KB, patch)
2014-08-11 06:16 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2014-08-11 10:56 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (3.26 KB, patch)
2014-08-12 01:45 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff
Patch (3.26 KB, patch)
2014-08-12 01:52 PDT, Eduardo Lima Mitev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eduardo Lima Mitev 2014-08-11 06:06:56 PDT
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.
Comment 1 Eduardo Lima Mitev 2014-08-11 06:16:13 PDT
Created attachment 236365 [details]
Patch
Comment 2 Philippe Normand 2014-08-11 06:32:51 PDT
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())
Comment 3 Eduardo Lima Mitev 2014-08-11 07:28:28 PDT
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.
Comment 4 Eduardo Lima Mitev 2014-08-11 10:56:25 PDT
Created attachment 236382 [details]
Patch
Comment 5 Philippe Normand 2014-08-11 23:10:33 PDT
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?
Comment 6 Eduardo Lima Mitev 2014-08-11 23:52:53 PDT
"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.
Comment 7 Eduardo Lima Mitev 2014-08-12 01:43:04 PDT
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.
Comment 8 Eduardo Lima Mitev 2014-08-12 01:45:16 PDT
Created attachment 236432 [details]
Patch
Comment 9 Philippe Normand 2014-08-12 01:49:12 PDT
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.
Comment 10 Eduardo Lima Mitev 2014-08-12 01:52:55 PDT
Created attachment 236433 [details]
Patch
Comment 11 WebKit Commit Bot 2014-08-12 02:35:52 PDT
Comment on attachment 236433 [details]
Patch

Clearing flags on attachment: 236433

Committed r172439: <http://trac.webkit.org/changeset/172439>
Comment 12 WebKit Commit Bot 2014-08-12 02:35:57 PDT
All reviewed patches have been landed.  Closing bug.