[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