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+

Nikolas Zimmermann
Reported 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!
Attachments
Patch (11.01 KB, patch)
2011-04-27 11:39 PDT, Nikolas Zimmermann 		koivisto: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2011-04-27 11:39:53 PDT
Created attachment 91316 [details] Patch
Antti Koivisto
Comment 2 2011-04-27 11:43:40 PDT
r=me
Nikolas Zimmermann
Comment 3 2011-04-27 11:44:24 PDT
Landed in r85069.
Luke Macpherson
Comment 4 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 ?
Luke Macpherson
Comment 5 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".
Nikolas Zimmermann
Comment 6 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!
Note You need to log in before you can comment on or make changes to this bug.