[webkit-reviews] review granted: [Bug 72926] Implement vertical-align property in CSSStyleApplyProperty. : [Attachment 116173] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Nov 25 09:09:13 PST 2011


Andreas Kling <kling at webkit.org> has granted Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 72926: Implement vertical-align property in CSSStyleApplyProperty.
https://bugs.webkit.org/show_bug.cgi?id=72926

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116173&action=review


r=me with some tweaks:

> Source/WebCore/css/CSSPrimitiveValue.h:123
> +    bool isPercent() const { return m_primitiveUnitType ==
CSSPrimitiveValue::CSS_PERCENTAGE; }

This should be called isPercentage().

> Source/WebCore/css/CSSStyleApplyProperty.cpp:845
> +	   if (primitiveValue->getIdent())
> +	       return selector->style()->setVerticalAlign(*primitiveValue);

I'm not sure this kind of "void return chaining" is allowed by the WebKit
coding style.

> Source/WebCore/rendering/style/RenderStyle.h:982
> -    void setVerticalAlignLength(Length l) { SET_VAR(m_box, m_verticalAlign,
l) }
> +    void setVerticalAlignLength(Length length) { setVerticalAlign(LENGTH);
SET_VAR(m_box, m_verticalAlign, length) }

While I agree with this change, I'd like to see it mentioned in the ChangeLog.


More information about the webkit-reviews mailing list