Bug 59623 - Cleanup CSSStyleApplyProperty.cpp
Summary: Cleanup CSSStyleApplyProperty.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 54707
Blocks: 59774
  Show dependency treegraph
 
Reported: 2011-04-27 11:37 PDT by Nikolas Zimmermann
Modified: 2011-04-28 22:07 PDT (History)
3 users (show)

See Also:


Attachments
Patch (11.01 KB, patch)
2011-04-27 11:39 PDT, Nikolas Zimmermann
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!