Bug 83313 – [CSS Exclusions] Add flag to enable / disable exclusions at runtime

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Bear Travis]

Add a flag similar to those implemented for shaders (bug 76444) and regions (bug 78525).
Exclusions will be enabled by default.

The only existing exclusions work that needs the flag is CSS parsing. The rest will be future work.

[Bear Travis]

Created attachment 136053 [details]
Prototype Patch

A prototype patch for adding the runtime flag. Not yet cleaned up enough for review.
May wind up removing some of the chromium modifications for a later patch.

[WebKit Review Bot]

Comment on attachment 136053 [details]
Prototype Patch

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

[Bear Travis]

Created attachment 136084 [details]
Updated patch

Modified the patch to skip the test in webkit2, and removed the WebKit/chromium work.

[WebKit Review Bot]

Comment on attachment 136084 [details]
Updated patch

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

New failing tests:
fast/exclusions/css-exclusions-disabled.html

[WebKit Review Bot]

Created attachment 136124 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Bear Travis]

Created attachment 136277 [details]
updated patch with chromium flag

adding the flag to the chromium platform, which should fix the cr-linux failure

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

[James Robinson]

Comment on attachment 136277 [details]
updated patch with chromium flag

Chromium WebKit API changes look good to me.

[Bear Travis]

Comment on attachment 136277 [details]
updated patch with chromium flag

Going to start looking for a reviewer.

[Alexis Menard (darktears)]

What is the use case with these runtime flags?

Because it makes harder to change the CSSParser code as we need to check this extra flags and it's not static data because it's at runtime.

[Bear Travis]

