Bug 72926 - Implement vertical-align property in CSSStyleApplyProperty.
Summary: Implement vertical-align property in CSSStyleApplyProperty.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-21 20:41 PST by Luke Macpherson
Modified: 2011-11-27 23:08 PST (History)
5 users (show)

See Also:

Attachments
Patch (7.09 KB, patch)
2011-11-21 20:46 PST, Luke Macpherson 		no flags Details | Formatted Diff | Diff
Patch for landing (7.18 KB, patch)
2011-11-27 16:29 PST, Luke Macpherson 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-11-21 20:41:51 PST 
Implement vertical-align property in CSSStyleApplyProperty.
Comment 1 Luke Macpherson 2011-11-21 20:46:52 PST 
Created attachment 116173 [details]
Patch
Comment 2 Andreas Kling 2011-11-25 09:09:14 PST 
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.
Comment 3 Luke Macpherson 2011-11-27 16:24:15 PST 
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.
Comment 4 Luke Macpherson 2011-11-27 16:29:03 PST 
Created attachment 116685 [details]
Patch for landing
Comment 5 WebKit Review Bot 2011-11-27 23:08:52 PST 
Comment on attachment 116685 [details]
Patch for landing

Clearing flags on attachment: 116685

Committed r101225: <http://trac.webkit.org/changeset/101225>
Comment 6 WebKit Review Bot 2011-11-27 23:08:56 PST 
All reviewed patches have been landed.  Closing bug.