I had a problem getting the computed style of a borderWidth in webkit. It looks like a bug. testGetComputedShorthandValues100px : function() { var c1 = document.getElementById("c1"), inp = "120em 110em 100em 90.1em"; c1.style.borderWidth = inp; c1.style.borderStyle = "solid"; c1.style.fontSize = "100px"; var cs = getComputedStyle(c1, ""); //alert([cs.borderTopWidth, cs.borderRightWidth, cs.borderBottomWidth, cs.borderLeftWidth]); var out = dom.getStyle(c1, "borderWidth"); Assert.areEqual("12000px 11000px 10000px 9010px", out); } Safari 3.1 fails with "3808px 2808px 1808px 818px, (string)" The html can be simplified to: <body style="position: relative;margin:0;padding:0;"><div id="c1"></ div>l</body> In Firefox, I get the expexted result. I don't understand webkit's result. What's going on?
Created attachment 21925 [details] First attempt This simple patch allows bigger computed border widths that use em. (for some reason the WebCore/ChangeLog) did not pick up the tests). Cheers, Rob.
Comment on attachment 21925 [details] First attempt Why is 28 bits enough? Doesn't this just move the problem to a higher number?
Hi Darin, (In reply to comment #2) > (From update of attachment 21925 [details] [edit]) > Why is 28 bits enough? Doesn't this just move the problem to a higher number? Sure, just moving the problem, but we have to have some boundary I think. There is something wrong with the patch I noticed, when setting we use a short so the extra room gets lost. When I correct that the value can go up to 2684354em at 100px font size, which is more than FF. However it seems the zoom factor is also multiplied into the border widths, which means the border width values differ on zoom level, unlike FF. I am not sure what is correct. Cheers, Rob.
Comment on attachment 21925 [details] First attempt It would be even better if this was a .js style test and it tested both sides of the expected failure point.
Comment on attachment 21925 [details] First attempt r- for needing a better test case.
Created attachment 122444 [details] Patch
Created attachment 122486 [details] Patch
Comment on attachment 122486 [details] Patch 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.
(In reply to comment #8) > (From update of attachment 122486 [details]) > 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. I can add it. > > 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. It's because the template requires a unsigned short defined function. I changed initialBorderWidth to return a unsigned therefore it doesn't compile. I created initialOutlineWidth which is unsigned short (and return the same as initialBorderWidth) for it. > > > 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? I guess no as we still have properties using short. > > > 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. why exactly?
Created attachment 122893 [details] Patch
Comment on attachment 122893 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122893&action=review > Source/WebCore/css/CSSStyleApplyProperty.cpp:1706 > + setPropertyHandler(CSSPropertyWebkitColumnRuleWidth, ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth, &RenderStyle::setColumnRuleWidth, &RenderStyle::initialOutlineWidth, NormalDisabled, ThicknessEnabled>::createHandler()); Can we also add a method to render style for initialColumnRuleWidth? It's confusing to use initialOutlineWidth here.
Created attachment 122980 [details] Patch for landing
Comment on attachment 122980 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=122980&action=review > Source/WebCore/platform/animation/AnimationUtilities.h:40 > + return static_cast<int>(lround(static_cast<double>(from) + static_cast<double>(to - from) * progress)); Shouldn't this use static_cast<unsigned> instead?
Comment on attachment 122980 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=122980&action=review >> Source/WebCore/platform/animation/AnimationUtilities.h:40 >> + return static_cast<int>(lround(static_cast<double>(from) + static_cast<double>(to - from) * progress)); > > Shouldn't this use static_cast<unsigned> instead? Yes, you're right.
Created attachment 123097 [details] Patch for landing
Comment on attachment 123097 [details] Patch for landing Clearing flags on attachment: 123097 Committed r105403: <http://trac.webkit.org/changeset/105403>
All reviewed patches have been landed. Closing bug.