[webkit-reviews] review denied: [Bug 54707] Introduce lookup-table based approach for applying CSS properties. : [Attachment 83271] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 54707: Introduce lookup-table based approach for applying CSS properties.
https://bugs.webkit.org/show_bug.cgi?id=54707

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=83271&action=review

> Source/WebCore/css/CSSStyleApplyProperty.cpp:48
> +	   : m_prop(prop)

I would have written out m_prop as m_property.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:54
> +    virtual void inherit(CSSStyleSelector *instance) const = 0;

arg names aren't useful here and should be removed.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:67
> +    virtual void inherit(CSSStyleSelector *instance) const
{UNUSED_PARAM(instance);}

Better to not name the instance parameter at all.  Also the * is in the wrong
palce.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:134
> +	   if (value->isPrimitiveValue()) {

I would have used early return.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:184
> +    m_propertyMap[PROPERTY_INDEX(CSSPropertyDirection)] = m_invalid;

This should just be a helper function.

markInvalid(CSSPropertyName);  Or you could put them in a static array and use
a for loop.  your choice.