Bug 192197 – Rename ENABLE_SUBTLE_CRYPTO to ENABLE_WEB_CRYPTO

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Don Olmstead]

Its really odd on the CMake side that there is ENABLE_WEB_CRYPTO and ENABLE_SUBTLE_CRYPTO. ENABLE_SUBTLE_CRYPTO seems to be pretty old and maybe its time to just change it to ENABLE_WEB_CRYPTO since that better describes things.

[Don Olmstead]

Created attachment 356082 [details]
Patch

Just a mass rename for the most part plus some CMake changes to remove ENABLE_SUBTLE_CRYPTO.

[Don Olmstead]

Created attachment 356087 [details]
Patch

Forgot some IDL files.

[Michael Catanzaro]

Yay!

[Michael Catanzaro]

Comment on attachment 356087 [details]
Patch

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

This looks good to be, just needs approval by Jiewen.

> Tools/Scripts/webkitperl/FeatureList.pm:-350
> -    { option => "subtle-crypto", desc => "Toggle WebCrypto Subtle-Crypto support",
> -      define => "ENABLE_SUBTLE_CRYPTO", value => \$subtleCrypto },
> -

Alternatively you could rename it to web-crypto so it's still possible to disable with build-webkit.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 356087 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356087&action=review
> 
> This looks good to be, just needs approval by Jiewen.
> 
> > Tools/Scripts/webkitperl/FeatureList.pm:-350
> > -    { option => "subtle-crypto", desc => "Toggle WebCrypto Subtle-Crypto support",
> > -      define => "ENABLE_SUBTLE_CRYPTO", value => \$subtleCrypto },
> > -
> 
> Alternatively you could rename it to web-crypto so it's still possible to
> disable with build-webkit.

I have some stuff locally where I'm going through and syncing up the perl script and the cmake side. A syncing of xcode features as well will either be in that or in a subsequent patch.

The feature flag stuff is wildly out of date.

[Jiewen Tan]

(In reply to Don Olmstead from comment #0)
> Its really odd on the CMake side that there is ENABLE_WEB_CRYPTO and
> ENABLE_SUBTLE_CRYPTO. ENABLE_SUBTLE_CRYPTO seems to be pretty old and maybe
> its time to just change it to ENABLE_WEB_CRYPTO since that better describes
> things.

Thanks for making this patch. However, I want to point it out the reason why this macro is named ENABLE_SUBTLE_CRYPTO instead of ENABLE_WEB_CRYPTO. Web Crypto API consists more than the SubtleCrypto interface. It means the whole Crypto interface: https://www.w3.org/TR/WebCryptoAPI/#crypto-interface. There is no flags for the crypto interface. Without the SubtleCrypto interface, there is the getRandomValues function. Even though SubtleCrypto interface is the bone and meat of Web Crypto API, it still feels semantically wrong to guard SubtleCrypto with ENABLE_WEB_CRYPTO.

Maybe it would be better to rename ENABLE_WEB_CRYPTO to ENABLE_SUBTLE_CRYPTO. From the Crypto class, I believe the crypto interface is enabled by default for all ports? Just the SubtleCrypto interface is platform dependent.

[Michael Catanzaro]

(In reply to Jiewen Tan from comment #6)
> Maybe it would be better to rename ENABLE_WEB_CRYPTO to
> ENABLE_SUBTLE_CRYPTO. From the Crypto class, I believe the crypto interface
> is enabled by default for all ports? Just the SubtleCrypto interface is
> platform dependent.

ENABLE_WEB_CRYPTO exists only to rename the ENABLE_SUBTLE_CRYPTO option so that it can be presented as a public build option. You'll see nothing is guarded by it, it's just a stub option so that we don't have to expose a confusing internal name ENABLE_SUBTLE_CRYPTO.

[Jiewen Tan]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Jiewen Tan from comment #6)
> > Maybe it would be better to rename ENABLE_WEB_CRYPTO to
> > ENABLE_SUBTLE_CRYPTO. From the Crypto class, I believe the crypto interface
> > is enabled by default for all ports? Just the SubtleCrypto interface is
> > platform dependent.
> 
> ENABLE_WEB_CRYPTO exists only to rename the ENABLE_SUBTLE_CRYPTO option so
> that it can be presented as a public build option. You'll see nothing is
> guarded by it, it's just a stub option so that we don't have to expose a
> confusing internal name ENABLE_SUBTLE_CRYPTO.

The whole thing is a little bit subtle. I saw confusions on both sides. Since the guard is not needed for Apple ports, I will r+ this patch as you both thinks it is good.

[Jiewen Tan]

Comment on attachment 356087 [details]
Patch

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 356087 [details]
Patch

Clearing flags on attachment: 356087

Committed r238754: <https://trac.webkit.org/changeset/238754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46384108>