Add support for disabling CSS Properties at runtime

Enable runtime switching of CSS Properties.

In order to prevent this from changing test results I had to also make CSS Exclusions runtime-enabled in DRT (now that we're actually respecting the runtime enabled flag in CSS).

In ContentShell this test will make more sense as we will be able to actually test a shipping configuration instead of whatever DRT happens to have its flags set to.

BUG=232181
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149139

[mitica, 2013-04-18 18:11:15]

Early version of the patch, requesting some feedback on the work so far :)

[eseidel, 2013-04-22 17:48:29]

I think that looks great.  We used to have a BitVector class in WTF, but I think
it got deleted as part of the grand purge.

We're likely going to need a way for these to be runtime toggled so that we can
test these.  I'll look into what sort of runtime testing support we have.

[eseidel, 2013-04-23 01:31:59]

https://codereview.chromium.org/14324009/diff/6001/bindings/v8/custom/V8CSSSt...
File bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):

https://codereview.chromium.org/14324009/diff/6001/bindings/v8/custom/V8CSSSt...
bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:252: if
(!isCSSValueEnabled(wsValue))
I don't believe this part is correct or necessary.  I think this would make it
detectable that a property existed and was disabled. :)  We should disable these
in the parser and not at this choke point.

[eseidel, 2013-04-23 01:36:36]

https://codereview.chromium.org/14324009/diff/6001/bindings/v8/custom/V8CSSSt...
File bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):

https://codereview.chromium.org/14324009/diff/6001/bindings/v8/custom/V8CSSSt...
bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:174: if (!propInfo ||
propInfo->propID == CSSPropertyInvalid)
When is CSSPropertyInvalid ever possible in propInfo?  Assuming
CSSPropertyInvalid == 0, then this should never be possible?

[mitica, 2013-04-23 14:02:13]

The BitVectors have different lengths for Properties and Values, I've made a
change for fixing this, also, I validated the feature with some manual testing
on CSS Regions properties, everything went as expected.

[Julien - ping for review, 2013-04-23 21:18:42]

Wouldn't it make more sense to have the files RuntimeCSSEnabled.{cpp|h} be
auto-generated from CSSPropertyNames.in (and CSSValueKeywords.in) in the same
manner that the element factories work instead of hard-coding some known
properties? (I agree this approach could be a good first pass but this won't
scale in the long run)

[eseidel, 2013-04-24 03:14:56]

Yup!  I completely agree Julien!  This is just enough to get Adobe and others
(who want to work on runtime-enabled features w/o risking shipping to stable)
unblocked.

[eseidel, 2013-04-24 03:18:14]

It's possible (probable?) that this will need more expectations updates since
this effectively disables CSS EXCLUSIONS on trunk (since they were runtime
disabled by chance).

Once this has seen at least a first-pass review, I should probably re-enable CSS
exclusions at runtime in DRT and make sure they're disabled at the content
layer.

[mitica, 2013-04-24 06:24:25]

https://codereview.chromium.org/14324009/diff/33001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp (right):

https://codereview.chromium.org/14324009/diff/33001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:162: for (int id =
firstCSSProperty; id < firstCSSProperty + numCSSProperties; ++id) {
It would probably be better to use lastCSSProperty instead of
firstCSSProperty+numCSSProperties

https://codereview.chromium.org/14324009/diff/33001/Source/core/css/CSSParser...
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/14324009/diff/33001/Source/core/css/CSSParser...
Source/core/css/CSSParser.cpp:1287: // FIXME: This should check
RuntimeCSSEnabled::isCSSPropertyEnabled(propertyID).
Hopefully I dont' get this wrong, but shouldn't this rather check
RuntimeCSSEnabled::isCSSValueEnabled ?

[eseidel, 2013-04-24 20:43:17]

https://codereview.chromium.org/14324009/diff/33001/Source/core/css/CSSParser...
> Source/core/css/CSSParser.cpp:1287: // FIXME: This should check
> RuntimeCSSEnabled::isCSSPropertyEnabled(propertyID).
> Hopefully I dont' get this wrong, but shouldn't this rather check
> RuntimeCSSEnabled::isCSSValueEnabled ?

It's going to need to test both.  I've updated the comment and will re-upload.

[abarth-chromium, 2013-04-24 20:47:34]

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:2: * Copyright (C) 2013 Adobe Systems
Incorporated. All rights reserved.
Adobe?

[eseidel, 2013-04-24 20:49:07]

Mitica worked on earlier revisions of the patch, hence the copyright is there. 
I can add Google's as well if you like.

[Use mkwst_at_chromium.org plz., 2013-04-24 20:51:43]

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
As a drive-by,
https://src.chromium.org/viewvc/blink/trunk/Source/wtf/BitVector.h is probably
what you're looking for. It was not purged. :)

[eseidel, 2013-04-24 20:56:31]

On 2013/04/24 20:51:43, mkwst wrote:
>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> File Source/core/page/RuntimeCSSEnabled.cpp (right):
> 
>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
> As a drive-by,
> https://src.chromium.org/viewvc/blink/trunk/Source/wtf/BitVector.h is probably
> what you're looking for. It was not purged. :)

Neat!  I thought it was.  I'll leave it as-is for now though.

[eseidel, 2013-04-24 20:57:36]

PTAL.  This is not WIP. :)  It's ready for review.

