Bug 59623

Summary: Cleanup CSSStyleApplyProperty.cpp
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: CSSAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, koivisto, macpherson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 54707    
Bug Blocks: 59774    
Attachments:
Description Flags
Patch koivisto: review+

Description Nikolas Zimmermann 2011-04-27 11:37:42 PDT
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!
Comment 1 Nikolas Zimmermann 2011-04-27 11:39:53 PDT
Created attachment 91316 [details]
Patch
Comment 2 Antti Koivisto 2011-04-27 11:43:40 PDT
r=me
Comment 3 Nikolas Zimmermann 2011-04-27 11:44:24 PDT
Landed in r85069.
Comment 4 Luke Macpherson 2011-04-27 15:26:33 PDT
Was this just the typedef code taken from my patch https://bug-59414-attachments.webkit.org/attachment.cgi?id=91221 ?
Comment 5 Luke Macpherson 2011-04-27 17:24:43 PDT
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".
Comment 6 Nikolas Zimmermann 2011-04-28 04:25:10 PDT
(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!