Bug 191167 – [CMake] Sync feature defines

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

The feature defines are currently out of sync within CMake.

[Don Olmstead]

Created attachment 353663 [details]
Listing of all ENABLE_* in Source

Here's a listing of all the ENABLE_* flags found in the source code at this time

[Don Olmstead]

Created attachment 353900 [details]
Patch

[Don Olmstead]

Comment on attachment 353900 [details]
Patch

I searched in the XCode configs for ENABLE_* flags and if they weren't present I added them here.

I found a few ENABLE_* that were no longer in use like ENABLE_IOS_AIRPLAY, ENABLE_WEB_CRYPTO and ENABLE_WEBVTT_REGIONS.

I have some scripts around this hanging out locally. Perhaps in the future we can generate those two files rather than doing hand editing.

[Michael Catanzaro]

Comment on attachment 353900 [details]
Patch

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

Well you synced WebKitFeatures.cmake with FeatureList.pm, which is great, but what about the XCode build?

> Source/cmake/OptionsGTK.cmake:-139
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_CRYPTO PUBLIC ON)

This is here to enable turning off ENABLE_SUBTLE_CRYPTO. We didn't want to expose ENABLE_SUBTLE_CRYPTO as a public option, because that's not a good name. So we exposed ENABLE_WEB_CRYPTO instead. I think we want to keep this build option for now. What we could do is get rid of ENABLE_SUBTLE_CRYPTO by renaming all uses to ENABLE_WEB_CRYTPO. Jiewen would need to approve that.

[Ross Kirsling]

Comment on attachment 353900 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:-191
> -    WEBKIT_OPTION_DEFINE(ENABLE_WEBVTT_REGIONS "Toggle webvtt region support" PRIVATE OFF)

Was this meant to be removed or just moved?
There are a number of other occurrences: https://github.com/WebKit/webkit/search?q=ENABLE_WEBVTT_REGIONS&unscoped_q=ENABLE_WEBVTT_REGIONS

[Don Olmstead]

(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 353900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353900&action=review
> 
> Well you synced WebKitFeatures.cmake with FeatureList.pm, which is great,
> but what about the XCode build?
> 
> > Source/cmake/OptionsGTK.cmake:-139
> > -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEB_CRYPTO PUBLIC ON)
> 
> This is here to enable turning off ENABLE_SUBTLE_CRYPTO. We didn't want to
> expose ENABLE_SUBTLE_CRYPTO as a public option, because that's not a good
> name. So we exposed ENABLE_WEB_CRYPTO instead. I think we want to keep this
> build option for now. What we could do is get rid of ENABLE_SUBTLE_CRYPTO by
> renaming all uses to ENABLE_WEB_CRYTPO. Jiewen would need to approve that.

Ok I'll back off of that then and open a bug against it.

(In reply to Ross Kirsling from comment #5)
> Comment on attachment 353900 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353900&action=review
> 
> > Source/cmake/WebKitFeatures.cmake:-191
> > -    WEBKIT_OPTION_DEFINE(ENABLE_WEBVTT_REGIONS "Toggle webvtt region support" PRIVATE OFF)
> 
> Was this meant to be removed or just moved?
> There are a number of other occurrences:
> https://github.com/WebKit/webkit/
> search?q=ENABLE_WEBVTT_REGIONS&unscoped_q=ENABLE_WEBVTT_REGIONS

There's no source files referencing them.

[Don Olmstead]

Created attachment 356385 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 356385 [details]
Patch

Clearing flags on attachment: 356385

Committed r238809: <https://trac.webkit.org/changeset/238809>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.