Bug 83313 - [CSS Exclusions] Add flag to enable / disable exclusions at runtime
: [CSS Exclusions] Add flag to enable / disable exclusions at runtime
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
: http://dev.w3.org/csswg/css3-exclusions/
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-05 14:21 PST by
Modified: 2012-04-12 12:54 PST (History)


Attachments
Prototype Patch (29.02 KB, patch)
2012-04-06 12:28 PST, Bear Travis
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (24.41 KB, patch)
2012-04-06 16:01 PST, Bear Travis
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (6.49 MB, application/zip)
2012-04-06 21:39 PST, WebKit Review Bot
no flags Details
updated patch with chromium flag (29.92 KB, patch)
2012-04-09 11:55 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
Patch using RuntimeEnabledFeatures (14.64 KB, patch)
2012-04-10 17:39 PST, Bear Travis
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated runtime enabled patch (14.08 KB, patch)
2012-04-10 18:24 PST, Bear Travis
pnormand: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
runtime enabled patch with appropriate symbols (17.80 KB, patch)
2012-04-10 22:32 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
alternate patch, checking property enabled in parseValue (16.85 KB, patch)
2012-04-11 12:07 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
runtime enabled patch, defaulting to exclusions disabled (22.17 KB, patch)
2012-04-11 14:07 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
updated patch with new-style script test (21.61 KB, patch)
2012-04-11 17:11 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (21.51 KB, patch)
2012-04-11 23:11 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff
patch with alternate test syntax (21.25 KB, patch)
2012-04-12 10:50 PST, Bear Travis
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-05 14:21:31 PST
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.
------- Comment #1 From 2012-04-06 12:28:38 PST -------
Created an attachment (id=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.
------- Comment #2 From 2012-04-06 13:19:59 PST -------
(From update of attachment 136053 [details])
Attachment 136053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12359056
------- Comment #3 From 2012-04-06 16:01:43 PST -------
Created an attachment (id=136084) [details]
Updated patch

Modified the patch to skip the test in webkit2, and removed the WebKit/chromium work.
------- Comment #4 From 2012-04-06 21:39:10 PST -------
(From update of attachment 136084 [details])
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
------- Comment #5 From 2012-04-06 21:39:17 PST -------
Created an attachment (id=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
------- Comment #6 From 2012-04-09 11:55:23 PST -------
Created an attachment (id=136277) [details]
updated patch with chromium flag

adding the flag to the chromium platform, which should fix the cr-linux failure
------- Comment #7 From 2012-04-09 11:59:38 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #8 From 2012-04-09 12:19:45 PST -------
(From update of attachment 136277 [details])
Chromium WebKit API changes look good to me.
------- Comment #9 From 2012-04-10 10:22:51 PST -------
(From update of attachment 136277 [details])
Going to start looking for a reviewer.
------- Comment #10 From 2012-04-10 10:37:42 PST -------
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.
------- Comment #11 From 2012-04-10 11:03:28 PST -------
(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.
------- Comment #12 From 2012-04-10 13:42:06 PST -------
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
------- Comment #13 From 2012-04-10 15:20:27 PST -------
(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?
------- Comment #14 From 2012-04-10 15:22:42 PST -------
(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 :)
------- Comment #15 From 2012-04-10 15:31:41 PST -------
(From update of attachment 136277 [details])
Investigating a better method of implementing the runtime flag using RuntimeEnabledFeatures.
------- Comment #16 From 2012-04-10 17:39:34 PST -------
Created an attachment (id=136583) [details]
Patch using RuntimeEnabledFeatures

Running EWS to get the necessary symbols
------- Comment #17 From 2012-04-10 18:01:50 PST -------
(From update of attachment 136583 [details])
Attachment 136583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12380609
------- Comment #18 From 2012-04-10 18:03:01 PST -------
(From update of attachment 136583 [details])
Attachment 136583 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12381573
------- Comment #19 From 2012-04-10 18:24:54 PST -------
Created an attachment (id=136598) [details]
Updated runtime enabled patch

Fixing the compile errors. Still need to run for the internal symbols.
------- Comment #20 From 2012-04-10 19:48:54 PST -------
(From update of attachment 136598 [details])
Attachment 136598 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12384565
------- Comment #21 From 2012-04-10 20:56:55 PST -------
(From update of attachment 136598 [details])
Attachment 136598 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12386168
------- Comment #22 From 2012-04-10 22:32:46 PST -------
Created an attachment (id=136622) [details]
runtime enabled patch with appropriate symbols

added the symbols for win and gtk
------- Comment #23 From 2012-04-11 00:24:56 PST -------
(From update of attachment 136622 [details])
Patch is ready for review. Will look for a reviewer tomorrow.
------- Comment #24 From 2012-04-11 03:50:10 PST -------
(From update of attachment 136622 [details])
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?
------- Comment #25 From 2012-04-11 10:31:42 PST -------
(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.
------- Comment #26 From 2012-04-11 12:07:47 PST -------
Created an attachment (id=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.
------- Comment #27 From 2012-04-11 14:07:56 PST -------
Created an attachment (id=136749) [details]
runtime enabled patch, defaulting to exclusions disabled

changing exclusions to default to disabled, per kling's feedback
------- Comment #28 From 2012-04-11 16:37:39 PST -------
(From update of attachment 136749 [details])
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
------- Comment #29 From 2012-04-11 17:11:54 PST -------
Created an attachment (id=136791) [details]
updated patch with new-style script test

Updating the added test to use the new-style script tests
------- Comment #30 From 2012-04-11 18:02:15 PST -------
(From update of attachment 136791 [details])
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?
------- Comment #31 From 2012-04-11 23:11:24 PST -------
Created an attachment (id=136828) [details]
updated patch

removing extra content in script test
moving property testing from parseSimpleLengthValue to isSimpleLengthValue to limit performance hit to exclusions properties
------- Comment #32 From 2012-04-12 02:30:40 PST -------
(From update of attachment 136828 [details])
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?
------- Comment #33 From 2012-04-12 10:50:51 PST -------
Created an attachment (id=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.
------- Comment #34 From 2012-04-12 12:10:05 PST -------
(From update of attachment 136931 [details])
Marking the second patch for review. The only difference between the two is how style properties are accessed in the test case.
------- Comment #35 From 2012-04-12 12:54:40 PST -------
(From update of attachment 136931 [details])
Clearing flags on attachment: 136931

Committed r114020: <http://trac.webkit.org/changeset/114020>
------- Comment #36 From 2012-04-12 12:54:56 PST -------
All reviewed patches have been landed.  Closing bug.