Bug 179095 - Sanity-check feature defaults in WebKitFeatures.cmake
Summary: Sanity-check feature defaults in WebKitFeatures.cmake
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-31 17:37 PDT by Michael Catanzaro
Modified: 2017-11-22 16:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (23.94 KB, patch)
2017-11-08 08:55 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (23.82 KB, patch)
2017-11-08 09:00 PST, Michael Catanzaro
annulen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-10-31 17:37:55 PDT
Sanity-check feature defaults in WebKitFeatures.cmake. Several features that are disabled by default here are being enabled on all ports.
Comment 1 Michael Catanzaro 2017-11-08 08:55:15 PST
Created attachment 326330 [details]
Patch
Comment 2 Michael Catanzaro 2017-11-08 09:00:37 PST
Created attachment 326331 [details]
Patch
Comment 3 Michael Catanzaro 2017-11-22 07:46:11 PST
This will be really hard to land if it's not approved soon; I see many changes in WebKitFeatures already since I uploaded this patch.
Comment 4 Konstantin Tokarev 2017-11-22 07:57:55 PST
Comment on attachment 326331 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:79
> +    # FIXME: Remove ENABLE_ALLINONE_BUILD.

This is already gone in trunk
Comment 5 Konstantin Tokarev 2017-11-22 08:10:41 PST
Comment on attachment 326331 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:117
> +    WEBKIT_OPTION_DEFINE(ENABLE_INDEXED_DATABASE_IN_WORKERS "Toggle support for indexed database in workers" PRIVATE ON)

Do we still need ENABLE_INDEXED_DATABASE_IN_WORKERS flag? Someone should ask Brady

> Source/cmake/WebKitFeatures.cmake:180
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEBGL "Toggle WebGL support" PRIVATE ON)

I think this should be OFF by default because this feature is not trivial to implement in new port. Just like with VIDEO

> Source/cmake/WebKitFeatures.cmake:185
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEB_CRYPTO "Whether to enable support for Web Crypto API." PRIVATE ON)

This flag doesn't seem to have any effect
Comment 6 Konstantin Tokarev 2017-11-22 08:17:30 PST
Comment on attachment 326331 [details]
Patch

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

Looks good overall, please rebase it and re-run EWS

> Source/cmake/OptionsMac.cmake:17
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_SELECTORS_LEVEL4 PRIVATE OFF)

This was probably a mistake, I don't see any platform-specific code for CSS_SELECTORS_LEVEL4 and it's quite old feature
Comment 7 Michael Catanzaro 2017-11-22 11:20:53 PST
(In reply to Konstantin Tokarev from comment #4)
> Comment on attachment 326331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326331&action=review
> 
> > Source/cmake/WebKitFeatures.cmake:79
> > +    # FIXME: Remove ENABLE_ALLINONE_BUILD.
> 
> This is already gone in trunk

Yes, once I get r+ I will have to audit all the changes that touched these files in the past two weeks. There were several.

(In reply to Konstantin Tokarev from comment #5)
> Comment on attachment 326331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326331&action=review
> 
> > Source/cmake/WebKitFeatures.cmake:117
> > +    WEBKIT_OPTION_DEFINE(ENABLE_INDEXED_DATABASE_IN_WORKERS "Toggle support for indexed database in workers" PRIVATE ON)
> 
> Do we still need ENABLE_INDEXED_DATABASE_IN_WORKERS flag? Someone should ask
> Brady

I don't know. Brady, can we get rid of it?

If so, it should be done in a different patch anyway.

> > Source/cmake/WebKitFeatures.cmake:180
> > +    WEBKIT_OPTION_DEFINE(ENABLE_WEBGL "Toggle WebGL support" PRIVATE ON)
> 
> I think this should be OFF by default because this feature is not trivial to
> implement in new port. Just like with VIDEO

As you prefer.

> > Source/cmake/WebKitFeatures.cmake:185
> > +    WEBKIT_OPTION_DEFINE(ENABLE_WEB_CRYPTO "Whether to enable support for Web Crypto API." PRIVATE ON)
> 
> This flag doesn't seem to have any effect

I agree this option is problematic. It toggles ENABLE_SUBTLE_CRYPTO. That option seems ill-named, and we did not want to expose it for GTK, so we added ENABLE_WEB_CRYPTO which just toggles that one. So it does do something, although it's useless for all other ports.

It could be moved to OptionsGTK.cmake.

(In reply to Konstantin Tokarev from comment #6)
> Comment on attachment 326331 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326331&action=review
> 
> Looks good overall, please rebase it and re-run EWS
> 
> > Source/cmake/OptionsMac.cmake:17
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_CSS_SELECTORS_LEVEL4 PRIVATE OFF)
> 
> This was probably a mistake, I don't see any platform-specific code for
> CSS_SELECTORS_LEVEL4 and it's quite old feature

I agree, but I don't want to change any feature values in this patch, so I added the FIXME to audit the options instead. This is not the only one that's problematic.
Comment 8 Michael Catanzaro 2017-11-22 11:22:54 PST
(In reply to Michael Catanzaro from comment #7)
> > > Source/cmake/WebKitFeatures.cmake:180
> > > +    WEBKIT_OPTION_DEFINE(ENABLE_WEBGL "Toggle WebGL support" PRIVATE ON)
> > 
> > I think this should be OFF by default because this feature is not trivial to
> > implement in new port. Just like with VIDEO
> 
> As you prefer.

Hm, actually, it seems useful to make new ports toggle this, so it can function as a TODO list of features that need to be implemented. I'd rather not have to list ENABLE_VIDEO ON in every port file... same applies to WebGL IMO.
Comment 9 Michael Catanzaro 2017-11-22 16:42:18 PST
(In reply to Michael Catanzaro from comment #8)
> Hm, actually, it seems useful to make new ports toggle this, so it can
> function as a TODO list of features that need to be implemented. I'd rather
> not have to list ENABLE_VIDEO ON in every port file... same applies to WebGL
> IMO.

I wound up implementing this. Sorry Konstantin. :P For ENABLE_VIDEO it doesn't actually simplify much anything, except reduce OptionsMac.cmake by two lines and GStreamerDependencies.cmake by one line, but every upstream port has ENABLE_VIDEO turned on and it's hard to justify not using that as the default. (Of course new ports will want to disable it at first.)
Comment 10 Michael Catanzaro 2017-11-22 16:44:05 PST
Also I'm going to sneak exposing ENABLE_API_TESTS on WPE into this patch.
Comment 11 Michael Catanzaro 2017-11-22 16:52:34 PST
(In reply to Michael Catanzaro from comment #10)
> Also I'm going to sneak exposing ENABLE_API_TESTS on WPE into this patch.

Nevermind, it's PRIVATE for GTK.
Comment 12 Michael Catanzaro 2017-11-22 16:57:38 PST
Committed r225105: <https://trac.webkit.org/changeset/225105>
Comment 13 Radar WebKit Bug Importer 2017-11-22 16:58:23 PST
<rdar://problem/35670772>
Comment 14 Michael Catanzaro 2017-11-22 16:59:56 PST
I also turned on ENABLE_VIDEO, ENABLE_VIDEO_TRACK, ENABLE_WEB_AUDIO, ENABLE_GEOLOCATION, and ENABLE_USERSELECT_ALL. I reserve the right to have forgotten one, but I think those are the only additional ones that I changed after Konstantin's review.