Summary: | [CSS Exclusions] Add flag to enable / disable exclusions at runtime | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bear Travis <betravis> | ||||||||||||||||||||||||||
Component: | CSS | Assignee: | Bear Travis <betravis> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, gustavo, jamesr, macpherson, menard, pnormand, rniwa, tkent, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
URL: | http://dev.w3.org/csswg/css3-exclusions/ | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Bear Travis
2012-04-05 14:21:31 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 on attachment 136053 [details] Prototype Patch Attachment 136053 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12359056 Created attachment 136084 [details]
Updated patch
Modified the patch to skip the test in webkit2, and removed the WebKit/chromium work.
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 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
Created attachment 136277 [details]
updated patch with chromium flag
adding the flag to the chromium platform, which should fix the cr-linux failure
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 on attachment 136277 [details]
updated patch with chromium flag
Chromium WebKit API changes look good to me.
Comment on attachment 136277 [details]
updated patch with chromium flag
Going to start looking for a reviewer.
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. (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. 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 (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? (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 on attachment 136277 [details]
updated patch with chromium flag
Investigating a better method of implementing the runtime flag using RuntimeEnabledFeatures.
Created attachment 136583 [details]
Patch using RuntimeEnabledFeatures
Running EWS to get the necessary symbols
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 on attachment 136583 [details] Patch using RuntimeEnabledFeatures Attachment 136583 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12381573 Created attachment 136598 [details]
Updated runtime enabled patch
Fixing the compile errors. Still need to run for the internal symbols.
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 on attachment 136598 [details] Updated runtime enabled patch Attachment 136598 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12386168 Created attachment 136622 [details]
runtime enabled patch with appropriate symbols
added the symbols for win and gtk
Comment on attachment 136622 [details]
runtime enabled patch with appropriate symbols
Patch is ready for review. Will look for a reviewer tomorrow.
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?
(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. 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.
Created attachment 136749 [details]
runtime enabled patch, defaulting to exclusions disabled
changing exclusions to default to disabled, per kling's feedback
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 Created attachment 136791 [details]
updated patch with new-style script test
Updating the added test to use the new-style script tests
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? 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 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? 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 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 on attachment 136931 [details] patch with alternate test syntax Clearing flags on attachment: 136931 Committed r114020: <http://trac.webkit.org/changeset/114020> All reviewed patches have been landed. Closing bug. |