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.
Created attachment 356082 [details] Patch Just a mass rename for the most part plus some CMake changes to remove ENABLE_SUBTLE_CRYPTO.
Created attachment 356087 [details] Patch Forgot some IDL files.
Yay!
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.
(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.
(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.
(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.
(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.
Comment on attachment 356087 [details] Patch LGTM. r=me.
Comment on attachment 356087 [details] Patch Clearing flags on attachment: 356087 Committed r238754: <https://trac.webkit.org/changeset/238754>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46384108>