[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





--- Comment #9 from Luke Macpherson <macpherson at chromium.org>  2011-04-26 19:27:39 PST ---
(From update of attachment 91049)
View in context: https://bugs.webkit.org/attachment.cgi?id=91049&action=review

>>> 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.
> 
> Just for the record, I asked for these things (typedef) after CSSStyleApplyProperty has landed nearly 2 months ago :(
> Luke, could you make those changes? I'm happy to review the patch...

Done. Previously these types were not used repeatedly and so it didn't make sense to typedef them. Now that these types are used in many places it does.

>> 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?

Removed.

>> 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.

Done everywhere.

>> 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.

Done.

>> 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.

Yes, just moved code. Added comment to reflect that negative border widths are invalid. I've moved the < 0 check into the switch statement as suggested.

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