243728 - [FIX]CSS2PropertiesTearoff could avoid property name lookups and reduce codesize

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Er, I meant to take this, assuming it sounds reasonable to people....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: 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 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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...)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 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 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148618 [details] [diff] [review]
Proof-of-concept patch

This breaks setting properties to "".

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the "" issue and use prop ids.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to changes in bug 245572

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.