[abarth-chromium, 2013-04-24 21:02:13]

I'm not the right person to give you the official review stamp.  Some minor nits
below.  Otherwise looks good.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
If you don't use WTF::BitVector, you should use a different name so folks aren't
confused.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:103: size_t indexForValue(CSSValueID
valueId)
static

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:109: size_t
indexForProperty(CSSPropertyID propertyId)
static

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.h (right):

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.h:36: #include <wtf/Vector.h>
There's no need to include this header.

https://codereview.chromium.org/14324009/diff/45001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/WebPreferences.cpp (right):

https://codereview.chromium.org/14324009/diff/45001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/WebPreferences.cpp:158:
WebRuntimeFeatures::enableCSSExclusions(experimentalCSSExclusionsEnabled);
This isn't really the right place to call these functions.  These are
process-global and immutable.  The WebSettings feature are per-page and mutable.

[ojan, 2013-04-25 02:25:21]

lgtm

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
File Source/core/page/RuntimeCSSEnabled.cpp (right):

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
+1 to just using BitVector.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:39: static void setFromFeatureValue(bool
featureFlag, CSSPropertyID* properties, size_t length)
Nit: this doesn't need to take a length. It can just call WTF_ARRAY_LENGTH
directly.

Also, this name is weird. Maybe setMultipleProperties? And, it's customary for a
set function to take the value it's setting as the second argument (e.g. as
setCSSPropertyEnabled does).

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
Source/core/page/RuntimeCSSEnabled.cpp:116: bool
RuntimeCSSEnabled::isCSSValueEnabled(CSSValueID valueId)
I don't think it makes sense to put these stubs in until we're actually going to
use them. I'm not sure we'll ever use this. We can already runtime disable CSS
values. Also, it's not really correct to call a given value enabled/disabled.
It's enabled/disabled as a value for a given property, which is why the current
way of disabling it in CSSParser.cpp seems better to me.

[ojan, 2013-04-25 02:25:39]

not lgtm

Whoops. Didn't mean to hit the lgtm button!

[eseidel, 2013-04-25 07:27:46]

On 2013/04/25 02:25:21, ojan wrote:
> lgtm
> 
>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> File Source/core/page/RuntimeCSSEnabled.cpp (right):
> 
>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
> +1 to just using BitVector.

It has a very different API.  I'd rather just leave it as a FIXME for now.

>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> Source/core/page/RuntimeCSSEnabled.cpp:39: static void
setFromFeatureValue(bool
> featureFlag, CSSPropertyID* properties, size_t length)
> Nit: this doesn't need to take a length. It can just call WTF_ARRAY_LENGTH
> directly.

I didn't know that.  So I can call WTF_ARRAY_LENGTH on any random pointer?

> Also, this name is weird. Maybe setMultipleProperties? And, it's customary for
a
> set function to take the value it's setting as the second argument (e.g. as
> setCSSPropertyEnabled does).

it felt odd to write it (array, length, bool) but if I only need (array, bool)
then that's fine.

This was all just an exploration to see which one looked better (manual calls or
an array-based helper.)

>
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> Source/core/page/RuntimeCSSEnabled.cpp:116: bool
> RuntimeCSSEnabled::isCSSValueEnabled(CSSValueID valueId)
> I don't think it makes sense to put these stubs in until we're actually going
to
> use them. I'm not sure we'll ever use this. We can already runtime disable CSS
> values. Also, it's not really correct to call a given value enabled/disabled.
> It's enabled/disabled as a value for a given property, which is why the
current
> way of disabling it in CSSParser.cpp seems better to me.

I can see that perspective.  Maybe this should be a
isValueEnabledForProperty(value, property) then.  I can remove the code, since
it's effectively dead.

[ojan, 2013-04-25 07:43:10]

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> > Source/core/page/RuntimeCSSEnabled.cpp:37: typedef Vector<bool> BitVector;
> > +1 to just using BitVector.
> 
> It has a very different API.  I'd rather just leave it as a FIXME for now.

