Bug 62957 – Introduce templated compute length class in CSSStyleApplyProperty, and use to handle several CSS properties.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Introduce templated compute length class in CSSStyleApplyProperty, and use to handle several CSS properties.

[Luke Macpherson]

Created attachment 97729 [details]
Patch

[Luke Macpherson]

Note that this patch causes negative border widths to clamp to zero, which is equivalent to the current behavior of ignoring negative values and using the default border width of zero, but avoids the conversions between short and unsigned short and additional checks for negative computed lengths that were present in the existing code.

[Luke Macpherson]

Created attachment 97731 [details]
Patch

[WebKit Review Bot]

Comment on attachment 97731 [details]
Patch

Attachment 97731 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8911342

[Luke Macpherson]

Created attachment 97738 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 97738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97738&action=review

> Source/WebCore/css/CSSStyleApplyProperty.cpp:395
> +template <typename T,
> +          ComputeLengthNormal normalEnabled = NormalDisabled,
> +          ComputeLengthThickness thicknessEnabled = ThicknessDisabled,
> +          ComputeLengthSVGZoom svgZoomEnabled = SVGZoomDisabled>

Really?  There isn't a better way to do this than 3 independent bools?

[Luke Macpherson]

(In reply to comment #6)
> (From update of attachment 97738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97738&action=review
> 
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:395
> > +template <typename T,
> > +          ComputeLengthNormal normalEnabled = NormalDisabled,
> > +          ComputeLengthThickness thicknessEnabled = ThicknessDisabled,
> > +          ComputeLengthSVGZoom svgZoomEnabled = SVGZoomDisabled>
> 
> Really?  There isn't a better way to do this than 3 independent bools?

Well, feel free to suggest one. I think it makes very good sense to do it that way, since they are orthogonal properties that you might want to switch on and off for individual CSS properties depending on what the spec allows.

[Luke Macpherson]

Ping. Patch still applies and builds cleanly.

[Dimitri Glazkov (Google)]

Comment on attachment 97738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97738&action=review

> Source/WebCore/ChangeLog:12
> +        Add computeLenght function that returns unsigned short.

computeLength

[Luke Macpherson]

Created attachment 100765 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 100765 [details]
Patch

you can just use webkit-patch land-safely next time. It will transfer the r+ from the previous patch and cq+ the new one.

[WebKit Review Bot]

Comment on attachment 100765 [details]
Patch

Clearing flags on attachment: 100765

Committed r91038: <http://trac.webkit.org/changeset/91038>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.