Clean up value clamping in CSSStyleSelector.

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Clean up value clamping in CSSStyleSelector.

[Luke Macpherson]

Created attachment 102735 [details]
Patch

[Darin Adler]

Comment on attachment 102735 [details]
Patch

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.

[Luke Macpherson]

Comment on attachment 102735 [details]
Patch

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.

[Darin Adler]

(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?

[Luke Macpherson]

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.

[Darin Adler]

Comment on attachment 102735 [details]
Patch

OK, Luke convinced me the patch is OK and fixes no bugs.

[Luke Macpherson]

Thanks, Darin!

[WebKit Review Bot]

Comment on attachment 102735 [details]
Patch

Clearing flags on attachment: 102735

Committed r92349: <http://trac.webkit.org/changeset/92349>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.