[Webkit-unassigned] [Bug 119929] Replace error-prone value with check from CSSProperty

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=119929





--- Comment #4 from Bruno de Oliveira Abinader <bruno.d at partner.samsung.com>  2013-08-19 08:55:27 PST ---
(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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.