OK.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> > Source/core/page/RuntimeCSSEnabled.cpp:39: static void
> setFromFeatureValue(bool
> > featureFlag, CSSPropertyID* properties, size_t length)
> > Nit: this doesn't need to take a length. It can just call WTF_ARRAY_LENGTH
> > directly.
> 
> I didn't know that.  So I can call WTF_ARRAY_LENGTH on any random pointer?

I think so? In practice, you're only passing in arrays here, right? I don't know
what it does if you call it on a non-array.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> > Source/core/page/RuntimeCSSEnabled.cpp:116: bool
> > RuntimeCSSEnabled::isCSSValueEnabled(CSSValueID valueId)
> > I don't think it makes sense to put these stubs in until we're actually
going
> to
> > use them. I'm not sure we'll ever use this. We can already runtime disable
CSS
> > values. Also, it's not really correct to call a given value
enabled/disabled.
> > It's enabled/disabled as a value for a given property, which is why the
> current
> > way of disabling it in CSSParser.cpp seems better to me.
> 
> I can see that perspective.  Maybe this should be a
> isValueEnabledForProperty(value, property) then.  I can remove the code, since
> it's effectively dead.

Figuring out the allowed values for a property is pretty much all that big
switch statement in CSSParser.cpp does. So, you'd be duplicating a lot of code.
I think the long-term solution to this is that all that code in CSSParser.cpp
should be generated from a .in file and the .in file needs to know about runtime
flags. In the short-term, I don't think we gain much by trying to generalize
which values are allowed by which properties.

[eseidel, 2013-04-25 07:49:56]

On 2013/04/25 07:43:10, ojan wrote:
https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> > > Source/core/page/RuntimeCSSEnabled.cpp:39: static void
> > setFromFeatureValue(bool
> > > featureFlag, CSSPropertyID* properties, size_t length)
> > > Nit: this doesn't need to take a length. It can just call WTF_ARRAY_LENGTH
> > > directly.
> > 
> > I didn't know that.  So I can call WTF_ARRAY_LENGTH on any random pointer?
> 
> I think so? In practice, you're only passing in arrays here, right? I don't
know
> what it does if you call it on a non-array.

It doesn't work. :)  The compiler doesn't have enough information if you pass it
in as a pointer.

https://codereview.chromium.org/14324009/diff/45001/Source/core/page/RuntimeC...
> > > Source/core/page/RuntimeCSSEnabled.cpp:116: bool
> > > RuntimeCSSEnabled::isCSSValueEnabled(CSSValueID valueId)
> > > I don't think it makes sense to put these stubs in until we're actually
> going
> > to
> > > use them. I'm not sure we'll ever use this. We can already runtime disable
> CSS
> > > values. Also, it's not really correct to call a given value
> enabled/disabled.
> > > It's enabled/disabled as a value for a given property, which is why the
> > current
> > > way of disabling it in CSSParser.cpp seems better to me.
> > 
> > I can see that perspective.  Maybe this should be a
> > isValueEnabledForProperty(value, property) then.  I can remove the code,
since
> > it's effectively dead.
> 
> Figuring out the allowed values for a property is pretty much all that big
> switch statement in CSSParser.cpp does. So, you'd be duplicating a lot of
code.
> I think the long-term solution to this is that all that code in CSSParser.cpp
> should be generated from a .in file and the .in file needs to know about
runtime
> flags. In the short-term, I don't think we gain much by trying to generalize
> which values are allowed by which properties.

I think having a function whos purpose is to map from values to runtime flags
seems useful, but maybe it won't end up being.  In any case, that's for a second
patch.

[eseidel, 2013-04-25 07:51:09]

PTAL

[eseidel, 2013-04-25 20:11:13]

Please please take another look?

[esprehn, 2013-04-25 20:44:31]

not lgtm. This isn't going to work because CSSComputedStyleDeclaration has a
hard list of properties for getComputedStyle. See the computedProperties[] we
use there.

[eseidel, 2013-04-25 21:09:50]

You people are ridiculous.  Can we not land something and move forward?

[eseidel, 2013-04-25 21:22:49]

I will have a patch up shortly to handle CSSComputedProperties.  Just frustrated
at my lack of time to code and how long it's taken this to get reviewed and in.

[eseidel, 2013-04-25 22:19:51]

PTAL

[esprehn, 2013-04-25 22:26:03]

