Bug 101192 – [CSSRegions] when WebKit uses V8, there should be a single variable to store if the CSS Regions feature is enabled

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mihai Maerean]

WebCore/page/Settings.h has a bit called m_cssRegionsEnabled that stores whether the CSS Regions feature is enabled at runtime.

When WebKit uses V8, there's also WebCore/bindings/generic/RuntimeEnabledFeatures that has a static bool isCSSRegionsEnabled.

Expected:
There should be a single place to store if CSS Regions is enabled.

[Mihai Maerean]

Created attachment 173595 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hajime Morrita]

Comment on attachment 173595 [details]
Patch

Looks good. Please ask tkent for a rubberstamp.

[Peter Beverloo (cr-android ews)]

Comment on attachment 173595 [details]
Patch

Attachment 173595 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14804575

[WebKit Review Bot]

Comment on attachment 173595 [details]
Patch

Attachment 173595 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14818161

[Adam Barth]

Comment on attachment 173595 [details]
Patch

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

This isn't how we normally handle these cases.  Why not just have Chromium use WebSettings to enable Regions?

> Source/WebCore/page/Settings.cpp:218
> -#if ENABLE(CSS_REGIONS)
> +#if ENABLE(CSS_REGIONS) && !USE(V8)

We generally don't have USE(V8) ifdefs in Settings.cpp....

[Kent Tamura]

Comment on attachment 173595 [details]
Patch

Do you want to add V8EnabledAtRuntime to CSSRegions-related IDL attributes?

[Mihai Maerean]

