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

RESOLVED FIXED in mozilla1.8alpha2

Status

()

P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({perf})

Trunk
mozilla1.8alpha2
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

15 years ago
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?
(Assignee)

Comment 1

15 years ago
Er, I meant to take this, assuming it sounds reasonable to people....
Assignee: dbaron → bzbarsky
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha
(Assignee)

Comment 2

15 years ago
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).
(Assignee)

Comment 3

15 years ago
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.
Attachment #148618 - Flags: superreview?(dbaron)
Attachment #148618 - Flags: review?(dbaron)
(Assignee)

Updated

15 years ago
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...)
(Assignee)

Comment 5

15 years ago
> 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....
(Assignee)

Comment 6

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

This breaks setting properties to "".
Attachment #148618 - Flags: superreview?(dbaron)
Attachment #148618 - Flags: superreview-
Attachment #148618 - Flags: review?(dbaron)
Attachment #148618 - Flags: review-
(Assignee)

Comment 7

15 years ago
Created attachment 149012 [details] [diff] [review]
Fix the "" issue and use prop ids.
Attachment #148618 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #149012 - Flags: superreview?(dbaron)
Attachment #149012 - Flags: review?(dbaron)
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.8alpha1 → mozilla1.8alpha2
Attachment #149012 - Flags: superreview?(dbaron)
Attachment #149012 - Flags: superreview+
Attachment #149012 - Flags: review?(dbaron)
Attachment #149012 - Flags: review+
(Assignee)

Comment 8

15 years ago
Created attachment 150239 [details] [diff] [review]
Patch updated to changes in bug 245572
(Assignee)

Updated

15 years ago
Attachment #149012 - Attachment is obsolete: true
(Assignee)

Comment 9

15 years ago
Checked in.
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.