You've been pretty busy lately, and I was concerned that if we missed computed
style we'd move all our experimental features behind this and then ship
something where we leak all the experimental properties to the web (or they
don't work at all).

Anyway LGTM.

[eseidel, 2013-04-25 22:35:04]

I don't believe that this necessarily gives us full coverage.  From my minimal
investigations, I believe we're already unintentionally leaking features
(prefixed or otherwise) and we are going to need some better testing to confirm.

As soon as Content Shell lands, I will add some tests to document our
Stable-exposed Web API.  I have them written, but they are meaningless in DRT
(since it's runtime feature flags are different from content's).

I believe I will need a second lg.. before the tools are happy.  I'm told lg..
is +1 and not lg.. is -1, and you need to be positive to land?

[ojan, 2013-04-25 22:54:09]

lgtm

On 2013/04/25 22:35:04, Eric Seidel (Google) wrote:
> I believe I will need a second lg.. before the tools are happy.  I'm told lg..
> is +1 and not lg.. is -1, and you need to be positive to land?

Really? In google3, last one wins.

[eseidel, 2013-04-25 23:05:23]

I'm probably just confused.

[commit-bot: I haz the power, 2013-04-25 23:05:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitica@adobe.com/14324009/64001

[commit-bot: I haz the power, 2013-04-25 23:05:33]

Failed to apply patch for Source/Source/core/page/RuntimeCSSEnabled.h:
While running svn copy Source/Source/core/rendering/ExclusionInterval.h
Source/Source/core/page/RuntimeCSSEnabled.h --config-dir
/b/commit-queue/subversion_config --non-interactive --username
commit-bot@chromium.org;
  A         Source/Source
  Created missing directory Source/Source.
  A         Source/Source/core
  Created missing directory Source/Source/core.
  A         Source/Source/core/page
  Created missing directory Source/Source/core/page.
  svn: Path 'Source/Source/core/rendering/ExclusionInterval.h' does not exist

Patch:   NR 
Source/Source/core/rendering/ExclusionInterval.h->Source/Source/core/page/RuntimeCSSEnabled.h
Index: Source/core/page/RuntimeCSSEnabled.h
diff --git Source/Source/core/rendering/ExclusionInterval.h
Source/Source/core/page/RuntimeCSSEnabled.h
similarity index 62%
copy from Source/Source/core/rendering/ExclusionInterval.h
copy to Source/Source/core/page/RuntimeCSSEnabled.h
index
5d31feb18113de8c1e4341e0103708b774ca351f..71a19a4630ead925e897edeca366108c8356138a
100644
--- a/Source/Source/core/rendering/ExclusionInterval.h
+++ b/Source/Source/core/page/RuntimeCSSEnabled.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.
+ * Copyright (C) 2013 Adobe Systems Incorporated. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -12,7 +12,7 @@
  *    copyright notice, this list of conditions and the following
  *    disclaimer in the documentation and/or other materials
  *    provided with the distribution.
- * 
+ *
  * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
  * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
  * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
@@ -27,32 +27,28 @@
  * OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef ExclusionInterval_h
-#define ExclusionInterval_h
+#ifndef RuntimeCSSEnabled_h
+#define RuntimeCSSEnabled_h
 
-#include <wtf/Vector.h>
+#include "CSSPropertyNames.h"
+#include "wtf/Vector.h"
 
 namespace WebCore {
 
-struct ExclusionInterval {
+// A class storing static arrays for enabling CSS properties at
+// runtime. By default, all properties are enabled.
+
+class RuntimeCSSEnabled {
 public:
-    float x1;
-    float x2;
+    static bool isCSSPropertyEnabled(CSSPropertyID);
+    static void setCSSPropertyEnabled(CSSPropertyID, bool);
 
-    ExclusionInterval(float x1 = 0, float x2 = 0)
-        : x1(x1)
-        , x2(x2)
-    {
-    }
+    static void filterEnabledCSSPropertiesIntoVector(const CSSPropertyID*,
size_t length, Vector<CSSPropertyID>&);
 
-    bool intersect(const ExclusionInterval&, ExclusionInterval&) const;
+private:
+    RuntimeCSSEnabled();
 };
 
-void sortExclusionIntervals(Vector<ExclusionInterval>&);
-void mergeExclusionIntervals(const Vector<ExclusionInterval>&, const
Vector<ExclusionInterval>&, Vector<ExclusionInterval>&);
-void intersectExclusionIntervals(const Vector<ExclusionInterval>&, const
Vector<ExclusionInterval>&, Vector<ExclusionInterval>&);
-void subtractExclusionIntervals(const Vector<ExclusionInterval>&, const
Vector<ExclusionInterval>&, Vector<ExclusionInterval>&);
-
 } // namespace WebCore
 
-#endif // ExclusionInterval_h
+#endif // RuntimeCSSEnabled_h

[jamesr, 2013-04-25 23:10:52]

lgtm for /public/

[jamesr, 2013-04-25 23:11:51]

Are you planning to modify the version of WebPreferences in the chromium tree
with this bit as well? It lives in webkit/glue/webpreferences.h

[eseidel, 2013-04-25 23:12:38]

Committed patchset #13 manually as r149139 (presubmit successful).