[Webkit-unassigned] [Bug 59774] Add template parameter to ApplyPropertyColor to improve clarity by removing constructor parameter side effects.

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Eric Seidel <eric at webkit.org>  2011-05-03 20:35:10 PST ---
(From update of attachment 92045)
View in context: https://bugs.webkit.org/attachment.cgi?id=92045&action=review

I have some rather nebulous random nits/questions:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:173
>              (selector->style()->*m_setter)(selector->getColorFromPrimitiveValue(primitiveValue));

I'm not a big fan of the direct use of function pointers.  But if this is the only use of m_setter, than maybe this is best. If we're using it more than once we should consider wrapping it in a helper to call m_setter for us.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:450
> +    setPropertyValue(CSSPropertyBackgroundColor, new ApplyPropertyColor<NoInheritFromParent>(&RenderStyle::backgroundColor, &RenderStyle::setBackgroundColor, 0));

I think we should consider giving the template a default value to inherit or not inherit, whichever is more common.

We should consider moving to the ::create() style as well at some point.

Are these OwnPtr's?  If so, we'll need to adopt() them to comply with strict own ptr rules.

We seem to have a bunch of repeated information between these lines.  Perhaps we can find a way to reduce the copy/paste so as to highlight the differences more?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:468
> +    setPropertyValue(CSSPropertyOutlineColor, new ApplyPropertyColor<InheritFromParent>(&RenderStyle::outlineColor, &RenderStyle::setOutlineColor, &RenderStyle::color));

I should probably know this by now, but I assume we have a single table for all of these?  Should we be concerned about the extra heap allocations moving this out of static data?  I suspect that given how huge the previous macro-based solution was, this is probably still a memory size win.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:474
> +    setPropertyValue(CSSPropertyWebkitColumnRuleColor, new ApplyPropertyColor<NoInheritFromParent>(&RenderStyle::columnRuleColor, &RenderStyle::setColumnRuleColor, &RenderStyle::color));

Maybe we should consider moving the creation of these onto RenderStyle so we could get rid of the RenderStyle:: copy/paste.

I'm trying to envison what the ideal code line would look like.  We're trying to map from a CSSPropertyName to a set of functions to apply it to the RenderStyle, right?  Obviously there is a little bit of logic as well, but that's encapsulated in the Apply* type.

Do these ever have more than 4 bits of data?  Should we do this using structs of some form?
{CSSPropertyWebkitColumnRuleColor, ApplyPropertyColor, columnRuleColor, setColumnRuleColor, color}

I'm not sure I'm a big enough fan of macros to suggest we macro some of this.  I'm not sure that would help.

Should we autogenerate this from some DSL?

This is probably all thinking too far ahead.

I think we seem to have general agreement that a function-pointer based solution is better than the previous huge switch statement with macros.

The question is what that solution looks like in the end.  One of my goals would be to make it as easily hackable/readable as possible in the final product.

Obviously you're not at the final product yet.  We have a lot of room to iterate.

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