Bug 191167 - [CMake] Sync feature defines
Summary: [CMake] Sync feature defines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-11-01 13:34 PDT by Don Olmstead
Modified: 2018-12-03 11:58 PST (History)
10 users (show)

See Also:


Attachments
Listing of all ENABLE_* in Source (4.03 KB, text/plain)
2018-11-01 17:08 PDT, Don Olmstead
no flags Details
Patch (46.11 KB, patch)
2018-11-05 12:35 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2018-12-03 10:21 PST, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2018-11-01 13:34:50 PDT
The feature defines are currently out of sync within CMake.
Comment 1 Don Olmstead 2018-11-01 17:08:16 PDT
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
Comment 2 Don Olmstead 2018-11-05 12:35:43 PST
Created attachment 353900 [details]
Patch
Comment 3 Don Olmstead 2018-11-05 13:04:57 PST
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.
Comment 4 Michael Catanzaro 2018-11-05 14:11:42 PST
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.
Comment 5 Ross Kirsling 2018-11-05 14:20:07 PST
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
Comment 6 Don Olmstead 2018-11-05 14:27:08 PST
(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.
Comment 7 Don Olmstead 2018-12-03 10:21:38 PST
Created attachment 356385 [details]
Patch
Comment 8 WebKit Commit Bot 2018-12-03 11:58:15 PST
Comment on attachment 356385 [details]
Patch

Clearing flags on attachment: 356385

Committed r238809: <https://trac.webkit.org/changeset/238809>
Comment 9 WebKit Commit Bot 2018-12-03 11:58:17 PST
All reviewed patches have been landed.  Closing bug.