Bug 179095 – Sanity-check feature defaults in WebKitFeatures.cmake

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

Sanity-check feature defaults in WebKitFeatures.cmake. Several features that are disabled by default here are being enabled on all ports.

[Michael Catanzaro]

Created attachment 326330 [details]
Patch

[Michael Catanzaro]

Created attachment 326331 [details]
Patch

[Michael Catanzaro]

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.

[Konstantin Tokarev]

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

[Konstantin Tokarev]

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

[Konstantin Tokarev]

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

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.)

[Michael Catanzaro]

Also I'm going to sneak exposing ENABLE_API_TESTS on WPE into this patch.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Committed r225105: <https://trac.webkit.org/changeset/225105>

[Radar WebKit Bug Importer]

<rdar://problem/35670772>

[Michael Catanzaro]

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.