[webkit-reviews] review denied: [Bug 18294] Strange Result for getComputedStyle on borderWidth set in em : [Attachment 122486] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Alexis Menard (darktears)
<alexis.menard at openbossa.org>'s request for review:
Bug 18294: Strange Result for getComputedStyle on borderWidth set in em
https://bugs.webkit.org/show_bug.cgi?id=18294

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

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


I think you're right that it's safe to use more bits because the size isn't
changing (we're probably 4 byte aligning), but we should add a compile assert
to verify.

Color also packs poorly because of the bool it contains, but there's probably
no way to get that back.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1710
> +    setPropertyHandler(CSSPropertyOutlineWidth,
ApplyPropertyComputeLength<unsigned short, &RenderStyle::outlineWidth,
&RenderStyle::setOutlineWidth, &RenderStyle::initialOutlineWidth,
NormalDisabled, ThicknessEnabled>::createHandler());
> +    setPropertyHandler(CSSPropertyWebkitColumnRuleWidth,
ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth,
&RenderStyle::setColumnRuleWidth, &RenderStyle::initialOutlineWidth,
NormalDisabled, ThicknessEnabled>::createHandler());

Should this be initialColumnWidth or initialColumnRuleWidth?  This fix seems
unrelated to this patch.

> Source/WebCore/page/animation/AnimationBase.cpp:87
> +static inline unsigned blendFunc(const AnimationBase*, unsigned from,
unsigned to, double progress)

Is there a version of this function using "unsigned short" that we can remove?

> Source/WebCore/rendering/RenderTheme.cpp:114
> +	       if (static_cast<unsigned>(borderBox.top().value()) !=
style->borderTopWidth()) {

I would cast border*Width to an int because it's going to fit (since it's only
27 bits) and Length::value() could be negative.  That said, this still makes me
a bit nervous.