Bug 116043 – [GTK][CSS Regions] Provide web setting to control CSS regions run time

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Anton Obzhirov]

Provide web setting to control CSS regions run time

[Anton Obzhirov]

Needed to add run time enabling/disabling of CSS regions for GTK port in order to play with CSS regions.

[Anton Obzhirov]

Created attachment 201581 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Martin Robinson]

I wonder how useful it is make this configurable... Perhaps we should just have it so that when it's turned on at compile-time, it's enabled at runtime. At some point in the future it won't make sense to have support for disabling CSS regions in the API. It's even a liability since the setting may go away some day.

[Carlos Garcia Campos]

Comment on attachment 201581 [details]
Patch

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

Thanks for the patch. I'm not sure if we want to add new API to WebKit1 which is currently in maintenance mode. I would split the patch into one for wk1 and one for wk2 in any case so that it can be discussed separately.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:34
> +#include "RuntimeEnabledFeatures.h"

Please use  angle-bracket form for WebCore header file includes.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1113
> +     * Since: 2.0

This should be 2.2, 2.0 was already released.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1120
> +            TRUE,

I don't think we want this to be enabled by default, at least not for now.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2753
> + */

Since 2.2.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2767
> + */

Since 2.2.

> Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2779
> +#if ENABLE(CSS_REGIONS)
> +    WebCore::RuntimeEnabledFeatures::setCSSRegionsEnabled(enabled);
> +#endif

Do we really need to do this in the UI process too?

[Martin Robinson]

(In reply to comment #5)

> I don't think we want this to be enabled by default, at least not for now.

This isn't a huge liability since we don't turn it on in release builds. I'm pretty sure this shouldn't even be configurable in the long-term.  CSS features are generally not turned on and off at runtime. This setting is useful for other browsers, because they can ship a stable release and still allow developers to play with the setting. They also don't have to worry about API stability as much (notice that this is a private API in WebKit2).  Given that, I'm fairly confidant we should not expose this and instead, enable CSS regions when they are turned on at compile-time.

[Gustavo Noronha (kov)]

Agree with Martin. If we want to allow experimentation with features such as this one we should probably use a way that does not impact the API for each feature. Perhaps an environment variable that can be set with feature names, for instance?

[Martin Robinson]

(In reply to comment #7)
> Agree with Martin. If we want to allow experimentation with features such as this one we should probably use a way that does not impact the API for each feature. Perhaps an environment variable that can be set with feature names, for instance?

I'd feel more comfortable with that, for sure. :)

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > Agree with Martin. If we want to allow experimentation with features such as this one we should probably use a way that does not impact the API for each feature. Perhaps an environment variable that can be set with feature names, for instance?
> 
> I'd feel more comfortable with that, for sure. :)

Agree, the public API is not the right the place for this temporary convenient settings to play with experimental features.

[Anton Obzhirov]

In this case I would suggest just to keep compilation time flag to disable/enable it: --enable-css-regions.

[Anton Obzhirov]

After talking to Gustavo on IRC it doesn't make sense to proceed with this patch any longer because ENABLE_CSS_REGIONS already can be enabled/disabled in WebKitFeatures