Bug 116043

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

Created attachment 201581 [details] Patch

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

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.

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?

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

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?

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

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

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

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