[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


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84983|review?                     |review-
               Flag|                            |




--- Comment #36 from Eric Seidel <eric at webkit.org>  2011-03-07 21:17:35 PST ---
(From update of attachment 84983)
View in context: https://bugs.webkit.org/attachment.cgi?id=84983&action=review

Some of my comments below, are just that: comments.  And not necessarily requiring action, but I thought it useful to write down my thoughts about the patch as I had them.

In general, this is great!  I'm ready to r+ this as soon as you have PLT numbers or some other performance test which shows that the total time for this is either unchanged, or better.  Unfortunately the samples you posted (although interesting!) do not prove that the overall execution time is less with this patch applied (which is the important part).

You should also be sure to document the rational and performance improvement in the ChangeLog.  (Basically your ChangeLog is very sparse.  It's the first thing historians look at when trying to understand what you did, so It's nice to explain yourself.  I look at them all the time when looing back through annotate logs.)

> Source/WebCore/css/CSSStyleApplyProperty.cpp:64
> +    virtual void inherit(CSSStyleSelector* instance) const
> +    {
> +        (instance->style()->*m_setter)((instance->parentStyle()->*m_getter)());
> +    }

I worry a little about this double-indirection with the function pointer *and* the virtual function.  Wouldn't it be faster to skip the virtual function and always just use function pointers?  Then the subclasses would just set the function pointers to the appropriate types, but the "inherit", "intial" and "value" functions themselves would just always be like this "default" one and not be virtual?  Maybe I'm missing something the virtual step is adding.

I'm not sure what I mention above is better... but I'd be interested to hear your thoughts on the matter.

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

Oh, I see by "backup" you mean, "in the case the value isn't found?"  Seems that is the same as m_initial in the Default class above?  Shouldn't this just be called m_initital?  Or maybe m_inheritFrom (since it doesn't seem to be used in the initial() case?)

> Source/WebCore/css/CSSStyleApplyProperty.cpp:94
> +    virtual void inherit(CSSStyleSelector* instance) const

Nit:   "selector" is probably a more useful arg name than "instance".

> Source/WebCore/css/CSSStyleApplyProperty.cpp:104
> +        Color col;

"color" here.

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

I think we generally prefer early-return in WebKit.  I can't remember if I mentioned this before or not. :)

> Source/WebCore/css/CSSStyleApplyProperty.cpp:137
> +            CSSPrimitiveValue* p = static_cast<CSSPrimitiveValue*>(value);
> +            if (p->getIdent() == CSSValueCurrentcolor)

Seems we don't need the local for this cast.  Again, early return is preferred.  Full english names for variables are generally preferred too.  These are all nits of course, but add up to general style readability (which for better or worse is very different from Google's).

> Source/WebCore/css/CSSStyleApplyProperty.cpp:169
> +    setPropertyValue(CSSPropertyBorderBottomColor, new ApplyPropertyColorBase(&RenderStyle::borderBottomColor, &RenderStyle::color, &RenderStyle::setBorderBottomColor));
> +    setPropertyValue(CSSPropertyBorderLeftColor, new ApplyPropertyColorBase(&RenderStyle::borderLeftColor, &RenderStyle::color, &RenderStyle::setBorderLeftColor));
> +    setPropertyValue(CSSPropertyBorderRightColor, new ApplyPropertyColorBase(&RenderStyle::borderRightColor, &RenderStyle::color, &RenderStyle::setBorderRightColor));
> +    setPropertyValue(CSSPropertyBorderTopColor, new ApplyPropertyColorBase(&RenderStyle::borderTopColor, &RenderStyle::color, &RenderStyle::setBorderTopColor));
> +    setPropertyValue(CSSPropertyOutlineColor, new ApplyPropertyColorBase(&RenderStyle::outlineColor, &RenderStyle::color, &RenderStyle::setOutlineColor));
> +    setPropertyValue(CSSPropertyWebkitColumnRuleColor, new ApplyPropertyColorBase(&RenderStyle::columnRuleColor, &RenderStyle::color, &RenderStyle::setColumnRuleColor));
> +    setPropertyValue(CSSPropertyWebkitTextEmphasisColor, new ApplyPropertyColorBase(&RenderStyle::textEmphasisColor, &RenderStyle::color, &RenderStyle::setTextEmphasisColor));
> +    setPropertyValue(CSSPropertyWebkitTextFillColor, new ApplyPropertyColorBase(&RenderStyle::textFillColor, &RenderStyle::color, &RenderStyle::setTextFillColor));
> +    setPropertyValue(CSSPropertyWebkitTextStrokeColor, new ApplyPropertyColorBase(&RenderStyle::textStrokeColor, &RenderStyle::color, &RenderStyle::setTextStrokeColor));

It seems these still could be made cleaner with a helper.  Given how many things have to be pulled from RenderStyle it almost feels like these should be created by some method on RenderStyle.

The Base cases certainly could be some helper function:
colorPropertyWithBackup(CSSPropertyWebkitTextStrokeColor, textStrokeColor, setTextStrokeColor)
but of course that could only be that short if it was actually on RenderStyle.  I'm not sure how that would look.

This is much nicer than before though, thank you.

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