Bug 72926 – Implement vertical-align property in CSSStyleApplyProperty.

• Component: CSS
• Status: RESOLVED FIXED

[Luke Macpherson]

Implement vertical-align property in CSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 116173 [details]
Patch

[Andreas Kling]

Comment on attachment 116173 [details]
Patch

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.

[Luke Macpherson]

Comment on attachment 116173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116173&action=review

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

done.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:845
>> +            return selector->style()->setVerticalAlign(*primitiveValue);
> 
> I'm not sure this kind of "void return chaining" is allowed by the WebKit coding style.

I checked the style guide and it isn't mentioned anywhere. I don't think void should be treated differently to any other type from a style perspective, and it is still useful to denote that it is a tail call.

>> Source/WebCore/rendering/style/RenderStyle.h:982
>> +    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.

done.

[Luke Macpherson]

Created attachment 116685 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116685 [details]
Patch for landing

Clearing flags on attachment: 116685

Committed r101225: <http://trac.webkit.org/changeset/101225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.