Bug 119929 – Replace error-prone value with check from CSSProperty

• Component: HTML Editing
• Status: NEW

[Ryosuke Niwa]

Merge https://chromium.googlesource.com/chromium/blink/+/1b43d6960e69852b97927ed2a39c5eecc207db48

In EditingStyle, there is a static integer that specifies the number of
non-inherited editing properties, that needed to appear in the beginning
of editingProperties vector. A better approach is to have a runtime
check using CSSProperty::isInheritedProperty().

[Darin Adler]

What makes the static integer “error-prone” exactly? Maybe there’s some better justification here, or the wording of the justification is confusing?

[Bruno Abinader]

(In reply to comment #1)
> What makes the static integer “error-prone” exactly? Maybe there’s some better justification here, or the wording of the justification is confusing?

What I wanted to meant was that the same check is already implemented on CSSProperty::isInheritedProperty(). By using a static integer value, we are (1) duplicating code checks, and (2) more vulnerable to errors: one can add/remove a non-inherited property but forget to update the static integer value, or add a new property in the wrong place (i.e. after the integer value specified). By having a runtime check (like the one already implemented in CSSProperty), we can no longer worry about where to put the property in the vector, or update the integer value.

[Darin Adler]

(In reply to comment #2)
> (1) duplicating code checks

Yes, the check should be shared and exist in only one place in the code, whether it’s based on a static integer or not.

> (2) more vulnerable to errors: one can add/remove a non-inherited property but forget to update the static integer value, or add a new property in the wrong place (i.e. after the integer value specified). By having a runtime check (like the one already implemented in CSSProperty), we can no longer worry about where to put the property in the vector, or update the integer value.

I am not completely convinced. It’s true that you could make errors before, but you can still make the error of adding a property and having it be treated as inherited when it should be treated as non-inherited. Is this benefit so good it’s worth runtime cost?

[Bruno Abinader]

(In reply to comment #3)
> (In reply to comment #2)
> > (1) duplicating code checks
> 
> Yes, the check should be shared and exist in only one place in the code, whether it’s based on a static integer or not.
> 
> > (2) more vulnerable to errors: one can add/remove a non-inherited property but forget to update the static integer value, or add a new property in the wrong place (i.e. after the integer value specified). By having a runtime check (like the one already implemented in CSSProperty), we can no longer worry about where to put the property in the vector, or update the integer value.
> 
> I am not completely convinced. It’s true that you could make errors before, but you can still make the error of adding a property and having it be treated as inherited when it should be treated as non-inherited. Is this benefit so good it’s worth runtime cost?

I agree with you, there's still the risk of erroneous handling of properties. There's a reason why I haven't backported this patch from Blink to WebKit after landing it: In Blink, most experimental features are now triggered in runtime (i.e. via chrome://flags), so we have to check for all runtime-enabled properties already. However, once they are filtered and stored in a static vector, these are just copied upon need (see link below):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/editing/EditingStyle.cpp&l=98

In WebKit, we copy directly from editingProperties vector, so the mechanism is easier to maintain (though we still duplicate inheritance information also available in CSSProperty::isInheritedProperty). That said, I have to agree with you this might not be worth the cost for WebKit, if we also don't implement the additional static vectors that Blink uses to store that information (and prevent runtime checks every time the vector needs to be copied).

[Bruno Abinader]

Additional info: Blink revision that adds runtime check of CSS properties (incl. static runtime-populated vectors in EditingStyle.cpp):
https://src.chromium.org/viewvc/blink?revision=149139&view=revision