Bug 62957 - Introduce templated compute length class in CSSStyleApplyProperty, and use to handle several CSS properties.
Summary: Introduce templated compute length class in CSSStyleApplyProperty, and use to...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-19 18:40 PDT by Luke Macpherson
Modified: 2011-07-14 18:29 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.83 KB, patch)
2011-06-19 18:44 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (18.84 KB, patch)
2011-06-19 18:50 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (18.88 KB, patch)
2011-06-19 20:45 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (18.92 KB, patch)
2011-07-13 21:23 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-06-19 18:40:43 PDT
Introduce templated compute length class in CSSStyleApplyProperty, and use to handle several CSS properties.
Comment 1 Luke Macpherson 2011-06-19 18:44:22 PDT
Created attachment 97729 [details]
Patch
Comment 2 Luke Macpherson 2011-06-19 18:48:20 PDT
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.
Comment 3 Luke Macpherson 2011-06-19 18:50:19 PDT
Created attachment 97731 [details]
Patch
Comment 4 WebKit Review Bot 2011-06-19 20:00:06 PDT
Comment on attachment 97731 [details]
Patch

Attachment 97731 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8911342
Comment 5 Luke Macpherson 2011-06-19 20:45:25 PDT
Created attachment 97738 [details]
Patch
Comment 6 Eric Seidel (no email) 2011-06-19 20:48:00 PDT
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?
Comment 7 Luke Macpherson 2011-06-19 20:52:27 PDT
(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.
Comment 8 Luke Macpherson 2011-06-28 14:03:23 PDT
Ping. Patch still applies and builds cleanly.
Comment 9 Dimitri Glazkov (Google) 2011-07-13 19:07:55 PDT
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
Comment 10 Luke Macpherson 2011-07-13 21:23:28 PDT
Created attachment 100765 [details]
Patch
Comment 11 Dimitri Glazkov (Google) 2011-07-14 07:33:40 PDT
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.
Comment 12 WebKit Review Bot 2011-07-14 18:29:09 PDT
Comment on attachment 100765 [details]
Patch

Clearing flags on attachment: 100765

Committed r91038: <http://trac.webkit.org/changeset/91038>
Comment 13 WebKit Review Bot 2011-07-14 18:29:14 PDT
All reviewed patches have been landed.  Closing bug.