[webkit-reviews] review granted: [Bug 59414] Implement CSS border width and related properties in CSSStyleApplyProperty. : [Attachment 91049] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 59414: Implement CSS border width and related properties in
CSSStyleApplyProperty.
https://bugs.webkit.org/show_bug.cgi?id=59414

Attachment 91049: Patch
https://bugs.webkit.org/attachment.cgi?id=91049&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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<CSSPr
imitiveValue*>(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.