Bug 143558 – [GTK] Mark as advanced the build options we don't want to expose

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Some build options are unconditionally enabled/disabled for GTK+ and we don't want users to change that.

[Michael Catanzaro]

My current set of public options (with defaults)

--  ENABLE_3D_RENDERING ..................... ON
--  ENABLE_ACCELERATED_2D_CANVAS              ON
--  ENABLE_API_TESTS ........................ ON
--  ENABLE_GEOLOCATION                        ON
--  ENABLE_JIT                                ON
--  ENABLE_LLINT_C_LOOP ..................... OFF
--  ENABLE_MEMORY_SAMPLER ................... ON
--  ENABLE_MINIBROWSER                        ON
--  ENABLE_NETSCAPE_PLUGIN_API .............. ON
--  ENABLE_NETWORK_PROCESS                    ON
--  ENABLE_SPELLCHECK ....................... ON
--  ENABLE_SUBTLE_CRYPTO .................... ON
--  ENABLE_VIDEO ............................ ON
--  ENABLE_WEB_AUDIO                          ON
--  ENABLE_WEBGL                              ON
--  USE_SYSTEM_MALLOC                         OFF
--  ENABLE_CREDENTIAL_STORAGE                 ON
--  ENABLE_GLES2 ............................ OFF
--  ENABLE_GTKDOC                             ON
--  ENABLE_INTROSPECTION .................... ON
--  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON
--  ENABLE_PLUGIN_PROCESS_GTK2 .............. ON
--  ENABLE_X11_TARGET                         ON
--  ENABLE_WAYLAND_TARGET ................... OFF
--  USE_GSTREAMER_MPEGTS                      ON
--  USE_REDIRECTED_XCOMPOSITE_WINDOW ........ ON

Everything else I have private (advanced). This hides it from the user but does NOT prevent the user from setting it if he knows it exists.

* Did I miss any important options that should be public?
* Did I include anything that shouldn't really be public?

My guidelines:

