Bug 65588 - Clean up value clamping in CSSStyleSelector.
: Clean up value clamping in CSSStyleSelector.
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-08-02 18:44 PST by
Modified: 2011-08-03 21:40 PST (History)


Attachments
Patch (3.03 KB, patch)
2011-08-02 20:15 PST, Luke Macpherson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-02 18:44:00 PST
Clean up value clamping in CSSStyleSelector.
------- Comment #1 From 2011-08-02 20:15:36 PST -------
Created an attachment (id=102735) [details]
Patch
------- Comment #2 From 2011-08-03 13:00:48 PST -------
(From update of attachment 102735 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=102735&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests / trivial code cleanup only.

This isn’t just code cleanup. It fixes clamping bugs. We need test cases showing the bad behavior changing to the good behavior. I know it can be frustrating to see an obvious mistake in the code, easy to fix, and then be asked to write test cases to show the bug, but this is one of those cases.

> Source/WebCore/css/CSSStyleSelector.cpp:6460
> -                    val = clampToPositiveInteger(val);
> -                    p = Length(static_cast<int>(val), Fixed);
> +                    p = Length(clampToPositiveInteger(val), Fixed);

This one is just code cleanup. The other three are bug fixes.
------- Comment #3 From 2011-08-03 16:24:11 PST -------
(From update of attachment 102735 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=102735&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests / trivial code cleanup only.
> 
> This isn’t just code cleanup. It fixes clamping bugs. We need test cases showing the bad behavior changing to the good behavior. I know it can be frustrating to see an obvious mistake in the code, easy to fix, and then be asked to write test cases to show the bug, but this is one of those cases.

CSSParser.cpp:1525 (validPrimitive = validUnit(value, FInteger | FNonNeg, true);) prevents the parser from accepting non-negative and non-integer values. So even though this looks like a behavioral change, the set of values held in the CSSPrimitiveValue should already be within the clamping range.
------- Comment #4 From 2011-08-03 18:27:09 PST -------
(In reply to comment #3)
> CSSParser.cpp:1525 (validPrimitive = validUnit(value, FInteger | FNonNeg, true);) prevents the parser from accepting non-negative and non-integer values. So even though this looks like a behavioral change, the set of values held in the CSSPrimitiveValue should already be within the clamping range.

Then wouldn’t we want assertions instead of clamping?
------- Comment #5 From 2011-08-03 18:42:49 PST -------
Asserts wouldn't hurt, but right now most properties do a clamp to reverse the cast-to-double that happens in CSSPrimitiveValue. You could argue that's extra work, but that's how it is.

What I'm working towards here is normalizing access to PrimitiveValue rather than having every property do its own thing. Having all the properties use the same pattern is a good thing in itself because bugs are easier to spot, but my underlying motivation is that it should allow all numeric properties to collapse into a single templated implementation in CSSStyleApplyProperty, which should give us a substantial reduction in LOC.
------- Comment #6 From 2011-08-03 20:41:19 PST -------
(From update of attachment 102735 [details])
OK, Luke convinced me the patch is OK and fixes no bugs.
------- Comment #7 From 2011-08-03 20:47:28 PST -------
Thanks, Darin!
------- Comment #8 From 2011-08-03 21:40:25 PST -------
(From update of attachment 102735 [details])
Clearing flags on attachment: 102735

Committed r92349: <http://trac.webkit.org/changeset/92349>
------- Comment #9 From 2011-08-03 21:40:30 PST -------
All reviewed patches have been landed.  Closing bug.