[webkit-reviews] review denied: [Bug 13709] Table border doesn't show up : [Attachment 196486] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 5 04:38:54 PDT 2013
Antti Koivisto <koivisto at iki.fi> has denied Suchit Agrawal
<a.suchit at samsung.com>'s request for review:
Bug 13709: Table border doesn't show up
https://bugs.webkit.org/show_bug.cgi?id=13709
Attachment 196486: Patch
https://bugs.webkit.org/attachment.cgi?id=196486&action=review
------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=196486&action=review
> Source/WebCore/css/StyleBuilder.cpp:625
> enum ComputeLengthSVGZoom {SVGZoomDisabled = 0, SVGZoomEnabled};
> template <typename T,
> T (RenderStyle::*getterFunction)() const,
> void (RenderStyle::*setterFunction)(T),
> T (*initialFunction)(),
> ComputeLengthNormal normalEnabled = NormalDisabled,
> - ComputeLengthThickness thicknessEnabled = ThicknessDisabled,
> ComputeLengthSVGZoom svgZoomEnabled = SVGZoomDisabled>
> -class ApplyPropertyComputeLength {
> +class ApplyPropertyComputeTextSpacing {
- text spacing properties are always int so T can be dropped
- svgZoomEnabled and normalEnabled are always true for text spacing properties.
Those can be dropped.
> Source/WebCore/css/StyleBuilder.cpp:669
> +template <typename T,
> + T (RenderStyle::*getterFunction)() const,
> + void (RenderStyle::*setterFunction)(T),
> + T (*initialFunction)()>
> +class ApplyPropertyComputeTransformOriginZ {
This is always float. No need for T.
> Source/WebCore/css/StyleBuilder.cpp:2200
> - setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing,
ApplyPropertyComputeLength<short, &RenderStyle::horizontalBorderSpacing,
&RenderStyle::setHorizontalBorderSpacing,
&RenderStyle::initialHorizontalBorderSpacing>::createHandler());
> + setPropertyHandler(CSSPropertyWebkitBorderHorizontalSpacing,
ApplyPropertyComputeTextSpacing<short, &RenderStyle::horizontalBorderSpacing,
&RenderStyle::setHorizontalBorderSpacing,
&RenderStyle::initialHorizontalBorderSpacing>::createHandler());
> setPropertyHandler(CSSPropertyWebkitBorderImage,
ApplyPropertyBorderImage<BorderImage, CSSPropertyWebkitBorderImage,
&RenderStyle::borderImage, &RenderStyle::setBorderImage>::createHandler());
> - setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing,
ApplyPropertyComputeLength<short, &RenderStyle::verticalBorderSpacing,
&RenderStyle::setVerticalBorderSpacing,
&RenderStyle::initialVerticalBorderSpacing>::createHandler());
> + setPropertyHandler(CSSPropertyWebkitBorderVerticalSpacing,
ApplyPropertyComputeTextSpacing<short, &RenderStyle::verticalBorderSpacing,
&RenderStyle::setVerticalBorderSpacing,
&RenderStyle::initialVerticalBorderSpacing>::createHandler());
These are not text spacing properties so shouldn't use
ApplyPropertyComputeTextSpacing. I suspect they can just use
ApplyPropertyComputeBorderWidth with the new rounding.
> Source/WebCore/css/StyleBuilder.cpp:2221
> - setPropertyHandler(CSSPropertyWebkitColumnRuleWidth,
ApplyPropertyComputeLength<unsigned short, &RenderStyle::columnRuleWidth,
&RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth,
NormalDisabled, ThicknessEnabled>::createHandler());
> + setPropertyHandler(CSSPropertyWebkitColumnRuleWidth,
ApplyPropertyComputeBorderWidth<unsigned short, &RenderStyle::columnRuleWidth,
&RenderStyle::setColumnRuleWidth, &RenderStyle::initialColumnRuleWidth,
ThicknessEnabled>::createHandler());
This too.
> LayoutTests/fast/borders/border-width-less-then-a-unit-of-pt.html:38
> +<div style="border-width:.8pt">There should be a border around this
text.</div>
> +<br/>
> +<table style="border-width:1.3pt">
> + <tr>
> + <td>Table border should present.</td>
> + </tr>
> +</table>
The tests should cover all properties where behavior changes (outline-width,
etc).
More information about the webkit-reviews
mailing list