[webkit-reviews] review denied: [Bug 74635] getComputedStyle for border-width is not implemented. : [Attachment 119469] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 74635: getComputedStyle for border-width is not implemented.
https://bugs.webkit.org/show_bug.cgi?id=74635

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119469&action=review


> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2166
> +	       RefPtr<CSSValueList> list =
CSSValueList::createSpaceSeparated();
> +	       list->append(zoomAdjustedPixelValue(style->borderTop().width(),
style.get(), cssValuePool));

When I added this for flex-flow, I didn't realize that getComputedStyle wasn't
implemented for other short hand properties.  I think adding this is a good
idea, but we should make some helper functions for this rather than duplicating
code (e.g., the code for margin width would be almost identical).

> LayoutTests/ChangeLog:11
> +	   *
fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt: Added.
> +	   * fast/css/getComputedStyle/getComputedStyle-border-width.html:
Added.

Do you need to also fix up fast/css/getComputedStyle/computed-style* and
svg/css/getComputedStyle-basic?

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width.html:16
> +testContainer.innerHTML = '<div id="test" style="border-width: 10px 5px 4px
3px; border-style: solid solid;">hello</div>';

Can we have some other units in the test case?	It would also be interesting to
see the setting of border-top and making sure the value makes it out when
calling getComputedStyle on border-width.