[webkit-reviews] review denied: [Bug 54707] Introduce lookup-table based approach for applying CSS properties. : [Attachment 84983] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 54707: Introduce lookup-table based approach for applying CSS properties.
https://bugs.webkit.org/show_bug.cgi?id=54707

Attachment 84983: Patch
https://bugs.webkit.org/attachment.cgi?id=84983&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.