[Webkit-unassigned] [Bug 59414] Implement CSS border width and related properties in CSSStyleApplyProperty.

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #91049|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2011-04-26 09:43:34 PST ---
(From update of attachment 91049)
View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review

Do we have some performance testing results for this change?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:87
> +    ApplyPropertyDefaultBase(T (RenderStyle::*getter)() const, void (RenderStyle::*setter)(T), T (*initial)())

These pointer-to-member-function and function types are a little hard to read. Maybe we could use typedefs instead of putting the type here directly to make the code easier to read.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:104
> +    virtual void applyValue(CSSStyleSelector*, CSSValue*) const {}

Can this be a pure virtual function instead? Is the empty version of the function ever helpful?

> Source/WebCore/css/CSSStyleApplyProperty.cpp:121
> +    virtual void applyValue(CSSStyleSelector* selector, CSSValue* value) const

I normally prefer to make a function like this private since it's only intended to be called through the base class. Making it private potentially catches virtual function calls that could instead be more-direct calls.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:124
> +            (selector->style()->*ApplyPropertyDefaultBase<T>::m_setter)(*(static_cast<CSSPrimitiveValue*>(value)));

The extra set of parentheses here around the static_cast makes this slightly harder to read.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:328
> +        if (width >= 0)

I think it would be worthwhile to have a “why” comment about this, although it looks like you just moved the code.

Also, this if could go inside the CSSValueInvalid case of the switch rather than out here.

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