We could avoid a bunch of property name lookups (and probably save on codesize, too), by creating an "interface" that would inherit from nsIDOMCSSDeclaration but would basically expose two methods: GetPropertyValue(const nsCSSProperty aName, nsAString& aValue); SetPropertyValue(const nsCSSProperty aName, nsAString& aValue); All our CSS declarations would implement this (all of these should be pretty much one-liners to existing functions or simple modifications of existing functions; computed style will need a bit of changing, but shouldn't be too bad). Since we already use the preprocessor to implement all the CSS2Properties methods, changing CSS2Properties to use this would be very simple. The win would be to eliminate the string-to-nsCSSProperty conversions we do now for access to CSS2Properties methods and to eliminate all those literal strings in the CSS2Properties implementation. Thoughts?
Er, I meant to take this, assuming it sounds reasonable to people....
Assignee: dbaron → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha
Created attachment 148618 [details] [diff] [review] Proof-of-concept patch This reduces the codesize of layout by about 30k (and speeds up inline style access, of course, which was the point of the exercise).
Comment on attachment 148618 [details] [diff] [review] Proof-of-concept patch David, I'm not sure I'm completely happy with nsICSSDeclaration as the classname, nor with it being an interface. I did want to have nsIComputedDOMStyle inherit from it, so that I wouldn't have an extra vtable pointer on the computed style declaration. But the class could be called nsBaseCSSDeclaration or something and could have no IID (and not be QIable to), if you'd prefer.
Summary: CSS2PropertiesTearoff could avoid property name lookups and reduce codesize → [FIX]CSS2PropertiesTearoff could avoid property name lookups and reduce codesize
Comment on attachment 148618 [details] [diff] [review] Proof-of-concept patch The logical name might be nsIDOMCSSStyleDeclarationInternal, but that's too long, so this seems fine, but if you think of something else that's ok too. The |aPropName|s that are now nsCSSProperty should probably be |aPropID|. Could the nsComputedDOMStyle changes look more like the nsDOMCSSDeclaration changes? Or am I confusing the class hierarchy. (I'll look shortly...)
> The |aPropName|s that are now nsCSSProperty should probably be |aPropID|. Will do. > Could the nsComputedDOMStyle changes look more like the nsDOMCSSDeclaration > changes? The current class hierarchy is: nsDOMCSSDeclaration : public nsIDOMCSSStyleDeclaration nsComputedDOMStyle : public nsIComputedDOMStyle : public nsIDOMCSSStyleDeclaration Or did you mean changing the CSS2Propreties methods of nsComputedDOMStyle to use prop ids directly instead of converting to string? I could do that, and change GetCSSPropertyValue(nsAString&) to convert the string to an ID and call the internal function I'd add....
Comment on attachment 148618 [details] [diff] [review] Proof-of-concept patch This breaks setting properties to "".
Created attachment 149012 [details] [diff] [review] Fix the "" issue and use prop ids.
Attachment #148618 - Attachment is obsolete: true
Target Milestone: mozilla1.8alpha1 → mozilla1.8alpha2
Attachment #149012 - Attachment is obsolete: true
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.