[Webkit-unassigned] [Bug 54707] Introduce lookup-table based approach for applying CSS properties.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54707





--- Comment #52 from Nikolas Zimmermann <zimmermann at kde.org>  2011-03-12 02:25:02 PST ---
(From update of attachment 85506)
View in context: https://bugs.webkit.org/attachment.cgi?id=85506&action=review

As the idea is to make the code more readable, I have a few suggestions:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:48
> +    ApplyPropertyDefault(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())

Use typedefs to avoid repeating the function pointers.

typedef T (RenderStyle::*GetterMethod)() const;
typedef T void (RenderStyle::*SetterMethod)(T);
typedef T (*initialMethod)();

..
protected:
GetterMethod m_getter;
..

> Source/WebCore/css/CSSStyleApplyProperty.cpp:57
> +    {
> +        (selector->style()->*m_setter)((selector->parentStyle()->*m_getter)());

While you're at it: I don't think ASSERTIONS() will hurt here.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:65
> +    virtual void value(CSSStyleSelector* selector, CSSValue* value) const

Why is this named, value, instead of setValue? Or even better, I think, you should have used:
applyInheritValue() (maybe s/Value//)
applyInitialValue() (ditto)
applyValue()

> Source/WebCore/css/CSSStyleApplyProperty.cpp:67
> +        if (value->isPrimitiveValue())

Early return style is prefereed:
if (!value->isPrimitiveValue())
    return;

> Source/WebCore/css/CSSStyleApplyProperty.cpp:78
> +// CSSPropertyColor
> +class ApplyPropertyColorBase : public ApplyPropertyBase {

I don't think this is a good idea at all. It should go into a new file. Otherwhise your CSSStyleApplyProperty file will look just as cluttered as CSSStyleSelector is now.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:80
> +    ApplyPropertyColorBase(const Color& (RenderStyle::*getter)() const, const Color& (RenderStyle::*defaultValue)() const, void (RenderStyle::*setter)(const Color&))

Use typedefs for the function pointers.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:88
> +        const Color& color = (selector->parentStyle()->*m_getter)();

I'd think ASSERTIONS would be helpful as well here.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:97
> +        (selector->style()->*m_setter)(color);

Ditto.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:102
> +        if (value->isPrimitiveValue())
> +            (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(static_cast<CSSPrimitiveValue*>(value)));

Ditto + Early return.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:110
> +class ApplyPropertyColor : public ApplyPropertyColorBase {

Why is this seperation needed ApplyPropertyColor and base? It's highly confusing IMHO.

Imagine how many classes we'd end up creating, when converting everything into your new approach.
We should rather minimize the need of new classes.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:149
> +    setPropertyValue(CSSPropertyColor, new ApplyPropertyColor(&RenderStyle::color, &RenderStyle::setColor, RenderStyle::initialColor));

I'd rather prefer to move the setPropertyValue() calls in a new CSSStyleApplyPropertyColor.cpp file, in a static function.
So that CSSStyleApplyProperty only ever has to look like this:
CSSStyleApplyPropertyColor::initialize(this);
CSSStyleApplyPropertyXXX::initialize(this);
...

For each of the groups of properties.

> Source/WebCore/css/CSSStyleApplyProperty.h:47
> +class CSSStyleApplyProperty : public RefCounted<CSSStyleApplyProperty> {

Why is this refcounted if you plan to share a static single instance??

> Source/WebCore/css/CSSStyleApplyProperty.h:51
> +    void inherit(CSSPropertyID property, CSSStyleSelector* selector) const

Same renamings would be nice.

Sorry, I had no chance to post earlier, I'm kind of ill.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.