134798 2014-07-09 22:44:14 -0700 [iOS][WK2] Fix withinEpsilon() 2014-07-14 14:02:49 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 benjamin benjamin darin zalan oldest_to_newest 1021568 0 benjamin 2014-07-09 22:44:14 -0700 [iOS][WK2] Fix withinEpsilon() 1021569 1 234691 benjamin 2014-07-09 22:51:58 -0700 Created attachment 234691 Patch 1021571 2 benjamin 2014-07-09 22:56:26 -0700 Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers. 1021616 3 zalan 2014-07-10 08:15:32 -0700 (In reply to comment #2) > Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers. Oh, probably Darin wants to look at it too. 1021635 4 darin 2014-07-10 09:45:02 -0700 (In reply to comment #2) > Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers. Not sure where you looked. I found this code in /usr/include/c++/4.2.1/cmath: inline double abs(double __x) { return __builtin_fabs(__x); } inline float abs(float __x) { return __builtin_fabsf(__x); } inline long double abs(long double __x) { return __builtin_fabsl(__x); } 1021640 5 234691 darin 2014-07-10 09:47:47 -0700 Comment on attachment 234691 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234691&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:792 > + return fabs(a - b) < std::numeric_limits<float>::epsilon(); We definitely don’t want fabs when the argument is a float. If you can’t use std::abs for this (I absolutely think you can), you can overload yourself: inline double overloadedAbs(double number) { return fabs(number); } inline float overloadedAbs(float number) { return fabsf(number); } return overloadedAbs(a - b) < std::numeric_limits<float>::epsilon(); Also, I think this needs a comment explaining why we use the float epsilon rather than the epsilon of the type of the return value of fabs. The use of the float epsilon even when comparing double values is something we are doing intentionally for a good reason, but a non-obvious technique that needs a why comment. 1021704 6 benjamin 2014-07-10 13:57:07 -0700 (In reply to comment #4) > (In reply to comment #2) > > Darin, you mentioned that std::abs() should do the trick but I could not find any definition taking floating points in the headers. > > Not sure where you looked. I found this code in /usr/include/c++/4.2.1/cmath: I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :( I'll update the patch. 1021720 7 darin 2014-07-10 15:51:53 -0700 (In reply to comment #6) > I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :( You should search for std::abs elsewhere in WebKit, then. I’m pretty sure that David Kilzer landed a patch that uses std::abs on floating point numbers recently, and if it’s truncating to integers, it would be good to know! Maybe it’s depending on whether we include <cmath>? 1021727 8 234729 benjamin 2014-07-10 16:19:54 -0700 Created attachment 234729 Patch 1021732 9 benjamin 2014-07-10 16:22:44 -0700 (In reply to comment #7) > (In reply to comment #6) > > I used Xcode to follow the symbol std::abs(double) and it sent me to stdlib... :( > > You should search for std::abs elsewhere in WebKit, then. I’m pretty sure that David Kilzer landed a patch that uses std::abs on floating point numbers recently, and if it’s truncating to integers, it would be good to know! > > Maybe it’s depending on whether we include <cmath>? I changed to std::abs(), then inspected the assembly: everything is done with floating point. It is probably just Xcode not resolving the right symbol. 1021750 10 234729 darin 2014-07-10 17:39:16 -0700 Comment on attachment 234729 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=234729&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:792 > + return std::abs(a - b) < std::numeric_limits<float>::epsilon(); I still think a comment is needed here to explain why it’s float epsilon rather than the epsilon for the type of the result of a - b. 1022306 11 benjamin 2014-07-14 14:02:49 -0700 Committed r171078: <http://trac.webkit.org/changeset/171078> 234691 2014-07-09 22:51:58 -0700 2014-07-10 16:19:52 -0700 Patch bug-134798-20140709225131.patch text/plain 2411 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTcwOTUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNTNmOGU2NDYzYTI0MDU5 YWIyMmE4ZGYwY2YxZGQzNjY1NDc2NDAwOC4uOWQwNzdjNjUwMjM5NGQ2YjcxYjUzYThlYjczNjQ1 ZmNlZWVlZDY5YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIyIEBACiAyMDE0LTA3LTA5ICBCZW5q YW1pbiBQb3VsYWluICA8YnBvdWxhaW5AYXBwbGUuY29tPgogCisgICAgICAgIFtpT1NdW1dLMl0g Rml4IHdpdGhpbkVwc2lsb24oKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTM0Nzk4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgTW92ZSB0aGUgZnVuY3Rpb24gYmFjayB0byBXS1dlYlZpZXcsIGl0IGlzIG5v IGxvbmdlciBuZWVkZWQgaW4gV0tDb250ZW50Vmlldy4KKworICAgICAgICBVc2UgdGhlIHJlYWwg dHlwZXMgYXMgaW5wdXQgdG8gcHJvcGVybHkgdmVyaWZ5IHRoYXQgdGhlIHR3byBpbnB1dHMgYXJl IHdpdGhpbgorICAgICAgICBhIHNtYWxsIHZhbHVlIG9mIHRoZSAzMmJpdCBmbG9hdGluZyBwb2lu dC4KKworICAgICAgICAqIFVJUHJvY2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tOgorICAgICAg ICAod2l0aGluRXBzaWxvbik6CisgICAgICAgICogVUlQcm9jZXNzL2lvcy9XS0NvbnRlbnRWaWV3 SW50ZXJhY3Rpb24uaDoKKyAgICAgICAgKHdpdGhpbkVwc2lsb24pOiBEZWxldGVkLgorCisyMDE0 LTA3LTA5ICBCZW5qYW1pbiBQb3VsYWluICA8YnBvdWxhaW5AYXBwbGUuY29tPgorCiAgICAgICAg IFtpT1NdW1dLMl0gRGlzYWJsZSB0ZXh0IHF1YW50aXphdGlvbiB3aGlsZSBhY3RpdmVseSBjaGFu Z2luZyB0aGUgcGFnZSdzIHNjYWxlIGZhY3RvcgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTM0NzgxCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tIGIvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0FQSS9Db2NvYS9XS1dlYlZpZXcubW0KaW5kZXggMzdjZmU3YjZjYWIwMGEyOGJkOGZhNWZi YzAzYjQyOWZjOWIxOWZkNC4uM2ZmZWYxY2RiMGMwODliZGM2ZTk5N2QxZjA4YzlkOGYzZGFkNjA1 YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9Db2NvYS9XS1dlYlZp ZXcubW0KKysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9Db2NvYS9XS1dlYlZpZXcu bW0KQEAgLTc4Niw2ICs3ODYsMTIgQEAgc3RhdGljIHZvaWQgY2hhbmdlQ29udGVudE9mZnNldEJv dW5kZWRJblZhbGlkUmFuZ2UoVUlTY3JvbGxWaWV3ICpzY3JvbGxWaWV3LCBXZWIKICAgICBzY3Jv bGxWaWV3LmNvbnRlbnRPZmZzZXQgPSBjb250ZW50T2Zmc2V0OwogfQogCit0ZW1wbGF0ZSA8dHlw ZW5hbWUgVHlwZUEsIHR5cGVuYW1lIFR5cGVCPgorc3RhdGljIGlubGluZSBib29sIHdpdGhpbkVw c2lsb24oVHlwZUEgYSwgVHlwZUIgYikKK3sKKyAgICByZXR1cm4gZmFicyhhIC0gYikgPCBzdGQ6 Om51bWVyaWNfbGltaXRzPGZsb2F0Pjo6ZXBzaWxvbigpOworfQorCiAtICh2b2lkKV9kaWRDb21t aXRMYXllclRyZWU6KGNvbnN0IFdlYktpdDo6UmVtb3RlTGF5ZXJUcmVlVHJhbnNhY3Rpb24mKWxh eWVyVHJlZVRyYW5zYWN0aW9uCiB7CiAgICAgaWYgKF9jdXN0b21Db250ZW50VmlldykKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0 aW9uLmggYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvaW9zL1dLQ29udGVudFZpZXdJbnRlcmFj dGlvbi5oCmluZGV4IGI3ZWJmNzZjYjA0YWVmNTlmZWY5MWExZjZlMGUxOGFiYmEwZWVkNWMuLjMx NTQ0YWEyN2I2YjVmOWQ4NDAzYjQ5MmM4ZWViMmQ0MzE4MGY5YTYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQyL1VJUHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLmgKKysrIGIv U291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2lvcy9XS0NvbnRlbnRWaWV3SW50ZXJhY3Rpb24uaApA QCAtMTg1LDkgKzE4NSw0IEBAIHN0cnVjdCBXS0F1dG9Db3JyZWN0aW9uRGF0YSB7CiAtICh2b2lk KV9kaXNhYmxlSW5zcGVjdG9yTm9kZVNlYXJjaDsKIEBlbmQKIAotc3RhdGljIGlubGluZSBib29s IHdpdGhpbkVwc2lsb24oZmxvYXQgYSwgZmxvYXQgYikKLXsKLSAgICByZXR1cm4gZmFicyhhIC0g YikgPCBzdGQ6Om51bWVyaWNfbGltaXRzPGZsb2F0Pjo6ZXBzaWxvbigpOwotfQotCiAjZW5kaWYg Ly8gUExBVEZPUk0oSU9TKQo= 234729 2014-07-10 16:19:54 -0700 2014-07-10 17:39:14 -0700 Patch bug-134798-20140710161926.patch text/plain 2953 benjamin U3VidmVyc2lvbiBSZXZpc2lvbjogMTcwOTUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNTNmOGU2NDYzYTI0MDU5 YWIyMmE4ZGYwY2YxZGQzNjY1NDc2NDAwOC4uZTAyODgyZjE3M2VlZTQ3OTQ0Yzg0YWRiMzZkMzk4 ZjUxODc4NzUxMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDMwIEBACiAyMDE0LTA3LTA5ICBCZW5q YW1pbiBQb3VsYWluICA8YnBvdWxhaW5AYXBwbGUuY29tPgogCisgICAgICAgIFtpT1NdW1dLMl0g Rml4IHdpdGhpbkVwc2lsb24oKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTM0Nzk4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgTW92ZSB0aGUgZnVuY3Rpb24gYmFjayB0byBXS1dlYlZpZXcsIGl0IGlzIG5v IGxvbmdlciBuZWVkZWQgaW4gV0tDb250ZW50Vmlldy4KKworICAgICAgICBVc2UgdGhlIHJlYWwg dHlwZXMgYXMgaW5wdXQgdG8gcHJvcGVybHkgdmVyaWZ5IHRoYXQgdGhlIHR3byBpbnB1dHMgYXJl IHdpdGhpbgorICAgICAgICBhIHNtYWxsIHZhbHVlIG9mIHRoZSAzMmJpdCBmbG9hdGluZyBwb2lu dC4KKworICAgICAgICBUaGUgZXBzaWxvbiB3ZSB1c2UgaXMgYWx3YXlzIG9uIDMyIGJpdHMgZmxv YXQgYmVjYXVzZSB3ZSB3YW50IHRvIGF2b2lkIGRvaW5nIHdvcmsgZm9yIGNoYW5nZXMKKyAgICAg ICAgdGhhdCB3b3VsZCBub3QgbWFrZSBhbnkgZGlmZmVyZW5jZSBvbiBmbG9hdC4KKworICAgICAg ICBUaGUgc291cmNlIG9mIHRob3NlIHNtYWxsIGNoYW5nZXMgY29tZXMgZnJvbSB0aGUgZmFjdCBV SVByb2Nlc3MgZG9lcyBhIGxvdCBvZiBwcm9jZXNzaW5nCisgICAgICAgIG9uIENHRmxvYXQsIHdo aWNoIGFyZSBkb3VibGUgb24gNjRiaXRzIGFyY2hpdGVjdHVyZSwgd2hpbGUgdGhlIFdlYlByb2Nl c3MgdXNlIDMyYml0cyBmbG9hdGluZyBwb2ludAorICAgICAgICBmb3Igc2NhbGUuIFdoZW4gd2Ug YXJlIGdldHRpbmcgdXBkYXRlcyBmcm9tIHRoZSBXZWJQcm9jZXNzLCB3ZSBzaG91bGQgaWdub3Jl IGFueSBzbWFsbCBkaWZmZXJlbmNlcworICAgICAgICBjYXVzZWQgYnkgdGhlIGNvbXB1dGF0aW9u cyBkb25lIHdpdGggbGVzcyBwcmVjaXNpb24uCisKKyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL0Nv Y29hL1dLV2ViVmlldy5tbToKKyAgICAgICAgKHdpdGhpbkVwc2lsb24pOgorICAgICAgICAqIFVJ UHJvY2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLmg6CisgICAgICAgICh3aXRoaW5F cHNpbG9uKTogRGVsZXRlZC4KKworMjAxNC0wNy0wOSAgQmVuamFtaW4gUG91bGFpbiAgPGJwb3Vs YWluQGFwcGxlLmNvbT4KKwogICAgICAgICBbaU9TXVtXSzJdIERpc2FibGUgdGV4dCBxdWFudGl6 YXRpb24gd2hpbGUgYWN0aXZlbHkgY2hhbmdpbmcgdGhlIHBhZ2UncyBzY2FsZSBmYWN0b3IKICAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzNDc4MQogCmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL0NvY29hL1dLV2ViVmlldy5t bSBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tCmluZGV4 IDM3Y2ZlN2I2Y2FiMDBhMjhiZDhmYTVmYmMwM2I0MjlmYzliMTlmZDQuLjM2Y2M0ZjdiY2ZlODAx Y2IwMzQxNjIyNjUwMDFiYzBkYzQ1YWFmZTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJ UHJvY2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9BUEkvQ29jb2EvV0tXZWJWaWV3Lm1tCkBAIC03ODYsNiArNzg2LDEyIEBAIHN0YXRpYyB2 b2lkIGNoYW5nZUNvbnRlbnRPZmZzZXRCb3VuZGVkSW5WYWxpZFJhbmdlKFVJU2Nyb2xsVmlldyAq c2Nyb2xsVmlldywgV2ViCiAgICAgc2Nyb2xsVmlldy5jb250ZW50T2Zmc2V0ID0gY29udGVudE9m ZnNldDsKIH0KIAordGVtcGxhdGUgPHR5cGVuYW1lIFR5cGVBLCB0eXBlbmFtZSBUeXBlQj4KK3N0 YXRpYyBpbmxpbmUgYm9vbCB3aXRoaW5FcHNpbG9uKFR5cGVBIGEsIFR5cGVCIGIpCit7CisgICAg cmV0dXJuIHN0ZDo6YWJzKGEgLSBiKSA8IHN0ZDo6bnVtZXJpY19saW1pdHM8ZmxvYXQ+OjplcHNp bG9uKCk7Cit9CisKIC0gKHZvaWQpX2RpZENvbW1pdExheWVyVHJlZTooY29uc3QgV2ViS2l0OjpS ZW1vdGVMYXllclRyZWVUcmFuc2FjdGlvbiYpbGF5ZXJUcmVlVHJhbnNhY3Rpb24KIHsKICAgICBp ZiAoX2N1c3RvbUNvbnRlbnRWaWV3KQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL2lvcy9XS0NvbnRlbnRWaWV3SW50ZXJhY3Rpb24uaCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9pb3MvV0tDb250ZW50Vmlld0ludGVyYWN0aW9uLmgKaW5kZXggYjdlYmY3NmNiMDRhZWY1 OWZlZjkxYTFmNmUwZTE4YWJiYTBlZWQ1Yy4uMzE1NDRhYTI3YjZiNWY5ZDg0MDNiNDkyYzhlZWIy ZDQzMTgwZjlhNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2lvcy9XS0Nv bnRlbnRWaWV3SW50ZXJhY3Rpb24uaAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvaW9z L1dLQ29udGVudFZpZXdJbnRlcmFjdGlvbi5oCkBAIC0xODUsOSArMTg1LDQgQEAgc3RydWN0IFdL QXV0b0NvcnJlY3Rpb25EYXRhIHsKIC0gKHZvaWQpX2Rpc2FibGVJbnNwZWN0b3JOb2RlU2VhcmNo OwogQGVuZAogCi1zdGF0aWMgaW5saW5lIGJvb2wgd2l0aGluRXBzaWxvbihmbG9hdCBhLCBmbG9h dCBiKQotewotICAgIHJldHVybiBmYWJzKGEgLSBiKSA8IHN0ZDo6bnVtZXJpY19saW1pdHM8Zmxv YXQ+OjplcHNpbG9uKCk7Ci19Ci0KICNlbmRpZiAvLyBQTEFURk9STShJT1MpCg==