* Anything we had a conditional statement for in OptionsGTK.cmake is a public option (except ENABLE_GAMEPAD_DEPRECATED; I figure that can be private since it's deprecated).
* A few new public options (USE_GSTREAMER_MPEGTS) added to make sure distros don't accidentally miss features. Distros have no chance of knowing that exists otherwise....
* A few things needed to build on non-x86 (JIT, LLINT_CLOOP, SYSTEM_MALLOC) also remain public.
* Otherwise, it becomes private.

I wonder if we can make even more private. E.g. can we make USE_REDIRECTED_XCOMPOSITE_WINDOW private and enable it only if both ENABLE_ACCELERATED_2D_CANVAS and ENABLE_X11_TARGET are set? Or does that need to be optional? What about ENABLE_3D_RENDERING?

I guess I should figure out why the dots aren't printed for the GTK-specific options.

Note: This patch also addresses bug #143546 because it was easiest to handle them both in the same patch, since this patch rejiggers so much of the file, but I can split those portions out if you want.

[Michael Catanzaro]

(In reply to comment #1)
> --  ENABLE_NETSCAPE_PLUGIN_API .............. ON

I guess this should be private, because the plugin process is mandatory.

(In reply to comment #1)
> --  ENABLE_GTKDOC                             ON

This actually defaults to OFF; I turn it on for myself. I'm not sure OFF is a good default -- I guess that's because it's slow to generate the docs -- but I don't care to change it here. (Also, MINIBROWSER and API_TESTS also still default to OFF except in developer mode.)

(In reply to comment #1)
> --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON

Maybe this should be USE_LIBNOTIFY?

[Michael Catanzaro]

(In reply to comment #2)
> I'm not sure OFF is a good default

It's a good default because users have probably not built all the gtkdoc of our dependencies, for the links to work correctly.

[Michael Catanzaro]

Created attachment 250498 [details]
[GTK] Mark as advanced the build options we don't want to expose

[Martin Robinson]

(In reply to comment #1)


This is a good start! Here's my rundown on the options. I'm probably wrong on a few of these.

> --  ENABLE_3D_RENDERING ..................... ON

3D rendering should always be enabled if we have OpenGL support and accelerated compositing, I think. This isn't something that's useful for the user to configure alone.

> --  ENABLE_NETWORK_PROCESS                    ON

Perhaps it is time to make this mandatory.

> --  ENABLE_SPELLCHECK ....................... ON

This was added to avoid an enchant dependency. It might be worth making this private as well.

> --  ENABLE_SUBTLE_CRYPTO .................... ON

Is this feature complete? If not, perhaps it should default to off.

> --  ENABLE_VIDEO ............................ ON

I'm okay with making this mandatory.

> --  ENABLE_WEB_AUDIO                          ON

This can probably be on by default unless it isn't ready. It's configurable at run time as well.

> --  ENABLE_WEBGL                              ON

This should always be on if we have OpenGL. I'm not sure it's worth making it configurable.

> --  ENABLE_CREDENTIAL_STORAGE                 ON

Only an option because we added the dependency after the last major release, I think. I can't recall exactly though.

> --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON

Same here.

> --  ENABLE_GTKDOC                             ON

I think this is an option because Gnomey packages and people who build them often turn it on and off.

> --  ENABLE_INTROSPECTION .................... ON

Ditto.

> --  ENABLE_X11_TARGET                         ON
> --  ENABLE_WAYLAND_TARGET ................... OFF

I really don't know why we have this option. If X11 or Wayland are available we should probably support them. One thing I'm not certain about is whether or not these can be enabled at the same time.

> --  USE_REDIRECTED_XCOMPOSITE_WINDOW ........ ON

I'm not sure this is worth showing. This is necessary for proper rendering in a GTK+ widget tree. This might be an issue on very poor hardware, but distributions shouldn't be touching it.


> I wonder if we can make even more private. E.g. can we make
> USE_REDIRECTED_XCOMPOSITE_WINDOW private and enable it only if both
> ENABLE_ACCELERATED_2D_CANVAS and ENABLE_X11_TARGET are set? Or does that
> need to be optional? What about ENABLE_3D_RENDERING?

The proper combination for redirected XComposite window is X11 and accelerated-compositing. Accelerated canvas shouldn't affect it one way or the other.

[Gwang Yoon Hwang]

(In reply to comment #5)
> (In reply to comment #1)
> 
> 
> This is a good start! Here's my rundown on the options. I'm probably wrong
> on a few of these.
> 
> > --  ENABLE_3D_RENDERING ..................... ON
> 
> 3D rendering should always be enabled if we have OpenGL support and
> accelerated compositing, I think. This isn't something that's useful for the
> user to configure alone.

I agree about it. We don't have to expose this at all.

> > --  ENABLE_WEBGL                              ON
> 
> This should always be on if we have OpenGL. I'm not sure it's worth making
> it configurable.
>

I'm also agree to enable this as a default. And we don't have to make it configurable.
 

> > --  ENABLE_X11_TARGET                         ON
> > --  ENABLE_WAYLAND_TARGET ................... OFF
> 
> I really don't know why we have this option. If X11 or Wayland are available
> we should probably support them. One thing I'm not certain about is whether
> or not these can be enabled at the same time.
> 

For now, we cannot support X11 and WAYLAND at sametime.
Until we can make it as a run-time option, I think we should expose them.

> > --  USE_REDIRECTED_XCOMPOSITE_WINDOW ........ ON
> 
> I'm not sure this is worth showing. This is necessary for proper rendering
> in a GTK+ widget tree. This might be an issue on very poor hardware, but
> distributions shouldn't be touching it.
> 

I agree to hide this option, because disabling REDIRECTED_XCOMPOSITE_WINDOW is a
some kind of hack to by-pass poor maintained devices.

[Carlos Garcia Campos]

(In reply to comment #2)
> (In reply to comment #1)
> > --  ENABLE_NETSCAPE_PLUGIN_API .............. ON
> 
> I guess this should be private, because the plugin process is mandatory.

Yes.

> (In reply to comment #1)
> > --  ENABLE_GTKDOC                             ON
> 
> This actually defaults to OFF; I turn it on for myself. I'm not sure OFF is
> a good default -- I guess that's because it's slow to generate the docs --
> but I don't care to change it here. (Also, MINIBROWSER and API_TESTS also
> still default to OFF except in developer mode.)

I think GTKDOC is OFF by default on non developer builds, enabled on developer builds. I wouldn't change that. Note also that this bug is about hiding private options, not changing the defaults.

> (In reply to comment #1)
> > --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON
> 
> Maybe this should be USE_LIBNOTIFY?

Where is this ENABLE_LIBNOTIFY_NOTIFICATIONS come from? LIBNOTIFY is not a feature you can enable/disable, it's an optional dependency to provide a default implementation of a feature that is always enabled.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #1)
> 
> 
> This is a good start! Here's my rundown on the options. I'm probably wrong
> on a few of these.
> 
> > --  ENABLE_3D_RENDERING ..................... ON
> 
> 3D rendering should always be enabled if we have OpenGL support and
> accelerated compositing, I think. This isn't something that's useful for the
> user to configure alone.

Agree. We could add an option to disable opengl in general, that would also disable this, but that would be a different option (some people using webkitgtk on headless envs asked for it, because having opengl with mesa makes everything unnecessarily slow for them).

> > --  ENABLE_NETWORK_PROCESS                    ON
> 
> Perhaps it is time to make this mandatory.

This is already mandatory, this should be just private.

> > --  ENABLE_SPELLCHECK ....................... ON
> 
> This was added to avoid an enchant dependency. It might be worth making this
> private as well.

I disagree, this should be ON by default, but public so that people can disable it if spellchecking is not needed.

> > --  ENABLE_SUBTLE_CRYPTO .................... ON
> 
> Is this feature complete? If not, perhaps it should default to off.

Maybe this is ON only when using build-webkit. Note that build-webkit overrides some of the features.

> > --  ENABLE_VIDEO ............................ ON
> 
> I'm okay with making this mandatory.

I disagree, video is still unneeded for some people, and gst dependency is not small. 

> > --  ENABLE_WEB_AUDIO                          ON
> 
> This can probably be on by default unless it isn't ready. It's configurable
> at run time as well.

Yes, I would enable this depending on whether deps are available.

> > --  ENABLE_WEBGL                              ON
> 
> This should always be on if we have OpenGL. I'm not sure it's worth making
> it configurable.

Agree, this should be private.

> > --  ENABLE_CREDENTIAL_STORAGE                 ON
> 
> Only an option because we added the dependency after the last major release,
> I think. I can't recall exactly though.

Same opinion than spellchecking. This is not needed in some cases like hybrid applications loading only local content. I agree with you from the linux distribution point of view, but in embedded it's quite common to build your own webkit optimized for your application, and having these options is important.

> > --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON
> 
> Same here.

This doesn't even exist, and doesn't make sense IMO. Maybe USE_LIBNOTIFY could be exposed.

> > --  ENABLE_GTKDOC                             ON
> 
> I think this is an option because Gnomey packages and people who build them
> often turn it on and off.

People building webkitgtk for a production image don't need this at all, and you save some build time and dont' need to have gtk-doc installed to build.

> > --  ENABLE_INTROSPECTION .................... ON
> 
> Ditto.

Ditto.

> > --  ENABLE_X11_TARGET                         ON
> > --  ENABLE_WAYLAND_TARGET ................... OFF
> 
> I really don't know why we have this option. If X11 or Wayland are available
> we should probably support them. One thing I'm not certain about is whether
> or not these can be enabled at the same time.

Again I think in embedded these options are useful, when building webkitgtk for a specific target system.

> > --  USE_REDIRECTED_XCOMPOSITE_WINDOW ........ ON
> 
> I'm not sure this is worth showing. This is necessary for proper rendering
> in a GTK+ widget tree. This might be an issue on very poor hardware, but
> distributions shouldn't be touching it.

This is probably a perfect example of an option that could be hidden, but still useful to be used by users. The problems I'm having with this in embedded are not poor performance, but a huge memory leak that killed the xserver after just a few minutes after running the MiniBrowser. In some other embedded systems, the problem was that xcomposite and/or xdamage extensions are not supported by the xserver. If you are not using GtkOverlay, bypassing the xcomposite window, makes everything more efficient.

> 
> > I wonder if we can make even more private. E.g. can we make
> > USE_REDIRECTED_XCOMPOSITE_WINDOW private and enable it only if both
> > ENABLE_ACCELERATED_2D_CANVAS and ENABLE_X11_TARGET are set? Or does that
> > need to be optional? What about ENABLE_3D_RENDERING?
> 
> The proper combination for redirected XComposite window is X11 and
> accelerated-compositing. Accelerated canvas shouldn't affect it one way or
> the other.

[Carlos Garcia Campos]

(In reply to comment #1)
> My current set of public options (with defaults)

In a developer build using build-webkit, right? Note that this is not what distros and users using the tarballs find.

> --  ENABLE_API_TESTS ........................ ON

I don't think api tests work for non developer builds, because some private symbols used by the tests are not exported in production builds. So this option should be hidden and enabled unconditionally for developer builds.

> --  USE_SYSTEM_MALLOC                         OFF

I think this should be hidden too, this is enabled by default in debug builds.

> --  USE_GSTREAMER_MPEGTS                      ON

This should be hidden as well, and enabled when video is ON and the required deps are found.

I would also expose some other options that are useful when building webkitgtk for a specific target device and environment. Things like ENABLE_ICONDATABASE or ENABLE_DRAG_SUPPORT. But maybe those are also examples of things that could be hidden, but still available.

[Michael Catanzaro]

Loooooong comment:

(In reply to comment #5)
> > --  ENABLE_SPELLCHECK ....................... ON
> 
> This was added to avoid an enchant dependency. It might be worth making this
> private as well.

Then Enchant would be a required dependency. :) Currently it is optional.

> > --  ENABLE_SUBTLE_CRYPTO .................... ON
> 
> Is this feature complete? If not, perhaps it should default to off.

I don't know, but it is already enabled by default (if gnutls-devel is installed) so this isn't a change. But I checked Fedora and openSUSE and neither installs gnutls-devel before building, so this will force them to either do so or explicitly disable the option. (Regarding downstream: Red Hat legal has to approve this before I can turn it on, I think, since it's crypto.)

> > --  ENABLE_VIDEO ............................ ON
> 
> I'm okay with making this mandatory.
> 
> > --  ENABLE_WEB_AUDIO                          ON
> 
> This can probably be on by default unless it isn't ready. It's configurable
> at run time as well.

These are optional to avoid making GStreamer a required dependency. (Which I am perfectly fine with adding, but you like to avoid that usually. :)

> > --  ENABLE_WEBGL                              ON
> 
> This should always be on if we have OpenGL. I'm not sure it's worth making
> it configurable.

Well half the point of this is to make clear what options are available, so that features are never disabled based on what is currently installed. If we don't want it to be configurable, then OpenGL should be mandatory; are we OK with doing that?

> > --  ENABLE_CREDENTIAL_STORAGE                 ON
> 
> Only an option because we added the dependency after the last major release,
> I think. I can't recall exactly though.
> 
> > --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON
> 
> Same here.

Are we OK with making enchant, libsecret, and libnotify mandatory? Every Linux distro will be fine with this; it could only possibly be an issue for embedded.

> > --  ENABLE_X11_TARGET                         ON
> > --  ENABLE_WAYLAND_TARGET ................... OFF
> 
> I really don't know why we have this option. If X11 or Wayland are available
> we should probably support them. One thing I'm not certain about is whether
> or not these can be enabled at the same time.

Again, I'd prefer the option to be either (a) private and always enabled or disabled regardless of whether X11 or Wayland is available, or (b) public.

> The proper combination for redirected XComposite window is X11 and
> accelerated-compositing. Accelerated canvas shouldn't affect it one way or
> the other.

TBH I thought accelerated canvas was the same as accelerated compositing. :p

(In reply to comment #6)
> For now, we cannot support X11 and WAYLAND at sametime.
> Until we can make it as a run-time option, I think we should expose them.

But note that with ENABLE_WAYLAND_TARGET turned OFF, the browser still works peachy fine in Wayland.

> I think GTKDOC is OFF by default on non developer builds, enabled on
> developer builds. I wouldn't change that. Note also that this bug is about
> hiding private options, not changing the defaults.

It's OFF by default for everyone. I agree not to change that in this bug, since there is so much else to discuss here already.

> Where is this ENABLE_LIBNOTIFY_NOTIFICATIONS come from? LIBNOTIFY is not a
> feature you can enable/disable, it's an optional dependency to provide a
> default implementation of a feature that is always enabled.

Yes, I understand. I think USE_LIBNOTIFY would be a bit less confusing; do you agree? libnotify should either be listed in the options or else made mandatory; otherwise, users won't get their notifications in Epiphany.

(In reply to comment #8)
> Agree. We could add an option to disable opengl in general, that would also
> disable this, but that would be a different option (some people using
> webkitgtk on headless envs asked for it, because having opengl with mesa
> makes everything unnecessarily slow for them).

OK, so let's replace all of the OpenGL-related options with simply ENABLE_OPENGL.
 
> > > --  ENABLE_SUBTLE_CRYPTO .................... ON
> > 
> > Is this feature complete? If not, perhaps it should default to off.
> 
> Maybe this is ON only when using build-webkit. Note that build-webkit
> overrides some of the features.

It's ON when gnutls-devel is installed. Indeed, build-webkit actually overrides most of the features. :) I do not consider what build-webkit does at all in this bug, only what happens when you run cmake directly! (Since build-webkit is for developers only.)

(In reply to comment #9)
> (In reply to comment #1)
> > My current set of public options (with defaults)
> 
> In a developer build using build-webkit, right? Note that this is not what
> distros and users using the tarballs find.

I had a script that manually passed -DDEVELOPER_MODE=ON -DENABLE_GTKDOC=ON, so yes. I should have been more clear about that; sorry.

> > --  ENABLE_API_TESTS ........................ ON
> 
> I don't think api tests work for non developer builds, because some private
> symbols used by the tests are not exported in production builds. So this
> option should be hidden and enabled unconditionally for developer builds.

OK, I agree. Let's expose DEVELOPER_MODE (default=OFF) instead.

> > --  USE_SYSTEM_MALLOC                         OFF
> 
> I think this should be hidden too, this is enabled by default in debug
> builds.

In the Fedora spec I see this:

%ifarch s390 s390x %{arm} ppc %{power64}
# Turn off bmalloc on secondary arches (as it is not ready for them)
%global optflags %{optflags} -DUSE_BMALLOC=0
%endif

Hence I think we need this.

> > --  USE_GSTREAMER_MPEGTS                      ON
> 
> This should be hidden as well, and enabled when video is ON and the required
> deps are found.

Eh, but again, I don't want to silently turn off support. E.g. in Fedora we have gstreamer1-plugins-base-devel in the buildroot, but that's not sufficient to get MPEG-TS, which needs gstreamer1-plugins-bad-free-devel. The presence of that weird devel package should not dictate whether users get the feature or not; there should be active choice required to turn it off.

> I would also expose some other options that are useful when building
> webkitgtk for a specific target device and environment. Things like
> ENABLE_ICONDATABASE or ENABLE_DRAG_SUPPORT. But maybe those are also
> examples of things that could be hidden, but still available.

OK, I'm fine with exposing those. You're thinking that embedded users will know for sure they don't want those features, right?

Summary of proposed changes:

* Remove ENABLE_NETWORK_PROCESS
* Replace ENABLE_3D_RENDERING, ENABLE_ACCELERATED_2D_CANVAS, and ENABLE_WEBGL with ENABLE_OPENGL
* Replace ENABLE_API_TESTS with DEVELOPER_MODE
* Replace ENABLE_LIBNOTIFY_NOTIFICATIONS with USE_LIBNOTIFY
* Add ENABLE_ICONDATABASE, ENABLE_DRAG_SUPPORT
* Keep USE_REDIRECTED_XCOMPOSITE_WINDOW since it can cause problems?
* Keep the rest, pending further discussion :)

[Carlos Garcia Campos]

(In reply to comment #10)
> Loooooong comment:
> 
> (In reply to comment #5)
> > > --  ENABLE_SPELLCHECK ....................... ON
> > 
> > This was added to avoid an enchant dependency. It might be worth making this
> > private as well.
> 
> Then Enchant would be a required dependency. :) Currently it is optional.
> 
> > > --  ENABLE_SUBTLE_CRYPTO .................... ON
> > 
> > Is this feature complete? If not, perhaps it should default to off.
> 
> I don't know, but it is already enabled by default (if gnutls-devel is
> installed) so this isn't a change. But I checked Fedora and openSUSE and
> neither installs gnutls-devel before building, so this will force them to
> either do so or explicitly disable the option. (Regarding downstream: Red
> Hat legal has to approve this before I can turn it on, I think, since it's
> crypto.)
> 
> > > --  ENABLE_VIDEO ............................ ON
> > 
> > I'm okay with making this mandatory.
> > 
> > > --  ENABLE_WEB_AUDIO                          ON
> > 
> > This can probably be on by default unless it isn't ready. It's configurable
> > at run time as well.
> 
> These are optional to avoid making GStreamer a required dependency. (Which I
> am perfectly fine with adding, but you like to avoid that usually. :)
> 
> > > --  ENABLE_WEBGL                              ON
> > 
> > This should always be on if we have OpenGL. I'm not sure it's worth making
> > it configurable.
> 
> Well half the point of this is to make clear what options are available, so
> that features are never disabled based on what is currently installed. If we
> don't want it to be configurable, then OpenGL should be mandatory; are we OK
> with doing that?

No.

> > > --  ENABLE_CREDENTIAL_STORAGE                 ON
> > 
> > Only an option because we added the dependency after the last major release,
> > I think. I can't recall exactly though.
> > 
> > > --  ENABLE_LIBNOTIFY_NOTIFICATIONS            ON
> > 
> > Same here.
> 
> Are we OK with making enchant, libsecret, and libnotify mandatory?

No.

> Every
> Linux distro will be fine with this; it could only possibly be an issue for
> embedded.

Embedded is enough reason to me.

> > > --  ENABLE_X11_TARGET                         ON
> > > --  ENABLE_WAYLAND_TARGET ................... OFF
> > 
> > I really don't know why we have this option. If X11 or Wayland are available
> > we should probably support them. One thing I'm not certain about is whether
> > or not these can be enabled at the same time.
> 
> Again, I'd prefer the option to be either (a) private and always enabled or
> disabled regardless of whether X11 or Wayland is available, or (b) public.
> 
> > The proper combination for redirected XComposite window is X11 and
> > accelerated-compositing. Accelerated canvas shouldn't affect it one way or
> > the other.
> 
> TBH I thought accelerated canvas was the same as accelerated compositing. :p

It's not.

> (In reply to comment #6)
> > For now, we cannot support X11 and WAYLAND at sametime.
> > Until we can make it as a run-time option, I think we should expose them.
> 
> But note that with ENABLE_WAYLAND_TARGET turned OFF, the browser still works
> peachy fine in Wayland.
> 
> > I think GTKDOC is OFF by default on non developer builds, enabled on
> > developer builds. I wouldn't change that. Note also that this bug is about
> > hiding private options, not changing the defaults.
> 
> It's OFF by default for everyone. I agree not to change that in this bug,
> since there is so much else to discuss here already.

GTKDOC is enabled by build-webkit, but the html generation is disabled because it's the slow part.

> > Where is this ENABLE_LIBNOTIFY_NOTIFICATIONS come from? LIBNOTIFY is not a
> > feature you can enable/disable, it's an optional dependency to provide a
> > default implementation of a feature that is always enabled.
> 
> Yes, I understand. I think USE_LIBNOTIFY would be a bit less confusing; do
> you agree? libnotify should either be listed in the options or else made
> mandatory; otherwise, users won't get their notifications in Epiphany.

Epiphany already depends on libnotify. Libnotify is not a feature, but an optional dependcy, the feature is NOTIFICATIONS and is always enabled (and should be hidden). Libnotify is always used when found in configure.

> (In reply to comment #8)
> > Agree. We could add an option to disable opengl in general, that would also
> > disable this, but that would be a different option (some people using
> > webkitgtk on headless envs asked for it, because having opengl with mesa
> > makes everything unnecessarily slow for them).
> 
> OK, so let's replace all of the OpenGL-related options with simply
> ENABLE_OPENGL.

That would work, yes. But still, this bug is about hidding existing options.
 
> > > > --  ENABLE_SUBTLE_CRYPTO .................... ON
> > > 
> > > Is this feature complete? If not, perhaps it should default to off.
> > 
> > Maybe this is ON only when using build-webkit. Note that build-webkit
> > overrides some of the features.
> 
> It's ON when gnutls-devel is installed. Indeed, build-webkit actually
> overrides most of the features. :) I do not consider what build-webkit does
> at all in this bug, only what happens when you run cmake directly! (Since
> build-webkit is for developers only.)

Exactly, that was my point.

> (In reply to comment #9)
> > (In reply to comment #1)
> > > My current set of public options (with defaults)
> > 
> > In a developer build using build-webkit, right? Note that this is not what
> > distros and users using the tarballs find.
> 
> I had a script that manually passed -DDEVELOPER_MODE=ON -DENABLE_GTKDOC=ON,
> so yes. I should have been more clear about that; sorry.

Ok.

> > > --  ENABLE_API_TESTS ........................ ON
> > 
> > I don't think api tests work for non developer builds, because some private
> > symbols used by the tests are not exported in production builds. So this
> > option should be hidden and enabled unconditionally for developer builds.
> 
> OK, I agree. Let's expose DEVELOPER_MODE (default=OFF) instead.

Yes.

> > > --  USE_SYSTEM_MALLOC                         OFF
> > 
> > I think this should be hidden too, this is enabled by default in debug
> > builds.
> 
> In the Fedora spec I see this:
> 
> %ifarch s390 s390x %{arm} ppc %{power64}
> # Turn off bmalloc on secondary arches (as it is not ready for them)
> %global optflags %{optflags} -DUSE_BMALLOC=0
> %endif
> 
> Hence I think we need this.

Good point.

> > > --  USE_GSTREAMER_MPEGTS                      ON
> > 
> > This should be hidden as well, and enabled when video is ON and the required
> > deps are found.
> 
> Eh, but again, I don't want to silently turn off support. E.g. in Fedora we
> have gstreamer1-plugins-base-devel in the buildroot, but that's not
> sufficient to get MPEG-TS, which needs gstreamer1-plugins-bad-free-devel.
> The presence of that weird devel package should not dictate whether users
> get the feature or not; there should be active choice required to turn it
> off.

We can show warnings when a dep is not found and it hasn't been explicitly disabled. Distros not building with the right options is not a reason to make dependencies mandatory.

> > I would also expose some other options that are useful when building
> > webkitgtk for a specific target device and environment. Things like
> > ENABLE_ICONDATABASE or ENABLE_DRAG_SUPPORT. But maybe those are also
> > examples of things that could be hidden, but still available.
> 
> OK, I'm fine with exposing those. You're thinking that embedded users will
> know for sure they don't want those features, right?

Yes, the same way distros should, btw.

> Summary of proposed changes:
> 
> * Remove ENABLE_NETWORK_PROCESS
> * Replace ENABLE_3D_RENDERING, ENABLE_ACCELERATED_2D_CANVAS, and
> ENABLE_WEBGL with ENABLE_OPENGL
> * Replace ENABLE_API_TESTS with DEVELOPER_MODE
> * Replace ENABLE_LIBNOTIFY_NOTIFICATIONS with USE_LIBNOTIFY
> * Add ENABLE_ICONDATABASE, ENABLE_DRAG_SUPPORT
> * Keep USE_REDIRECTED_XCOMPOSITE_WINDOW since it can cause problems?
> * Keep the rest, pending further discussion :)

[Michael Catanzaro]

Carlos, I think we're on the same page for all of these, with maybe one exception:

(In reply to comment #11)
> We can show warnings when a dep is not found and it hasn't been explicitly
> disabled. Distros not building with the right options is not a reason to
> make dependencies mandatory.

I think it should not be mandatory, but an option. How about I make it so that it's turned off automatically when either ENABLE_VIDEO or ENABLE_AUDIO is off? But if building with both VIDEO and AUDIO support, then I think we should fail the build unless it's explicitly disabled. I don't think we should show a warning without failing the build, because that would be hard to notice. That's how we wound up with this feature disabled in every distro (I checked Fedora, openSUSE, and Debian).

I will ping Phillipe to see if maybe he wants it disabled by default, but I suspect that's not his intention.

[Michael Catanzaro]

(In reply to comment #12)
> I think it should not be mandatory, but an option. How about I make it so
> that it's turned off automatically when either ENABLE_VIDEO or ENABLE_AUDIO
> is off? But if building with both VIDEO and AUDIO support

But I guess we only use it for video?

[Philippe Normand]

The gstreamer-mpegts support we have currently should be considered experimental, it also depends on unstable gst API (from gat-plugins-bad) and shouldn't be enabled in production builds IMHO.

For developer builds I think it's still fine to keep it as it is, e.g., if the API is available let's use it.

[Michael Catanzaro]

OK I will change that then. And I forgot about GStreamer GL, what about that: should it be on or off by default?

[Martin Robinson]

(In reply to comment #15)
> OK I will change that then. And I forgot about GStreamer GL, what about
> that: should it be on or off by default?

I think there are some performance concerns, since it operates on another thread with a sharing context. I'd definitely like to see some performance numbers before turning it on by default.

[Michael Catanzaro]

It's already on "by default," automagically based on whether gstreamer-gl is installed. I will make it off by default instead.

One more question: should it be a public option? I guess not, and will mark it advanced, unless someone tells me otherwise, since it is very new.

[Martin Robinson]

(In reply to comment #17)
> It's already on "by default," automagically based on whether gstreamer-gl is
> installed. I will make it off by default instead.
> 
> One more question: should it be a public option? I guess not, and will mark
> it advanced, unless someone tells me otherwise, since it is very new.

Yeah, I think all of the incomplete features should be hidden. They will be turned on and off mainly by build-webkit.

[Philippe Normand]

The gstreamer-gl case is similar to the gstreamer-mpegts case.

Unstable gst API, we shouldn't enable it in production build until it is mature enough. 

I don't think performance is going to a huge concern here, actually we will benefit from hw-decoding support and zero-copy rendering from glimagesink (and underneath hw decoders support) there. Not even mentioning the reduced maintenance cost, some day we' ll be able to drop our internal video sink all together, when glimagesink is widely available in distros.

[Martin Robinson]

(In reply to comment #19)
> The gstreamer-gl case is similar to the gstreamer-mpegts case.
> 
> Unstable gst API, we shouldn't enable it in production build until it is
> mature enough. 
> 
> I don't think performance is going to a huge concern here, actually we will
> benefit from hw-decoding support and zero-copy rendering from glimagesink
> (and underneath hw decoders support) there. Not even mentioning the reduced
> maintenance cost, some day we' ll be able to drop our internal video sink
> all together, when glimagesink is widely available in distros.

The issue is that at least for some drivers, as soon as you let the sharing context cross a thread boundary, the driver starts protecting *all* GL operations using those sharing contexts with locks. I think that to defend against this, we can allow GL decoding, but we need to receive the surfaces from gstreamer as some hardware, but non-GL type. I'll try to find out more information about this, but I know this is one consideration that is driving the design of Servo.

[Michael Catanzaro]

OK, next iteration. Here are the exposed options:

-- Enabled features:
--  ENABLE_CREDENTIAL_STORAGE ............... ON
--  ENABLE_DRAG_SUPPORT                       ON
--  ENABLE_GEOLOCATION ...................... ON
--  ENABLE_GTKDOC ........................... ON
--  ENABLE_ICONDATABASE ..................... ON
--  ENABLE_INTROSPECTION .................... ON
--  ENABLE_JIT                                ON
--  ENABLE_LLINT_C_LOOP ..................... OFF
--  ENABLE_MEMORY_SAMPLER ................... ON
--  ENABLE_MINIBROWSER                        ON
--  ENABLE_PLUGIN_PROCESS_GTK2                ON
--  ENABLE_SPELLCHECK                         ON
--  ENABLE_SUBTLE_CRYPTO                      ON
--  ENABLE_VIDEO ............................ ON
--  ENABLE_WAYLAND_TARGET                     OFF
--  ENABLE_WEB_AUDIO ........................ ON
--  ENABLE_X11_TARGET ....................... ON
--  USE_DEBUG_FISSION                         ON
--  USE_LD_GOLD                               ON
--  USE_LIBNOTIFY                             ON
--  USE_OPENGL .............................. ON
--  USE_SYSTEM_MALLOC                         OFF

Notes:

* I moved ENABLE_GLES2 to be private, since it's new. Is it still experimental, or is it good enough to make public? CC Victor for advice (hi! Don't think we've met!)
* Based on the comments from Yoon and Carlos, I can't decide whether or not to expose USE_REDIRECTED_XCOMPOSITE_WINDOW: think it would be fine to either way. I moved it to private and left a comment "Should never be needed. Disable to workaround hardware support issues." Hope that's right.
* I needed to move things around quite a bit, to make the GTK-specific options into real WebKit options that get printed properly, rather than secret CMake variables. I've tried to not break configurations but it's impossible to test properly due to the combinatorial nature of the problem and long build times.
* I did not expose DEVELOPER_MODE.
* This patch also solves bug #143546

TODO:

* Make the WEBKIT_DEPEND macro work when an option depends on multiple other options (should be done in another patch). This is a minor point.
* Expose USE_LD_GOLD and USE_DEBUG_FISSION (should be done in another patch). I doctored the list above to show my plan to make them public.
* Figure out how I broke the dots (they should print every other line).

[Michael Catanzaro]

Made the same mistake twice: ENABLE_GTKDOC and ENABLE_MINIBROWSER both default to OFF, not ON (my script uses ENABLE_GTKDOC=ON and also DEVELOPER_MODE=ON, which turns ENABLE_MINIBROWSER on automatically, as mentioned above).

I wonder just a bit about whether subtle crypto should be enabled by default. (As mentioned above, it's currently enabled automatically if you have the GnuTLS development package installed.) Sergio is the person to ask, right?

[Martin Robinson]

(In reply to comment #22)
> Made the same mistake twice: ENABLE_GTKDOC and ENABLE_MINIBROWSER both
> default to OFF, not ON (my script uses ENABLE_GTKDOC=ON and also
> DEVELOPER_MODE=ON, which turns ENABLE_MINIBROWSER on automatically, as
> mentioned above).
> 
> I wonder just a bit about whether subtle crypto should be enabled by
> default. (As mentioned above, it's currently enabled automatically if you
> have the GnuTLS development package installed.) Sergio is the person to ask,
> right?

I think Eduardo Lima is the person behind the WebCrypto support.

[Michael Catanzaro]

Created attachment 250593 [details]
Patch

[Michael Catanzaro]

Hola Eduardo! Is the subtle crypto feature still experimental, or is it OK to enable by default? Currently it's enabled automatically if you have the GnuTLS development package installed, but I want to change that to either ON (causing the build to fail if gnutls-devel is missing unless -DENABLE_SUBTLE_CRYPTO=OFF has been passed to cmake) or OFF (so you have to use -DENABLE_SUBTLE_CRYPTO=ON or you won't get it). In the patch above, I guessed it's ready and picked ON. Note: this doesn't affect developers who use the build-webkit script (which sets it to ON).

[Michael Catanzaro]

Comment on attachment 250593 [details]
Patch

Seems to have broken ENABLE_MEDIA_STREAM....

[Philippe Normand]

(In reply to comment #26)
> Comment on attachment 250593 [details]
> Patch
> 
> Seems to have broken ENABLE_MEDIA_STREAM....

Seems like mediatream is enabled but your patch disables openwebrtc support.

[Philippe Normand]

Comment on attachment 250593 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:123
> +# These options are private because they are experimental.
> +WEBKIT_OPTION_DEFINE(ENABLE_GLES2 "Whether to enable use of OpenGL ES 2.0." OFF)
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_MPEGTS "Whether to enable support for MPEG-TS." OFF)
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL." OFF)
> +WEBKIT_OPTION_DEFINE(USE_OPENWEBRTC "Whether to enable support for WebRTC." OFF)

This isn't right imho. For developer builds at least, if those libraries are present they should be used.

[Carlos Garcia Campos]

(In reply to comment #21)
> OK, next iteration. Here are the exposed options:
> 
> -- Enabled features:
> --  ENABLE_CREDENTIAL_STORAGE ............... ON
> --  ENABLE_DRAG_SUPPORT                       ON
> --  ENABLE_GEOLOCATION ...................... ON
> --  ENABLE_GTKDOC ........................... ON
> --  ENABLE_ICONDATABASE ..................... ON
> --  ENABLE_INTROSPECTION .................... ON
> --  ENABLE_JIT                                ON
> --  ENABLE_LLINT_C_LOOP ..................... OFF
> --  ENABLE_MEMORY_SAMPLER ................... ON
> --  ENABLE_MINIBROWSER                        ON
> --  ENABLE_PLUGIN_PROCESS_GTK2                ON
> --  ENABLE_SPELLCHECK                         ON
> --  ENABLE_SUBTLE_CRYPTO                      ON
> --  ENABLE_VIDEO ............................ ON
> --  ENABLE_WAYLAND_TARGET                     OFF
> --  ENABLE_WEB_AUDIO ........................ ON
> --  ENABLE_X11_TARGET ....................... ON
> --  USE_DEBUG_FISSION                         ON
> --  USE_LD_GOLD                               ON
> --  USE_LIBNOTIFY                             ON
> --  USE_OPENGL .............................. ON
> --  USE_SYSTEM_MALLOC                         OFF
> 
> Notes:
> 
> * I moved ENABLE_GLES2 to be private, since it's new. Is it still
> experimental, or is it good enough to make public? CC Victor for advice (hi!
> Don't think we've met!)

It's not new, nor experimental, it's a regression of the CMake switch and should be public.

[Carlos Garcia Campos]

(In reply to comment #27)
> (In reply to comment #26)
> > Comment on attachment 250593 [details]
> > Patch
> > 
> > Seems to have broken ENABLE_MEDIA_STREAM....
> 
> Seems like mediatream is enabled but your patch disables openwebrtc support.

As long as mediastream requires openwebrtc it should be disabled by default (but enable by build-webkit, ideally only when openwebrtc is found, I had to explicitly disable mediastream because I don't have openwebrtc)

[Michael Catanzaro]

(In reply to comment #27)
> Seems like mediatream is enabled but your patch disables openwebrtc support.

ENABLE_MEDIA_STREAM and USE_OPENWEBRTC are both disabled by default, but build-webkit (used by the bots) sets ENABLE_MEDIA_STREAM=ON for developer builds. It doesn't set USE_OPENWEBRTC because I didn't modify the settings used by build-webkit, presuming those to have already been correct, and because USE_OPENWEBRTC was not an option until this patch (it was just enabled or disabled depending on whether the development package was present) so build-webkit could not have set it. So the change on the bots is indeed that USE_OPENWEBRTC is now off.

Is USE_OPENWEBRTC intended to be required by ENABLE_MEDIA_STREAM? If so, I will remove the USE_OPENWEBRTC option that I added in this patch and error out if it's not found and ENABLE_MEDIA_STREAM=ON. If that's a bug and it's supposed to be optional, let's fix it in a different bug, then add USE_OPENWEBRTC to the cross-platform build options so that it can be set by the build-webkit script if it should continue to be enabled on the bots. My assumption was that USE_OPENWEBRTC only makes sense in combination with ENABLE_MEDIA_STREAM, but that ENABLE_MEDIA_STREAM can be used without USE_OPENWEBRTC.

(In reply to comment #28)
> This isn't right imho. For developer builds at least, if those libraries are
> present they should be used.

Those are the values for people using CMake directly. I think developers doing that should enable the options explicitly if wanted. Default settings for developers using build-webkit (including the bots) are in Tools/Scripts/webkitperl/FeatureList.pm. If we want any of those settings enabled for developers using CMake directly, we can make them conditional on DEVELOPER_MODE. The big advantage of doing that would be that you get the same settings using CMake directly as you do with build-webkit, since that was quite confusing when I was newer (I actually did not even notice FeatureList.pm for several months until Zan pointed it out to me). That is quite a big advantage TBH. But I'm opposed to doing that right now because then FeatureList.pm has no purpose and would just make everything even more confusing than it already is (now you have to specify your developer settings in two different places!), so if we do that we should delete FeatureList.pm (in coordination with Apple folks) to avoid that horrible mess. I think as the Apple CMake work matures, that will become a viable option. And we can improve our WEBKIT_SETTING macro to allow specifying different defaults for users and developers in the same place.

I left this comment above the point in the code where I set ENABLE_MINIBROWSER and ENABLE_API_TESTS conditional on the value of DEVELOPER_MODE:

# Think hard before addding more options here. Developers using CMake directly
# instead of build-webkit should get mostly the same build options as users.
# Instead, modify Tools/Scripts/webkitperl/FeatureList.pm to override default
# values for developers, e.g. to enable experimental features.

(In reply to comment #29)
> It's not new, nor experimental, it's a regression of the CMake switch and
> should be public.

OK, ENABLE_GLES2 should be public then.

[Martin Robinson]

(In reply to comment #31)
 
> Those are the values for people using CMake directly. I think developers
> doing that should enable the options explicitly if wanted. Default settings
> for developers using build-webkit (including the bots) are in
> Tools/Scripts/webkitperl/FeatureList.pm. If we want any of those settings
> enabled for developers using CMake directly, we can make them conditional on
> DEVELOPER_MODE. The big advantage of doing that would be that you get the
> same settings using CMake directly as you do with build-webkit, since that
> was quite confusing when I was newer (I actually did not even notice
> FeatureList.pm for several months until Zan pointed it out to me). That is
> quite a big advantage TBH. But I'm opposed to doing that right now because
> then FeatureList.pm has no purpose and would just make everything even more
> confusing than it already is (now you have to specify your developer
> settings in two different places!), so if we do that we should delete
> FeatureList.pm (in coordination with Apple folks) to avoid that horrible
> mess. I think as the Apple CMake work matures, that will become a viable
> option. And we can improve our WEBKIT_SETTING macro to allow specifying
> different defaults for users and developers in the same place.

The point of FeatureList.pm and the reason we cannot delete it, is that it sets the same options no matter what port or build system you are using. It knows how to tweak the XCode build, for instance. The only way we make these same settings conditional on DEVELOPER MODE is to duplicate the settings in the CMake build. I'm a little bit wary of that simply because I think they will get out of sync. Building a developer build without build-webkit is already "you are on your own" territory anyway.

[Carlos Garcia Campos]

(In reply to comment #31)
> (In reply to comment #27)
> > Seems like mediatream is enabled but your patch disables openwebrtc support.
> 
> ENABLE_MEDIA_STREAM and USE_OPENWEBRTC are both disabled by default, but
> build-webkit (used by the bots) sets ENABLE_MEDIA_STREAM=ON for developer
> builds. It doesn't set USE_OPENWEBRTC because I didn't modify the settings
> used by build-webkit, presuming those to have already been correct, and
> because USE_OPENWEBRTC was not an option until this patch (it was just
> enabled or disabled depending on whether the development package was
> present) so build-webkit could not have set it. So the change on the bots is
> indeed that USE_OPENWEBRTC is now off.
> 
> Is USE_OPENWEBRTC intended to be required by ENABLE_MEDIA_STREAM?

I'm afraid so.

> If so, I
> will remove the USE_OPENWEBRTC option that I added in this patch and error
> out if it's not found and ENABLE_MEDIA_STREAM=ON. If that's a bug and it's
> supposed to be optional, let's fix it in a different bug, then add
> USE_OPENWEBRTC to the cross-platform build options so that it can be set by
> the build-webkit script if it should continue to be enabled on the bots. My
> assumption was that USE_OPENWEBRTC only makes sense in combination with
> ENABLE_MEDIA_STREAM, but that ENABLE_MEDIA_STREAM can be used without
> USE_OPENWEBRTC.

That used to be the case, but not anymore.

[Víctor M. Jáquez L.]

(In reply to comment #29)
> (In reply to comment #21)
> > 
> > * I moved ENABLE_GLES2 to be private, since it's new. Is it still
> > experimental, or is it good enough to make public? CC Victor for advice (hi!
> > Don't think we've met!)
> 
> It's not new, nor experimental, it's a regression of the CMake switch and
> should be public.

Hi Michael! What Carlos says :3

[Michael Catanzaro]

Created attachment 250635 [details]
[GTK] Mark as advanced the build options we don't want to expose

[Philippe Normand]

Comment on attachment 250635 [details]
[GTK] Mark as advanced the build options we don't want to expose

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

> Source/cmake/OptionsGTK.cmake:130
> +WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL USE_OPENGL)

GSTREAMER_GL isn't limited to OpenGL, it can also work with GLES2.

[Michael Catanzaro]

(In reply to comment #36)
> GSTREAMER_GL isn't limited to OpenGL, it can also work with GLES2.

I think I have that right, but it is quite confusing... USE_GLES2 depends on USE_OPENGL because GLES is-a OpenGL, and USE_OPENGL toggles off all the other related options. USE_OPENGL without USE_GLES2 uses find_package(OpenGL) but with USE_GLES2 uses find_package(OpenGLES2). It should be the same as before.

[Carlos Garcia Campos]

Comment on attachment 250635 [details]
[GTK] Mark as advanced the build options we don't want to expose

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

> Source/cmake/OptionsGTK.cmake:65
> +find_package(X11)

Why do we find for X11 even if ENABLE_X11_TARGET is OFF?

> Source/cmake/OptionsGTK.cmake:76
> +# These features are public beacuse they aren't needed in embedded systems.

This is not true, they might not be needed in desktops envs too, I know desktop apps in kiosk mode disabling DRAG_SUPPORT, for example, or the opposite, apps in embedded systems using drag support. Note that text selections depend on drag support enabled.

> Source/cmake/OptionsGTK.cmake:80
> +# These features are public because they may be needed to build on non-x86.

This isn't accurate either, you might want to use system malloc to debug using valgrind, for example.

> Source/cmake/OptionsGTK.cmake:94
> +WEBKIT_OPTION_DEFINE_PUBLIC(USE_OPENGL "Whether to use OpenGL for hardware acceleration and WebGL" ON)

I think this new option should be added in a separate patch, because this one is about hiding existing options, not about adding new ones. But still, I think this should be ENABLE_OPENGL, USE_OPENGL is confusing because we have USE(OPENGL) in the code to refer to use OpenGL and not OpenGL ES2, and what we want here is an option to disable all features depending on OpenGL (WebGL, Accelerated compositing, etc.).

> Source/cmake/OptionsGTK.cmake:122
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_MPEGTS "Whether to enable support for MPEG-TS." OFF)
> +WEBKIT_OPTION_DEFINE(USE_GSTREAMER_GL "Whether to enable support for GStreamer GL." OFF)

I'm not sure whether ENABLE would be better than USE here as well, or maybe the description could be changed instead.

>> Source/cmake/OptionsGTK.cmake:130
>> +WEBKIT_OPTION_DEPEND(USE_GSTREAMER_GL USE_OPENGL)
> 
> GSTREAMER_GL isn't limited to OpenGL, it can also work with GLES2.

USE_OPENGL != WTF_USE_OPENGL, I said this was confusing :-)

> Source/cmake/OptionsGTK.cmake:223
> +if (USE_LIBNOTIFY)
> +    if (LIBNOTIFY_FOUND)

Why don't we find for libnotify here?

> Source/cmake/OptionsGTK.cmake:230
> +else ()
> +    add_definitions(-DWTF_USE_LIBNOTIFY=0)
> +endif ()

I know this was in current code, but I would just remove it.

> Source/cmake/OptionsGTK.cmake:248
> +if (ENABLE_GLES2)
> +    find_package(OpenGLES2 REQUIRED)
> +    if (NOT EGL_FOUND)
> +        message(FATAL_ERROR "EGL is needed for OpenGL ES 2.0.")
> +    endif ()
> +else ()
> +    find_package(OpenGL)
> +endif ()

Shouldn't this be done only is OPENGL id enabled

> Source/cmake/OptionsGTK.cmake:251
> +if (ENABLE_X11_TARGET)
> +    if (NOT X11_FOUND)

why don't we find for x11 here?

> Source/cmake/OptionsGTK.cmake:284
> +if (USE_OPENGL)
> +    if ((NOT OPENGL_FOUND AND NOT OPENGLES2_FOUND) OR (NOT GLX_FOUND AND NOT EGL_FOUND))

Why don't we find the required packages here?

> Source/cmake/OptionsGTK.cmake:374
>  if (ENABLE_GEOLOCATION)
> +    if (NOT GEOCLUE2_FOUND)

why don't we find geoclue2 here too?

> Source/cmake/OptionsGTK.cmake:-363
> -    add_definitions(-DENABLE_3D_RENDERING=1)

Why removing this?

[Carlos Garcia Campos]

(In reply to comment #37)
> (In reply to comment #36)
> > GSTREAMER_GL isn't limited to OpenGL, it can also work with GLES2.
> 
> I think I have that right, but it is quite confusing... USE_GLES2 depends on
> USE_OPENGL because GLES is-a OpenGL, and USE_OPENGL toggles off all the
> other related options. USE_OPENGL without USE_GLES2 uses
> find_package(OpenGL) but with USE_GLES2 uses find_package(OpenGLES2). It
> should be the same as before.

It's not the same because we don't have any USE_OPENGL cmake option.

[Michael Catanzaro]

(In reply to comment #38)
> Why do we find for X11 even if ENABLE_X11_TARGET is OFF?

Since we no longer use REQUIRED, it doesn't have to be in the conditional anymore. I guess I moved it to have as many of the find_package calls at the start of the file as possible. I don't have any preference where we put it, but since you do, I'll move it.

> This is not true, they might not be needed in desktops envs too, I know
> desktop apps in kiosk mode disabling DRAG_SUPPORT, for example, or the
> opposite, apps in embedded systems using drag support. Note that text
> selections depend on drag support enabled.

Yeah that was an oversimplification; I'll fix the comment.
 
> This isn't accurate either, you might want to use system malloc to debug
> using valgrind, for example.

OK, I'll change that comment too.
 
> I think this new option should be added in a separate patch, because this
> one is about hiding existing options, not about adding new ones. But still,
> I think this should be ENABLE_OPENGL, USE_OPENGL is confusing because we
> have USE(OPENGL) in the code to refer to use OpenGL and not OpenGL ES2, and
> what we want here is an option to disable all features depending on OpenGL
> (WebGL, Accelerated compositing, etc.).

OK, ENABLE_OPENGL it is. TBH it would be easier for me to do this all in the same patch, but if you really want it separately I will split it out. Please let me know. :)

> I'm not sure whether ENABLE would be better than USE here as well, or maybe
> the description could be changed instead.

I don't care either way. ENABLE_MPEGTS would work fine. I don't know what to name the ENABLE_ variant of USE_GSTREAMER_GL though.

> USE_OPENGL != WTF_USE_OPENGL, I said this was confusing :-)

OK, USE_OPENGL -> ENABLE_OPENGL. Still confusing, but less confusing.
 
> Why don't we find for libnotify here?

I'll move the check.

> I know this was in current code, but I would just remove it.

Yup, that doesn't belong.
 
> Shouldn't this be done only is OPENGL id enabled

ENABLE_GLES2 will be forced off if USE/ENABLE_OPENGL is off, by WEBKIT_OPTION_DEPENDS. I rely on that elsewhere as well, to simplify things a bit.

> why don't we find for x11 here?
> Why don't we find the required packages here?
> why don't we find geoclue2 here too?

I will move the checks.

> Why removing this?

It's a cross-platform option, so it's already in config.h.

(In reply to comment #39)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > GSTREAMER_GL isn't limited to OpenGL, it can also work with GLES2.
> > 
> > I think I have that right, but it is quite confusing... USE_GLES2 depends on
> > USE_OPENGL because GLES is-a OpenGL, and USE_OPENGL toggles off all the
> > other related options. USE_OPENGL without USE_GLES2 uses
> > find_package(OpenGL) but with USE_GLES2 uses find_package(OpenGLES2). It
> > should be the same as before.
> 
> It's not the same because we don't have any USE_OPENGL cmake option.

[Carlos Garcia Campos]

(In reply to comment #40)
> (In reply to comment #38)
> > Why do we find for X11 even if ENABLE_X11_TARGET is OFF?
> 
> Since we no longer use REQUIRED, it doesn't have to be in the conditional
> anymore. I guess I moved it to have as many of the find_package calls at the
> start of the file as possible. I don't have any preference where we put it,
> but since you do, I'll move it.

I guess we can avoid looking for deps we don't need when options are explicitly disabled no?

> > This is not true, they might not be needed in desktops envs too, I know
> > desktop apps in kiosk mode disabling DRAG_SUPPORT, for example, or the
> > opposite, apps in embedded systems using drag support. Note that text
> > selections depend on drag support enabled.
> 
> Yeah that was an oversimplification; I'll fix the comment.
>  
> > This isn't accurate either, you might want to use system malloc to debug
> > using valgrind, for example.
> 
> OK, I'll change that comment too.
>  
> > I think this new option should be added in a separate patch, because this
> > one is about hiding existing options, not about adding new ones. But still,
> > I think this should be ENABLE_OPENGL, USE_OPENGL is confusing because we
> > have USE(OPENGL) in the code to refer to use OpenGL and not OpenGL ES2, and
> > what we want here is an option to disable all features depending on OpenGL
> > (WebGL, Accelerated compositing, etc.).
> 
> OK, ENABLE_OPENGL it is. TBH it would be easier for me to do this all in the
> same patch, but if you really want it separately I will split it out. Please
> let me know. :)

This patch changes a lot of things, and claims to be marking as advanced options we don't want to expose, so adding new options only makes it more difficult to review. I would add the new option in a separate patch, either after or before this one. That way we can discuss it separately instead of adding it hidden inside a big patch.

> > I'm not sure whether ENABLE would be better than USE here as well, or maybe
> > the description could be changed instead.
> 
> I don't care either way. ENABLE_MPEGTS would work fine. I don't know what to
> name the ENABLE_ variant of USE_GSTREAMER_GL though.
> 
> > USE_OPENGL != WTF_USE_OPENGL, I said this was confusing :-)
> 
> OK, USE_OPENGL -> ENABLE_OPENGL. Still confusing, but less confusing.
>  
> > Why don't we find for libnotify here?
> 
> I'll move the check.
> 
> > I know this was in current code, but I would just remove it.
> 
> Yup, that doesn't belong.
>  
> > Shouldn't this be done only is OPENGL id enabled
> 
> ENABLE_GLES2 will be forced off if USE/ENABLE_OPENGL is off, by
> WEBKIT_OPTION_DEPENDS. I rely on that elsewhere as well, to simplify things
> a bit.

But we have a if (USE_OPENGL) below, so this could be moved inside that block.

> > why don't we find for x11 here?
> > Why don't we find the required packages here?
> > why don't we find geoclue2 here too?
> 
> I will move the checks.
> 
> > Why removing this?
> 
> It's a cross-platform option, so it's already in config.h.
>

[Michael Catanzaro]

(In reply to comment #41) 
> I guess we can avoid looking for deps we don't need when options are
> explicitly disabled no?

Yeah. Also, there's no good reason to have the search for deps far away from where the result is used.

> This patch changes a lot of things, and claims to be marking as advanced
> options we don't want to expose, so adding new options only makes it more
> difficult to review. I would add the new option in a separate patch, either
> after or before this one. That way we can discuss it separately instead of
> adding it hidden inside a big patch.

OK :(

> But we have a if (USE_OPENGL) below, so this could be moved inside that
> block.

We can't: it has to be done before we check for GLX, which is done inside an ENABLE_X11_TARGET conditional, and it has to be done unconditionally. So we could add a new conditional around it, but it's not needed.

[Michael Catanzaro]

This patch does nothing except change options from PUBLIC to PRIVATE. List of options:

-- Enabled features:
--  ENABLE_API_TESTS ........................ OFF
--  ENABLE_CREDENTIAL_STORAGE                 ON
--  ENABLE_DRAG_SUPPORT ..................... ON
--  ENABLE_GEOLOCATION                        ON
--  ENABLE_MEMORY_SAMPLER ................... ON
--  ENABLE_MINIBROWSER                        OFF
--  ENABLE_PLUGIN_PROCESS_GTK2 .............. ON
--  ENABLE_SPELLCHECK                         ON
--  ENABLE_SUBTLE_CRYPTO .................... ON
--  ENABLE_TOUCH_EVENTS                       ON
--  ENABLE_VIDEO ............................ ON
--  ENABLE_WEB_AUDIO                          ON

The rest of the changes from my original patch (including exposing more options like ENABLE_X11_TARGET etc.) are removed to ease review, and can be handled in bug #143546, bug #143640, bug #144106, bug #144105, etc.

[Michael Catanzaro]

Created attachment 251453 [details]
Patch

[Michael Catanzaro]

Created attachment 251473 [details]
Patch

[Michael Catanzaro]

Comment on attachment 251473 [details]
Patch

Clearing flags on attachment: 251473

Committed r183205: <http://trac.webkit.org/changeset/183205>

[Michael Catanzaro]

All reviewed patches have been landed.  Closing bug.