Bug 59623 – Cleanup CSSStyleApplyProperty.cpp

• Component: CSS
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Cleanup CSSStyleApplyProperty.cpp, it's pretty hard to read as is, and uses too many Base classes.
As first step remove ApplyPropertyColorBase, merge it with ApplyPropertyColor.
Use typedefs for the function pointers where possible!

[Nikolas Zimmermann]

Created attachment 91316 [details]
Patch

[Antti Koivisto]

r=me

[Nikolas Zimmermann]

Landed in r85069.

[Luke Macpherson]

Was this just the typedef code taken from my patch https://bug-59414-attachments.webkit.org/attachment.cgi?id=91221 ?

[Luke Macpherson]

View in context: https://bugs.webkit.org/attachment.cgi?id=91316&action=review

Overall I like the cleanup. Thanks.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:147
> +        (selector->style()->*m_setter)(m_initial ? m_initial() : Color());

Looks good.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:155
> +        if (m_initial && primitiveValue->getIdent() == CSSValueCurrentcolor)

Use of m_initial here seems to add an odd side-effect to that parameter. Perhaps this would be better handled by adding another constructor value "inheritColorFromParent = true".

[Nikolas Zimmermann]

(In reply to comment #4)
> Was this just the typedef code taken from my patch https://bug-59414-attachments.webkit.org/attachment.cgi?id=91221 ?
I started from scratch, I forgot that you partly fixed that problem in your patch!

(In reply to comment #5)
> View in context: https://bugs.webkit.org/attachment.cgi?id=91316&action=review
> 
> Overall I like the cleanup. Thanks.
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:147
> > +        (selector->style()->*m_setter)(m_initial ? m_initial() : Color());
> 
> Looks good.
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:155
> > +        if (m_initial && primitiveValue->getIdent() == CSSValueCurrentcolor)
> 
> Use of m_initial here seems to add an odd side-effect to that parameter. Perhaps this would be better handled by adding another constructor value "inheritColorFromParent = true".
That's true, if you want to modify it I'm happy to review!