Bug 65588 - Clean up value clamping in CSSStyleSelector.
: Clean up value clamping in CSSStyleSelector.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Luke Macpherson
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-02 18:44 PDT by Luke Macpherson
Modified: 2011-08-03 21:40 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.03 KB, patch)
2011-08-02 20:15 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-08-02 18:44:00 PDT
Clean up value clamping in CSSStyleSelector.
Comment 1 Luke Macpherson 2011-08-02 20:15:36 PDT
Created attachment 102735 [details]
Patch
Comment 2 Darin Adler 2011-08-03 13:00:48 PDT
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.
Comment 3 Luke Macpherson 2011-08-03 16:24:11 PDT
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.
Comment 4 Darin Adler 2011-08-03 18:27:09 PDT
(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 Luke Macpherson 2011-08-03 18:42:49 PDT
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 Darin Adler 2011-08-03 20:41:19 PDT
Comment on attachment 102735 [details]
Patch

OK, Luke convinced me the patch is OK and fixes no bugs.
Comment 7 Luke Macpherson 2011-08-03 20:47:28 PDT
Thanks, Darin!
Comment 8 WebKit Review Bot 2011-08-03 21:40:25 PDT
Comment on attachment 102735 [details]
Patch

Clearing flags on attachment: 102735

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