Bug 12159 – Most of CSSComputedStyleDeclaration, cssparser.cpp, cssstyleselector.cpp should be autogenerated

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Most of CSSComputedStyleDeclaration, cssparser.cpp, cssstyleselector.cpp should be autogenerated

Hyatt and I were just talking about this over IRC.... most of the CSS code is pretty repetitive (and as a result ugly).  It's full of copy/paste and macro code which is either prone to errors or difficult to debug.  It would be best to replace most of this CSS code with autogenerated code, similar to what was done with the JS bindings system.

[Alexey Proskuryakov]

Is autogeneration the only way to improve CSS code? Maybe it's just me, but I find autogenerated code to be harder to read and debug than any other kind of code.

*** Bug 15427 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

I'd be happy to discuss specific examples of code that is suggested for auto-generation, hoping to suggest alternatives, e.g. template-based. If there aren't any, then auto-generation may be a great answer (as it is for IDL-generated bindings).

[Alexey Proskuryakov]

I think that a lot of ugly code can be eliminated by moving enum mapping to CSSPrimitiveValue. E.g.

            switch (style->resize()) {
                case RESIZE_BOTH:
                    return new CSSPrimitiveValue(CSS_VAL_BOTH);
                case RESIZE_HORIZONTAL:
                    return new CSSPrimitiveValue(CSS_VAL_HORIZONTAL);
                case RESIZE_VERTICAL:
                    return new CSSPrimitiveValue(CSS_VAL_VERTICAL);
                case RESIZE_NONE:
                    return new CSSPrimitiveValue(CSS_VAL_NONE);
            }

could hopefully become

            return new CSSPrimitiveValue(style->resize());

[Eric Seidel (no email)]

(In reply to comment #4)
> I think that a lot of ugly code can be eliminated by moving enum mapping to
> CSSPrimitiveValue. E.g.

That's an interesting idea, but that would mean creating 100 different constructors, no?  Or would you use some sort of template magic to do such?  (If so, please provide an example.)

[Alexey Proskuryakov]

(In reply to comment #5)
> That's an interesting idea, but that would mean creating 100 different
> constructors, no?

Yes, I don't see how to avoid that yet. Still, should save quite a few lines of code.

I'm going to actually work on some patches along these lines.

[Alexey Proskuryakov]

Created attachment 16667 [details]
Move enum mapping into a separate file

This patch is IMHO helpful regardless of whether we decide to auto-generate any more code.

The CSSComputedStyleDeclaration part is pretty straightforward. In CSSStyleSelector, there was some error checking I don't quite understand, especially because not all cases have it. I might have removed something important, or kept something unnecessary.

-        if (!primitiveValue || !primitiveValue->getIdent())
-            return;
...
-    if (!value->isPrimitiveValue()) return;

It seems that these are guaranteed by the parser. Similarly, some checks for unexpected ident values may have changed, but it doesn't seem that they actually come into play unless memory is overwritten.

[Darin Adler]

Comment on attachment 16667 [details]
Move enum mapping into a separate file

+            ASSERT(boxAlign != BJUSTIFY);
+            if (boxAlign == BJUSTIFY)
+                return 0;

+            ASSERT(pageBreak != PBALWAYS);
+            if (pageBreak == PBALWAYS)
+                return 0;

The checking here seems unnecessary. I think we can just omit it.

Do we have test cases that cover all the different length types and property values?

-        CSSPrimitiveValue* val = new CSSPrimitiveValue(pair);
+        CSSPrimitiveValue* val = new CSSPrimitiveValue(PassRefPtr<Pair>(pair));

I think that needing to do this is quite unfortunate. Because of this, I'm not happy with the template conversion technique. Can we switch to a simpler technique that doesn't involve templates?

We could just put all the enums into a separate header file and use plain old overloading.

I otherwise think this is cool, and a great direction.

[Alexey Proskuryakov]

(In reply to comment #8)

> Can we switch to a simpler technique that doesn't involve templates?

  I thought plain overloading would be unsafe, because missing constructors wouldn't be detected - there are already constructors that take int and unsigned.

  One option I thought of (and actually started to implement) was to use a separate construction method, something like

    template<typename T> static CSSPrimitiveValue* fromEnum(T);

  But that looked even worse than having to explicitly construct PassRefPtr in a few places.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #8)
> 
> > Can we switch to a simpler technique that doesn't involve templates?
> 
>   I thought plain overloading would be unsafe, because missing constructors
> wouldn't be detected - there are already constructors that take int and
> unsigned.

Wow, that's a good point. I had overlooked that.

Maybe we should go back to your original patch, but I have some other ideas worth considering:

    1) Change the identifier values into an enum, and get rid of the int constructor. Probably straightforward to do.
    2) For colors, change the constructor to either use a wrapper (WebCore::Color has an unwanted "invalid" state, and is slightly less efficient than a plain RGB value, but it might still be OK) or a named constructor like fromColor.

I don't feel super-strongly about the color one, but I do think that the "int for identifier" thing is weak and should be changed.

[Darin Adler]

3) Use the simple "extra parameter" technique used in RetainPtr's Adopt constructors. An initial parameter with a type that's significant, but not a value.

[Alexey Proskuryakov]

One thing that I like about the template-based solution is that there is no need to add dozens of constructors to CSSPrimitiveValue.h. Not that there would be anything inherently wrong with that, but it looks somewhat ugly to me.

[Darin Adler]

(In reply to comment #12)
> One thing that I like about the template-based solution is that there is no
> need to add dozens of constructors to CSSPrimitiveValue.h. Not that there would
> be anything inherently wrong with that, but it looks somewhat ugly to me.

I think it's clearer to have the declarations, even though it's a little uglier.

[Alexey Proskuryakov]

Created attachment 16692 [details]
updated patch

(In reply to comment #8)

> The checking here seems unnecessary. I think we can just omit it.

Done.

> Do we have test cases that cover all the different length types and property
> values?

Hmm, I think the tests in css1/ and css2.1/ cover this.

> I think that needing to do this is quite unfortunate. Because of this, I'm not
> happy with the template conversion technique. Can we switch to a simpler
> technique that doesn't involve templates?

Attached is another attempt to save the template technique :)

[Darin Adler]

Comment on attachment 16692 [details]
updated patch

OK. I can live with this. r=me

[Alexey Proskuryakov]

Comment on attachment 16692 [details]
updated patch

Patch committed in revision 26742, clearing the review flag to keep the bug open.

[Eric Seidel (no email)]

We should probably just open a new bug and mark this as a "depends on".  Generally one commit per bug.

[Eric Seidel (no email)]

This was done a while ago.