(In reply to comment #7)
> (From update of attachment 173595 [details])
> Do you want to add V8EnabledAtRuntime to CSSRegions-related IDL attributes?

Yes, I will add this kind of changes in the IDL files (Element, Document etc):
-    [Conditional=CSS_REGIONS] readonly attribute DOMString webkitRegionOverset;
-    [Conditional=CSS_REGIONS] sequence<Range> webkitGetRegionFlowRanges();
+    [Conditional=CSS_REGIONS, V8EnabledAtRuntime=cssRegions] readonly attribute DOMString webkitRegionOverset;
+    [Conditional=CSS_REGIONS, V8EnabledAtRuntime=cssRegions] sequence<Range> webkitGetRegionFlowRanges();

in order to fix https://bugs.webkit.org/show_bug.cgi?id=94070 [CSSRegions] broken feature detection: syntax parsing succeeds when ENABLE_CSS_REGIONS is false

[Kent Tamura]

ok.  So, the patch shouldn't have Settings.{cpp.h} changes.  JSC platforms can't have a runtime flag for CSS Regions.

[Mihai Maerean]

(In reply to comment #9)
> ok.  So, the patch shouldn't have Settings.{cpp.h} changes.  JSC platforms can't have a runtime flag for CSS Regions.

While developing the CSS Regions feature, it is needed that the CSS Regions code is built for the build bots of the JSC platforms so that the code doesn't get broken accidentally.

The JSC platforms must have a way to disable the CSS Regions code at runtime. This is the purpose of the cssRegionsEnabled property inside Settings (which is set to False by default).

For the platforms (like Chromium) that have WebRuntimeFeatures and RuntimeEnabledFeatures, there must be a property for the CSS Regions feature inside RuntimeEnabledFeatures like for the other features that are present in RuntimeEnabledFeatures. This way, the feature can be enabled by using the "experimental WebKit features" switch in chrome://flags/.

For Chromium, there would be 2 variables (one in Settings and one static variable in RuntimeEnabledFeatures)  storing whether the CSS Regions should be active at run-time. These 2 variables can become out of sync and cause bugs.

This bug (101192) is meant to fix this by changing Settings to point to RuntimeEnabledFeatures when RuntimeEnabledFeatures is available.

This is why there are changes in Settings.{cpp.h}. 

My question is what should we use instead of USE(V8) since that is not desired (see comment #6) and also because it doesn't build on cr-android?

[Kent Tamura]

I don't think introducing two flags is a correct solution.

How to implement the runtime switch for JSC?  I think a straight-forward way is to implement EnabledAtRuntime IDL extended attribute for JSC.

[Mihai Maerean]

(In reply to comment #6)
> Why not just have Chromium use WebSettings to enable Regions?

chromium/src/content/renderer/render_thread_impl.cc uses WebRuntimeFeatures and RuntimeEnabledFeatures to enable similar features:

void RenderThreadImpl::EnsureWebKitInitialized() {
...
  WebRuntimeFeatures::enableStyleScoped(command_line.HasSwitch(switches::kEnableExperimentalWebKitFeatures));
  WebRuntimeFeatures::enableCSSExclusions(command_line.HasSwitch(switches::kEnableExperimentalWebKitFeatures));

The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.

Using WebSettings would be nice because it would have a direct connection with WebCore::Settings.

Unfortunately, there is no mention of WebSettings inside chromium/src/content/renderer/render_thread_impl.cc .

How should I get access to WebSettings from inside render_thread_impl.cc?

[Adam Barth]

> The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.

That's fine, but then you shouldn't use WebCore::Settings.  The problem is that you're trying to use two different mechanisms to control whether the feature is enabled.  You need to pick one and use that.

> How should I get access to WebSettings from inside render_thread_impl.cc?

You can't.  WebSettings is a per WebView concept.  render_thread_impl is shared across all the WebViews in a given process.  Try grepping around for uses of WebSettings in Chromium and you'll see how to do it.

[Mihai Maerean]

(In reply to comment #13)
> > The idea was to use switches::kEnableExperimentalWebKitFeatures in a way that is similar to how it has been used before.
> 
> That's fine, but then you shouldn't use WebCore::Settings.  The problem is that you're trying to use two different mechanisms to control whether the feature is enabled.  You need to pick one and use that.

We are forced to use WebRuntimeFeatures and RuntimeEnabledFeatures in order to stop the feature from leaking information at run-time by adding V8EnabledAtRuntime=cssRegions in the IDL files.

On the other hand, we are forced to use the bit in Settings so that the JSC platforms build the code (so people don't accidentally break it) and also keep the feature disabled at run-time.

So we can't remove any on them.

The solution that I see is to have them both and hopefully the 2 variables won't get out of sync as the webkit/chromium code evolves over time.

Is this acceptable?

[James Robinson]

(In reply to comment #10)
> The JSC platforms must have a way to disable the CSS Regions code at runtime.

Are you sure about this?

[Adam Barth]

> Is this acceptable?

It sounds like the Settings approach doesn't really work for JSC either.  If it worked for JSC, then you could use it for V8 as well.  The ability to turn this feature off at runtime for JSC is half-baked.  IMHO, you should either fully bake it (likely by implementing EnabledAtRuntime for JSC) or you should remove it (e.g., and use ENABLE macros to enable/disable the feature for JSC).

[Mihai Maerean]

Created attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Work in Progress.

[WebKit Review Bot]

Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Attachment 176456 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15018420

[Mihai Maerean]

(In reply to comment #18)
> (From update of attachment 176456 [details])
> Attachment 176456 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/15018420

My changes did include the fix for Chromium, but the changes are located in Source/WebKit/chromium/webkit/glue/ (brought by update-webkit --chromium) and aren't picked up by webkit-patch or git diff.

I have googled for "how to submit patch affects both webkit chromium glue code", but I didn't find the answer.

So how do I add the changes found in Source/WebKit/chromium/webkit/glue/ to the patch so the chromium-ews can successfully build the sources?

[Adam Barth]

> So how do I add the changes found in Source/WebKit/chromium/webkit/glue/ to the patch so the chromium-ews can successfully build the sources?

webkit/glue is located in svn.chromium.org:

http://src.chromium.org/viewvc/chrome/trunk/src/webkit/glue/

You would first need to land the changes in svn.chromium.org and then increase the value of chromium_rev in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS so that WebKit can see your change to Chromium.

[Adam Barth]

Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

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

> Source/WebKit/chromium/public/WebSettings.h:-104
> -    virtual void setExperimentalCSSRegionsEnabled(bool) = 0;

I suspect the problem is that you're trying to change this public API.  What you should do instead is move this function to the bottom of the interface and make it a no-op.  After this patch lands and Chromium's webkit_rev is increased, you can remove any callers in Chromium.  Once the Chromium callers are gone and WebKit's chromium_rev is increased, you can remove the deprecated API.

[Peter Beverloo (cr-android ews)]

Comment on attachment 176456 [details]
Incorporated Adam Barth's feedback to remove the CSS Regions flag in Settings and only use the new flag in RuntimeEnabledFeatures.

Attachment 176456 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15016417

[Mihai Maerean]

Created attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

[Hajime Morrita]

Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

rs=me, all green, adam's comments being addressed.

[WebKit Review Bot]

Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

Rejecting attachment 176486 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 176486 [details]
Incorporated Adam Barth's feedback to move setExperimentalCSSRegionsEnabled to the bottom of the WebSettings interface and make it a no-op.

Clearing flags on attachment: 176486

Committed r136210: <http://trac.webkit.org/changeset/136210>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.