65588 2011-08-02 18:44:00 -0700 Clean up value clamping in CSSStyleSelector. 2011-08-03 21:40:30 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 macpherson macpherson darin eric macpherson simon.fraser webkit.review.bot oldest_to_newest 445662 0 macpherson 2011-08-02 18:44:00 -0700 Clean up value clamping in CSSStyleSelector. 445697 1 102735 macpherson 2011-08-02 20:15:36 -0700 Created attachment 102735 Patch 446035 2 102735 darin 2011-08-03 13:00:48 -0700 Comment on attachment 102735 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. 446196 3 102735 macpherson 2011-08-03 16:24:11 -0700 Comment on attachment 102735 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. 446261 4 darin 2011-08-03 18:27:09 -0700 (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? 446273 5 macpherson 2011-08-03 18:42:49 -0700 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. 446302 6 102735 darin 2011-08-03 20:41:19 -0700 Comment on attachment 102735 Patch OK, Luke convinced me the patch is OK and fixes no bugs. 446305 7 macpherson 2011-08-03 20:47:28 -0700 Thanks, Darin! 446327 8 102735 webkit.review.bot 2011-08-03 21:40:25 -0700 Comment on attachment 102735 Patch Clearing flags on attachment: 102735 Committed r92349: <http://trac.webkit.org/changeset/92349> 446328 9 webkit.review.bot 2011-08-03 21:40:30 -0700 All reviewed patches have been landed. Closing bug. 102735 2011-08-02 20:15:36 -0700 2011-08-03 21:40:24 -0700 Patch bug-65588-20110803131534.patch text/plain 3102 macpherson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDkyMjU0KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDgtMDIgIEx1a2UgTWFj cGhlcnNvbiAgIDxtYWNwaGVyc29uQGNocm9taXVtLm9yZz4KKworICAgICAgICBDbGVhbiB1cCB2 YWx1ZSBjbGFtcGluZyBpbiBDU1NTdHlsZVNlbGVjdG9yLgorICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjU1ODgKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMgLyB0cml2aWFsIGNvZGUgY2xl YW51cCBvbmx5LgorCisgICAgICAgICogY3NzL0NTU1N0eWxlU2VsZWN0b3IuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6Q1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eSk6CisgICAgICAgIFJl cGxhY2UgKHVuc2lnbmVkIGludClwcmltaXRpdmVWYWx1ZS0+Z2V0RG91YmxlVmFsdWUoKSB3aXRo IHByaW10aXZlVmFsdWUtPmdldFZhbHVlPHVuc2lnbmVkPigpIGZvciBjb3JyZWN0IGNsYW1waW5n LgorICAgICAgICBSZXBsYWNlIGNsYW1wVG9JbnRlZ2VyKHByaW1pdGl2ZVZhbHVlLT5nZXREb3Vi bGVWYWx1ZSgpKSB3aXRoIHByaW1pdGl2ZVZhbHVlLT5nZXRJbnRWYWx1ZSgpLgorCiAyMDExLTA4 LTAxICBEYXZpZCBMZXZpbiAgPGxldmluQGNocm9taXVtLm9yZz4KIAogICAgICAgICBBZGQgYXNz ZXJ0cyB0byBSZWZDb3VudGVkIHRvIG1ha2Ugc3VyZSByZWYvZGVyZWYgaGFwcGVucyBvbiB0aGUg cmlnaHQgdGhyZWFkLgpJbmRleDogU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2VsZWN0b3Iu Y3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNlbGVjdG9yLmNw cAkocmV2aXNpb24gOTIyNTQpCisrKyBTb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTZWxlY3Rv ci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQ1MzksMTMgKzQ1MzksMTMgQEAgdm9pZCBDU1NTdHls ZVNlbGVjdG9yOjphcHBseVByb3BlcnR5KGludAogICAgICAgICBIQU5ETEVfSU5IRVJJVF9BTkRf SU5JVElBTChib3hGbGV4R3JvdXAsIEJveEZsZXhHcm91cCkKICAgICAgICAgaWYgKCFwcmltaXRp dmVWYWx1ZSB8fCBwcmltaXRpdmVWYWx1ZS0+cHJpbWl0aXZlVHlwZSgpICE9IENTU1ByaW1pdGl2 ZVZhbHVlOjpDU1NfTlVNQkVSKQogICAgICAgICAgICAgcmV0dXJuOyAvLyBFcnJvciBjYXNlLgot ICAgICAgICBtX3N0eWxlLT5zZXRCb3hGbGV4R3JvdXAoKHVuc2lnbmVkIGludCkocHJpbWl0aXZl VmFsdWUtPmdldERvdWJsZVZhbHVlKCkpKTsKKyAgICAgICAgbV9zdHlsZS0+c2V0Qm94RmxleEdy b3VwKHByaW1pdGl2ZVZhbHVlLT5nZXRWYWx1ZTx1bnNpZ25lZD4oKSk7CiAgICAgICAgIHJldHVy bjsKICAgICBjYXNlIENTU1Byb3BlcnR5V2Via2l0Qm94T3JkaW5hbEdyb3VwOgogICAgICAgICBI QU5ETEVfSU5IRVJJVF9BTkRfSU5JVElBTChib3hPcmRpbmFsR3JvdXAsIEJveE9yZGluYWxHcm91 cCkKICAgICAgICAgaWYgKCFwcmltaXRpdmVWYWx1ZSB8fCBwcmltaXRpdmVWYWx1ZS0+cHJpbWl0 aXZlVHlwZSgpICE9IENTU1ByaW1pdGl2ZVZhbHVlOjpDU1NfTlVNQkVSKQogICAgICAgICAgICAg cmV0dXJuOyAvLyBFcnJvciBjYXNlLgotICAgICAgICBtX3N0eWxlLT5zZXRCb3hPcmRpbmFsR3Jv dXAoKHVuc2lnbmVkIGludCkocHJpbWl0aXZlVmFsdWUtPmdldERvdWJsZVZhbHVlKCkpKTsKKyAg ICAgICAgbV9zdHlsZS0+c2V0Qm94T3JkaW5hbEdyb3VwKHByaW1pdGl2ZVZhbHVlLT5nZXRWYWx1 ZTx1bnNpZ25lZD4oKSk7CiAgICAgICAgIHJldHVybjsKICAgICBjYXNlIENTU1Byb3BlcnR5Qm94 U2l6aW5nOgogICAgICAgICBIQU5ETEVfSU5IRVJJVF9BTkRfSU5JVElBTChib3hTaXppbmcsIEJv eFNpemluZykKQEAgLTQ3MDcsNyArNDcwNyw3IEBAIHZvaWQgQ1NTU3R5bGVTZWxlY3Rvcjo6YXBw bHlQcm9wZXJ0eShpbnQKICAgICAgICAgcmV0dXJuOwogICAgIGNhc2UgQ1NTUHJvcGVydHlXZWJr aXRDb250ZW50T3JkZXI6CiAgICAgICAgIEhBTkRMRV9JTkhFUklUX0FORF9JTklUSUFMKHJlZ2lv bkluZGV4LCBSZWdpb25JbmRleCk7Ci0gICAgICAgIG1fc3R5bGUtPnNldFJlZ2lvbkluZGV4KGNs YW1wVG9JbnRlZ2VyKHByaW1pdGl2ZVZhbHVlLT5nZXREb3VibGVWYWx1ZSgpKSk7CisgICAgICAg IG1fc3R5bGUtPnNldFJlZ2lvbkluZGV4KHByaW1pdGl2ZVZhbHVlLT5nZXRJbnRWYWx1ZSgpKTsK ICAgICAgICAgcmV0dXJuOwogICAgIGNhc2UgQ1NTUHJvcGVydHlXZWJraXRSZWdpb25PdmVyZmxv dzoKICAgICAgICAgSEFORExFX0lOSEVSSVRfQU5EX0lOSVRJQUxfQU5EX1BSSU1JVElWRShyZWdp b25PdmVyZmxvdywgUmVnaW9uT3ZlcmZsb3cpOwpAQCAtNjQ1Nyw4ICs2NDU3LDcgQEAgYm9vbCBD U1NTdHlsZVNlbGVjdG9yOjpjcmVhdGVUcmFuc2Zvcm1PcAogICAgICAgICAgICAgICAgICAgICAv LyBUaGlzIGlzIGEgcXVpcmsgdGhhdCBzaG91bGQgZ28gYXdheSB3aGVuIDNkIHRyYW5zZm9ybXMg YXJlIGZpbmFsaXplZC4KICAgICAgICAgICAgICAgICAgICAgZG91YmxlIHZhbCA9IGZpcnN0VmFs dWUtPmdldERvdWJsZVZhbHVlKCk7CiAgICAgICAgICAgICAgICAgICAgIG9rID0gdmFsID49IDA7 Ci0gICAgICAgICAgICAgICAgICAgIHZhbCA9IGNsYW1wVG9Qb3NpdGl2ZUludGVnZXIodmFsKTsK LSAgICAgICAgICAgICAgICAgICAgcCA9IExlbmd0aChzdGF0aWNfY2FzdDxpbnQ+KHZhbCksIEZp eGVkKTsKKyAgICAgICAgICAgICAgICAgICAgcCA9IExlbmd0aChjbGFtcFRvUG9zaXRpdmVJbnRl Z2VyKHZhbCksIEZpeGVkKTsKICAgICAgICAgICAgICAgICB9CiAgICAgICAgICAgICAgICAgCiAg ICAgICAgICAgICAgICAgaWYgKCFvaykK