1298517 - Key & lookup CSS custom property registrations / values by atom instead of string?

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[Jonathan Chan [:jyc]]

In reviewing patches for bug 1273706, heycam noted that because in many places in animation code we convert the custom property atom (from CSSProperty::AsCustom()) to a string in order to look up custom property registrations & values in CSSVariableRegistrations/CSSVariableValues, it might make sense to just key & lookup custom properties by atom instead of by string.

Other things to consider: maybe we could just have CSSProperty store an nsString instead of an atom? I originally stored it as an nsIAtom so I could hack custom property names into nsCSSProperty values (now nsCSSPropertyID). Would the nsStrings be shared?

Also, it is just animation code that accesses custom properties by atoms (because of how CSSProperty stores them). Is it worth changing over the other uses to access by atom?

Comment 1

[Jonathan Chan [:jyc]]

dbaron pointed out that comparisons between nsStrings, even if they share the same reference-counted buffer, would be more costly. With nsIAtoms we can use pointer equality, but nsStrings don't have that fastpath.

Comment 2

[Frédéric Wang (:fredw)]

This bug is referring to the initial implementation of 1273706 from 7 years ago. Is it still valid or can we close it?

AFAIK, in Servo (including animation code) we use crate::custom_properties::Name as keys which are Atoms:
https://searchfox.org/mozilla-central/rev/7d77ff808f8407a3e4fc0911779da446c050f9ee/servo/components/style/custom_properties.rs#158

Comment 3

[Frédéric Wang (:fredw)]

Changing dependency order since bug 1273706 is used as a meta bug.

Comment 4

[Zach Hoffman [:zrhoffman]]

A decent amount of the animation code is still in C++. I think bug 1298517 is still valid (though much less work) because dom::KeyframeEffect still gets property values by strings:

https://searchfox.org/mozilla-central/rev/7d77ff808f/dom/animation/KeyframeEffect.cpp#1154

Comment 5

[Emilio Cobos Álvarez (:emilio)]

The code linked in comment 4 above is only to expose property names to script (so that needs actual strings). There's a more general issue of web animations not dealing well with custom properties. When that code is taught about custom props, we should use atoms. But this bug as it exists I think can be closed.