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
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Bear Travis
http://dev.w3.org/csswg/css3-exclusions/
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-05 14:21 PDT by Bear Travis
Modified: 2012-04-12 12:54 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2012-04-05 14:21:31 PDT
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 Bear Travis 2012-04-06 12:28:38 PDT
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.
Comment 2 WebKit Review Bot 2012-04-06 13:19:59 PDT
Comment on attachment 136053 [details]
Prototype Patch

Attachment 136053 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12359056
Comment 3 Bear Travis 2012-04-06 16:01:43 PDT
Created attachment 136084 [details]
Updated patch

Modified the patch to skip the test in webkit2, and removed the WebKit/chromium work.
Comment 4 WebKit Review Bot 2012-04-06 21:39:10 PDT
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
Comment 5 WebKit Review Bot 2012-04-06 21:39:17 PDT
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
Comment 6 Bear Travis 2012-04-09 11:55:23 PDT
Created attachment 136277 [details]
updated patch with chromium flag

adding the flag to the chromium platform, which should fix the cr-linux failure
Comment 7 WebKit Review Bot 2012-04-09 11:59:38 PDT
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 James Robinson 2012-04-09 12:19:45 PDT
Comment on attachment 136277 [details]
updated patch with chromium flag

Chromium WebKit API changes look good to me.
Comment 9 Bear Travis 2012-04-10 10:22:51 PDT
Comment on attachment 136277 [details]
updated patch with chromium flag

Going to start looking for a reviewer.
Comment 10 Alexis Menard (darktears) 2012-04-10 10:37:42 PDT
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 Bear Travis 2012-04-10 11:03:28 PDT
(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 Darin Fisher (:fishd, Google) 2012-04-10 13:42:06 PDT
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 Bear Travis 2012-04-10 15:20:27 PDT
(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 Alexis Menard (darktears) 2012-04-10 15:22:42 PDT
(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 Bear Travis 2012-04-10 15:31:41 PDT
Comment on attachment 136277 [details]
updated patch with chromium flag

Investigating a better method of implementing the runtime flag using RuntimeEnabledFeatures.
Comment 16 Bear Travis 2012-04-10 17:39:34 PDT
Created attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Running EWS to get the necessary symbols
Comment 17 Build Bot 2012-04-10 18:01:50 PDT
Comment on attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Attachment 136583 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12380609
Comment 18 Build Bot 2012-04-10 18:03:01 PDT
Comment on attachment 136583 [details]
Patch using RuntimeEnabledFeatures

Attachment 136583 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12381573
Comment 19 Bear Travis 2012-04-10 18:24:54 PDT
Created attachment 136598 [details]
Updated runtime enabled patch

Fixing the compile errors. Still need to run for the internal symbols.
Comment 20 Philippe Normand 2012-04-10 19:48:54 PDT
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
Comment 21 Build Bot 2012-04-10 20:56:55 PDT
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
Comment 22 Bear Travis 2012-04-10 22:32:46 PDT
Created attachment 136622 [details]
runtime enabled patch with appropriate symbols

added the symbols for win and gtk
Comment 23 Bear Travis 2012-04-11 00:24:56 PDT
Comment on attachment 136622 [details]
runtime enabled patch with appropriate symbols

Patch is ready for review. Will look for a reviewer tomorrow.
Comment 24 Alexis Menard (darktears) 2012-04-11 03:50:10 PDT
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?
Comment 25 Bear Travis 2012-04-11 10:31:42 PDT
(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 Bear Travis 2012-04-11 12:07:47 PDT
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.
Comment 27 Bear Travis 2012-04-11 14:07:56 PDT
Created attachment 136749 [details]
runtime enabled patch, defaulting to exclusions disabled

changing exclusions to default to disabled, per kling's feedback
Comment 28 Ryosuke Niwa 2012-04-11 16:37:39 PDT
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
Comment 29 Bear Travis 2012-04-11 17:11:54 PDT
Created attachment 136791 [details]
updated patch with new-style script test

Updating the added test to use the new-style script tests
Comment 30 Ryosuke Niwa 2012-04-11 18:02:15 PDT
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?
Comment 31 Bear Travis 2012-04-11 23:11:24 PDT
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
Comment 32 Ryosuke Niwa 2012-04-12 02:30:40 PDT
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?
Comment 33 Bear Travis 2012-04-12 10:50:51 PDT
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.
Comment 34 Bear Travis 2012-04-12 12:10:05 PDT
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.
Comment 35 WebKit Review Bot 2012-04-12 12:54:40 PDT
Comment on attachment 136931 [details]
patch with alternate test syntax

Clearing flags on attachment: 136931

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