172207 - The huge switch in nsComputedDOMStyle::GetPropertyCSSValue() should die

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

I've hated this thing for quite a while.  I'm finally looking into removing it
once and for all.

Comment 1

[Alec Flett]

kick ass!

for reference:
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsComputedDOMStyle.cpp#453

There is another giant CSS switch that is multi-tiered. I'll see if I can find
it and file another bug. it looks something like:

switch(some_enum)
case eCSSFoo:
case eCSSBar:
case eCSSfoopy:
    CreateSomeObject(some_enum);

and CreateSomeObject has another switch, if I recall correctly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

You must be thinking of nsCSSDeclaration::AppendValue,
nsCSSDeclaration::SetValueImportant, nsCSSDeclaration::RemoveProperty,
nsCSSDeclaration::GetValue. In fact, most of nsCSSDeclaration.cpp is huge switch
statements.... That's why dbaron wants to put more info in nsCSSPropList.h -- if
not helping footprint, it would at least lead to more-readable code....

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1

Unfortunately, this doesn't seem to save us much in the way of footprint.  The
change with gcc 2.95.3 is minimal - 29 bytes.  With gcc 3.2, we get 2,885
bytes.

However, this makes for more maintanable code, and I can see a need for the
computed style map in the near future.	(Implementing GetCssText could iterate
through this and call the appropriate methods, and we may be able to use this
to make CSSValues live as well...)

Comments?

Comment 4

[Boris Zbarsky [:bzbarsky]]

>    NS_ENSURE_ARG_POINTER(aLength);
> -  *aLength = sizeof(queryableProperties) / sizeof(nsCSSProperty);

I know that's not your code, but change the NS_ENSURE_ARG_POINTER to a
NS_PRECONDITION?  JS can't pass a null there and C++ callers should really know
better.

> +      (this->*(propMap[i].mGetter))(frame, aReturn);

Check the return value of this and null out *aReturn as needed?

> +    NS_WARNING("aPropertyName is non-queryable!");

How about a useful error string?  Like:

NS_WARNING(PromiseFlatCString(aPropertyName + 
                              NS_LITERAL_CSTRING(" is non-queryable!")).get());

> +#define COMPUTED_STYLE_MAP(_prop, _method)  \
> +  ComputedStyleMap(eCSSProperty_##_prop, &nsComputedDOMStyle::Get##_method)

It's more of a map entry than a map, no?

> +  ComputedStyleMap* GetQueryablePropertyMap(PRUint32* aLength) const;

This would make more sense as a class-static method, I think, than as a member
method....

> +class ComputedStyleMap

Would it make sense to make this an inner class?

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2

Fixes silly goofs that bz spotted.  (Thanks!)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101858 [details] [diff] [review]
Patch v2

sr=bzbarsky but watch the tree when you check in; I would not be surprised if
some of our compilers wanted you to qualify all uses of ComputedStyleMap, not
just the return value....

Comment 7

[Alec Flett]

I think the reason that you don't see much of a code size improvement is that
you're calling through the constructor during creation of each entry. This is
going to result in a lot of bloat, because you're copying data from the code
segment into some writable region of memory the first time this function is
executed.

Instead, make the static ComputedStyleMap (which should also be called
ComputedStyleMapEntry) not have to initialize, because its value should just be
mapped into memory.

To determine if you're causing the copy, remove the constructor definition and
see if you can still compile. If so, then you're calling through it. If not,
you're probably safe, but I don't think there should be a constructor anyway.

Independent of that issue, you need to make more use of "const" - that will make
sure that the data structure you declare stays in read-only memory.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v3

More const, use a struct instead of a class w/ ctor.  I wasn't able to compile
with simply removing the ctor definition, but changing it to a struct and
killing it altogether seems to work nicely.  I did originally have a struct
instead of a class w/ ctor, but at one point something forced me to use a class
and I just left it that way.

Anyway, with gcc 2.95.3 we now reduce code size by about 3k.  I won't be able
to test with gcc 3.2 until later on, but this definitely looks a bit better.

Comment 9

[Alec Flett]

Comment on attachment 101977 [details] [diff] [review]
Patch v3

nice. Glad to hear we got some real savings out of this! Its a first example of
a number of places we need to repair. 

sr=alecf

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101977 [details] [diff] [review]
Patch v3

r=bzbarsky assuming you have tested this and it works and all.	;)

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in.