(In reply to comment #10)
> What is the use case with these runtime flags?
> 
The runtime flag enables us to keep the feature in the build, even when it is disabled by default. This enables consistent testing of the feature, and also the eventual exposure of the feature to end users for experimentation through something like a chromium flag.

This is similar to how shaders (bug 76444) and regions (bug 78525) are currently exposed.

[Darin Fisher (:fishd, Google)]

Why does the feature need to be enabled/disabled on a page-by-page basis?  Why not control CSS features globally as we do DOM features?  See RuntimeEnabledFeatures.cpp:
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp&exact_package=chromium&q=RuntimeEnabledFeatures.cpp&type=cs

[Bear Travis]

(In reply to comment #12)
> Why does the feature need to be enabled/disabled on a page-by-page basis?  Why not control CSS features globally as we do DOM features? 

I don't see why the css features couldn't be enabled globally via RuntimeEnabledFeatures. The code is in its current location only because that's where other similar flags are implemented (regions and shaders). I've filed a bug to move them over (bug 83626). Does it seem reasonable to land this patch and then move them all over together, or implement this flag using RuntimeEnabledFeatures and then move the others over?

[Alexis Menard (darktears)]

(In reply to comment #13)
> (In reply to comment #12)
> > Why does the feature need to be enabled/disabled on a page-by-page basis?  Why not control CSS features globally as we do DOM features? 
> 
> I don't see why the css features couldn't be enabled globally via RuntimeEnabledFeatures. The code is in its current location only because that's where other similar flags are implemented (regions and shaders). I've filed a bug to move them over (bug 83626). Does it seem reasonable to land this patch and then move them all over together, or implement this flag using RuntimeEnabledFeatures and then move the others over?

It's better to land good code directly :)

[Bear Travis]

Comment on attachment 136277 [details]
updated patch with chromium flag

Investigating a better method of implementing the runtime flag using RuntimeEnabledFeatures.

[Bear Travis]

Created attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Running EWS to get the necessary symbols

[Build Bot]

Comment on attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Attachment 136583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12380609

[Build Bot]

Comment on attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Attachment 136583 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12381573

[Bear Travis]

Created attachment 136598 [details]
Updated runtime enabled patch

Fixing the compile errors. Still need to run for the internal symbols.

[Philippe Normand]

Comment on attachment 136598 [details]
Updated runtime enabled patch

Attachment 136598 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12384565

[Build Bot]

Comment on attachment 136598 [details]
Updated runtime enabled patch

Attachment 136598 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12386168

[Bear Travis]

Created attachment 136622 [details]
runtime enabled patch with appropriate symbols

added the symbols for win and gtk

[Bear Travis]

Comment on attachment 136622 [details]
runtime enabled patch with appropriate symbols

Patch is ready for review. Will look for a reviewer tomorrow.

[Alexis Menard (darktears)]

Comment on attachment 136622 [details]
runtime enabled patch with appropriate symbols

I'm wondering if instead of cluttering CSSParser::parseValue with these runtime flags checks all different if we could have some internal function like :
RuntimeEnabledFeatures::isCSSFeatureEnabled(CSSPropertyID) (maybe not in RuntimeEnabledFeatures) so that at the beginning of CSSParser::parseValue you can bail out if not activated and you have one function call rather than cssExclusionsEnabled() cssRegionsEnabled() at multiple places. You can see that we have already 3 cases and maybe more so maybe we could have some generic function in here. The function could have a switch case for the property ids we have runtime flags and will call RuntimeEnabledFeatures::XXXXenabled and the default would be to return true (always enable for the rest). What do you think?

[Bear Travis]

(In reply to comment #24)

I think it's an interesting idea. I could see adding a method CSSParser::isPropertyEnabled(propertyID) to do an upfront check in parseValue. I would keep the current is[Feature]Enabled methods because they are used elsewhere in the code (especially in layout). Unfortunately, we can't tell whether to reject certain CSS properties until we start parsing, as it's the value that is restricted (as is the case with custom filters).

The only two property sets we could pull out are those for regions and exclusions. I'm not sure its worth checking all calls to parseValue up front, given these are the only two we can do. If it is worth doing, I would suggest moving only exclusions properties in this patch, and regions properties in a subsequent patch.

[Bear Travis]

Created attachment 136720 [details]
alternate patch, checking property enabled in parseValue

Patch demonstrating the isCSSPropertyEnabled approach in parseValue.
A subsequent patch could factor out the checks for css regions as well.

[Bear Travis]

Created attachment 136749 [details]
runtime enabled patch, defaulting to exclusions disabled

changing exclusions to default to disabled, per kling's feedback

[Ryosuke Niwa]

Comment on attachment 136749 [details]
runtime enabled patch, defaulting to exclusions disabled

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

> LayoutTests/fast/exclusions/css-exclusions-disabled.html:10
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<script src="../../fast/js/resources/js-test-pre.js"></script>
> +</head>
> +<body>
> +<script src="script-tests/css-exclusions-disabled.js"></script>
> +<script src="../../fast/js/resources/js-test-post.js"></script>
> +</body>
> +</html>

Please use new style script tests: http://trac.webkit.org/browser/trunk/Tools/Scripts/make-new-script-test

[Bear Travis]

Created attachment 136791 [details]
updated patch with new-style script test

Updating the added test to use the new-style script tests

[Ryosuke Niwa]

Comment on attachment 136791 [details]
updated patch with new-style script test

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

> LayoutTests/fast/exclusions/css-exclusions-disabled.html:6
> +<head>
> +<meta charset="utf-8">
> +<script src="../js/resources/js-test-pre.js"></script>
> +</head>

We don't need to specify the charset here, right? You can just get rid of the head and move the script element to body.

> LayoutTests/fast/exclusions/css-exclusions-disabled.html:40
> +// wrap-flow

These one-line comments are pure noise since they repat the code below them.

> Source/WebCore/css/CSSParser.cpp:464
> +    if ((propertyId == CSSPropertyWebkitWrapMargin || propertyId == CSSPropertyWebkitWrapPadding)
> +        && !RuntimeEnabledFeatures::cssExclusionsEnabled())
> +        return false;
> +

Can't we just modify CSSParser::parseValue instead?

[Bear Travis]

Created attachment 136828 [details]
updated patch

removing extra content in script test
moving property testing from parseSimpleLengthValue to isSimpleLengthValue to limit performance hit to exclusions properties

[Ryosuke Niwa]

Comment on attachment 136828 [details]
updated patch

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

> LayoutTests/fast/exclusions/css-exclusions-disabled.html:20
> +    return div.style[toCamelCase(property)];

Can't you just use getPropertyValue?

[Bear Travis]

Created attachment 136931 [details]
patch with alternate test syntax

Patch using style.getPropertyValue(propertyName) instead of style[camelCase(propertyName)]

The original exclusions tests used the following syntax, with -webkit-wrap-margin being an example property

Inline style
setting: div.setAttribute("style", "-webkit-wrap-margin: 10px")
getting: div.style.webkitWrapMargin

Computed style
setting: div.style.setProperty("-webkit-wrap-margin", "10px")
getting: getComputedStyle(div).getPropertyValue("-webkit-wrap-margin")

That's where that came from, but I don't mind just using getPropertyValue here either.

[Bear Travis]

Comment on attachment 136931 [details]
patch with alternate test syntax

Marking the second patch for review. The only difference between the two is how style properties are accessed in the test case.

[WebKit Review Bot]

Comment on attachment 136931 [details]
patch with alternate test syntax

Clearing flags on attachment: 136931

Committed r114020: <http://trac.webkit.org/changeset/114020>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.