234104 2021-12-09 14:43:27 -0800 Momentum Generator: Scroll tail hiccup only when scrolling up on 60Hz displays 2021-12-09 16:18:27 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 thorton thorton darin simon.fraser webkit-bug-importer oldest_to_newest 1822349 0 thorton 2021-12-09 14:43:27 -0800 Momentum Generator: Scroll tail hiccup only when scrolling up on 60Hz displays 1822352 1 446611 thorton 2021-12-09 14:45:43 -0800 Created attachment 446611 Patch 1822356 2 446613 thorton 2021-12-09 14:50:01 -0800 Created attachment 446613 Patch 1822368 3 thorton 2021-12-09 15:02:34 -0800 https://trac.webkit.org/changeset/286805/webkit 1822369 4 webkit-bug-importer 2021-12-09 15:03:21 -0800 <rdar://problem/86292894> 1822375 5 446613 darin 2021-12-09 15:20:05 -0800 Comment on attachment 446613 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446613&action=review > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:442 > + if (!firstZeroIndex[axis]) > + return; > + > + if (deltas[axis][0] > 0) > + std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::greater<float>()); > + else > + std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::less<float>()); Your "micro-style" choices here show a lack of love for local variables. I probably would have written this with a couple locals, just to make the code less verbose: auto begin = deltas[axis].begin(); auto comparator = *begin > 0 ? std::greater<float>() : std::less<float>(); std::sort(begin, std::next(begin, firstZeroIndex[axis]), comparator); The check for 0 of firstZeroIndex is not needed for correctness, and presumably it’s also not an important optimization. 1822390 6 thorton 2021-12-09 15:37:08 -0800 (In reply to Darin Adler from comment #5) > Comment on attachment 446613 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=446613&action=review > > > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:442 > > + if (!firstZeroIndex[axis]) > > + return; > > + > > + if (deltas[axis][0] > 0) > > + std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::greater<float>()); > > + else > > + std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::less<float>()); > > Your "micro-style" choices here show a lack of love for local variables. I > probably would have written this with a couple locals, just to make the code > less verbose: Hehe, fair! > auto begin = deltas[axis].begin(); > auto comparator = *begin > 0 ? std::greater<float>() : > std::less<float>(); > std::sort(begin, std::next(begin, firstZeroIndex[axis]), comparator); > > The check for 0 of firstZeroIndex is not needed for correctness, and > presumably it’s also not an important optimization. I actually originally wrote something similar for the comparator (though inline...) but got incompatible operand types ('std::greater<float>' and 'std::less<float>') (your suggestion does the same) and didn't want to fuss too much. 1822401 7 darin 2021-12-09 15:56:52 -0800 (In reply to Tim Horton from comment #6) > I actually originally wrote something similar for the comparator (though > inline...) but got > > incompatible operand types ('std::greater<float>' and 'std::less<float>') > > (your suggestion does the same) and didn't want to fuss too much. Wow, so sad. Now I understand your choice. For fun, I will research to see if this is a solvable problem. I’m sure one way is to use std::function as the result type, and clang, at least, is smart enough to still optimize and inline this even when std::function is involved. But writing out the std::function type might make things too ugly. 1822403 8 darin 2021-12-09 16:01:31 -0800 Oh, I see why. std::greater<float> and std::less<float> are types, so of course the two types are two different types. Look how "beautiful" my solution is: auto comparator = *begin > 0 ? std::function<bool(float,float)>(std::greater<float>()) : std::less<float>(); Sadly not really good. 1822412 9 446613 darin 2021-12-09 16:18:27 -0800 Comment on attachment 446613 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446613&action=review >>> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:442 >>> + std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::less<float>()); >> >> Your "micro-style" choices here show a lack of love for local variables. I probably would have written this with a couple locals, just to make the code less verbose: >> >> auto begin = deltas[axis].begin(); >> auto comparator = *begin > 0 ? std::greater<float>() : std::less<float>(); >> std::sort(begin, std::next(begin, firstZeroIndex[axis]), comparator); >> >> The check for 0 of firstZeroIndex is not needed for correctness, and presumably it’s also not an important optimization. > > Hehe, fair! Hey, today I learned this (from Anders): std::sort(deltas[axis].begin(), std::next(deltas[axis].begin(), firstZeroIndex[axis]), std::greater<>()); No need to specify "float". 446611 2021-12-09 14:45:43 -0800 2021-12-09 14:50:00 -0800 Patch bug-234104-20211209144542.patch text/plain 2266 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMjg2Nzg0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGFmMzBkZDk0ZjQyMGVhMmJm YWY0Yjc2MTg4OTdjOTUyZGQ4ZDdmZjUuLmFiZTZiZTI3Yzg3NTA0YzJhYzNjMjM4ZmI4OGEzOGM0 ZDBkMjg4YTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjEtMTItMDkgIFRpbSBIb3J0 b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisKKyAgICAgICAgTW9tZW50dW0gR2VuZXJh dG9yOiBTY3JvbGwgdGFpbCBoaWNjdXAgb25seSB3aGVuIHNjcm9sbGluZyB1cCBvbiA2MEh6IGRp c3BsYXlzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0y MzQxMDQKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzg2MjkxNDEzPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9XZWJQYWdlL01v bWVudHVtRXZlbnREaXNwYXRjaGVyLmNwcDoKKyAgICAgICAgKFdlYktpdDo6TW9tZW50dW1FdmVu dERpc3BhdGNoZXI6OmVxdWFsaXplVGFpbEdhcHMpOgorICAgICAgICBTb3J0IGluIHRoZSBjb3Jy ZWN0IGRpcmVjdGlvbiBiYXNlZCBvbiB0aGUgc2lnbiBvZiB0aGUgZmlyc3QgZGVsdGEuLi4KKwog MjAyMS0xMi0wOSAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAgICAgIFVu cmV2aWV3ZWQgZml4IGFmdGVyIHIyODY3NzggdG8gYWRkcmVzcyBXZWJDb250ZW50IGNyYXNoZXMg b24gbGF1bmNoCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2ViUGFnZS9N b21lbnR1bUV2ZW50RGlzcGF0Y2hlci5jcHAgYi9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2Vi UGFnZS9Nb21lbnR1bUV2ZW50RGlzcGF0Y2hlci5jcHAKaW5kZXggYTA0NWM4M2QxOGQ2NmFiZmQw NTAwZjZlZGYwZjZlYjYwYmVkMzM1ZS4uMGYzYmM2MDM0ZjVlZWViYWU4MzhlYjdiYjNjYWY4OGY2 YWY2ZDQ3MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL1dlYlBhZ2UvTW9t ZW50dW1FdmVudERpc3BhdGNoZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vzcy9X ZWJQYWdlL01vbWVudHVtRXZlbnREaXNwYXRjaGVyLmNwcApAQCAtNDI5LDExICs0MjksMTkgQEAg dm9pZCBNb21lbnR1bUV2ZW50RGlzcGF0Y2hlcjo6ZXF1YWxpemVUYWlsR2FwcygpCiAgICAgICAg IGlmICghZmlyc3RaZXJvSW5kZXhbVmVydGljYWxdICYmICF0YWJsZVtpXS5oZWlnaHQoKSkKICAg ICAgICAgICAgIGZpcnN0WmVyb0luZGV4W1ZlcnRpY2FsXSA9IGk7CiAgICAgfQotICAgIAotICAg IGlmIChhdXRvIGluZGV4ID0gZmlyc3RaZXJvSW5kZXhbSG9yaXpvbnRhbF0pCi0gICAgICAgIHN0 ZDo6c29ydChkZWx0YXNbSG9yaXpvbnRhbF0uYmVnaW4oKSwgc3RkOjpuZXh0KGRlbHRhc1tIb3Jp em9udGFsXS5iZWdpbigpLCBpbmRleCkpOwotICAgIGlmIChhdXRvIGluZGV4ID0gZmlyc3RaZXJv SW5kZXhbVmVydGljYWxdKQotICAgICAgICBzdGQ6OnNvcnQoZGVsdGFzW1ZlcnRpY2FsXS5iZWdp bigpLCBzdGQ6Om5leHQoZGVsdGFzW1ZlcnRpY2FsXS5iZWdpbigpLCBpbmRleCkpOworCisgICAg YXV0byBzb3J0RGVsdGFzID0gWyZdIChBeGlzIGF4aXMpIHsKKyAgICAgICAgaWYgKCFmaXJzdFpl cm9JbmRleFtheGlzXSkKKyAgICAgICAgICAgIHJldHVybjsKKworICAgICAgICBpZiAoZGVsdGFz W2F4aXNdWzBdID4gMCkKKyAgICAgICAgICAgIHN0ZDo6c29ydChkZWx0YXNbYXhpc10uYmVnaW4o KSwgc3RkOjpuZXh0KGRlbHRhc1theGlzXS5iZWdpbigpLCBmaXJzdFplcm9JbmRleFtheGlzXSks IHN0ZDo6Z3JlYXRlcjxmbG9hdD4oKSk7CisgICAgICAgIGVsc2UKKyAgICAgICAgICAgIHN0ZDo6 c29ydChkZWx0YXNbYXhpc10uYmVnaW4oKSwgc3RkOjpuZXh0KGRlbHRhc1theGlzXS5iZWdpbigp LCBmaXJzdFplcm9JbmRleFtheGlzXSksIHN0ZDo6bGVzczxmbG9hdD4oKSk7CisgICAgfTsKKwor ICAgIHNvcnREZWx0YXMoSG9yaXpvbnRhbCk7CisgICAgc29ydERlbHRhcyhWZXJ0aWNhbCk7CiAK ICAgICAvLyBHYXBTaXplIGlzIGEgY291bnQgb2YgY29udGlndW91cyBmcmFtZXMgd2l0aCB6ZXJv IGRlbHRhcy4KICAgICB0eXBlZGVmIHVuc2lnbmVkIEdhcFNpemVbMl07Cg== 446613 2021-12-09 14:50:01 -0800 2021-12-09 14:51:17 -0800 Patch bug-234104-20211209145001.patch text/plain 2552 thorton U3VidmVyc2lvbiBSZXZpc2lvbjogMjg2Nzg0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGFmMzBkZDk0ZjQyMGVhMmJm YWY0Yjc2MTg4OTdjOTUyZGQ4ZDdmZjUuLmFiZTZiZTI3Yzg3NTA0YzJhYzNjMjM4ZmI4OGEzOGM0 ZDBkMjg4YTYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMjEtMTItMDkgIFRpbSBIb3J0 b24gIDx0aW1vdGh5X2hvcnRvbkBhcHBsZS5jb20+CisKKyAgICAgICAgTW9tZW50dW0gR2VuZXJh dG9yOiBTY3JvbGwgdGFpbCBoaWNjdXAgb25seSB3aGVuIHNjcm9sbGluZyB1cCBvbiA2MEh6IGRp c3BsYXlzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0y MzQxMDQKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzg2MjkxNDEzPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogV2ViUHJvY2Vzcy9XZWJQYWdlL01v bWVudHVtRXZlbnREaXNwYXRjaGVyLmNwcDoKKyAgICAgICAgKFdlYktpdDo6TW9tZW50dW1FdmVu dERpc3BhdGNoZXI6OmVxdWFsaXplVGFpbEdhcHMpOgorICAgICAgICBTb3J0IGluIHRoZSBjb3Jy ZWN0IGRpcmVjdGlvbiBiYXNlZCBvbiB0aGUgc2lnbiBvZiB0aGUgZmlyc3QgZGVsdGEuLi4KKwog MjAyMS0xMi0wOSAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAgICAgIFVu cmV2aWV3ZWQgZml4IGFmdGVyIHIyODY3NzggdG8gYWRkcmVzcyBXZWJDb250ZW50IGNyYXNoZXMg b24gbGF1bmNoCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2ViUGFnZS9N b21lbnR1bUV2ZW50RGlzcGF0Y2hlci5jcHAgYi9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3MvV2Vi UGFnZS9Nb21lbnR1bUV2ZW50RGlzcGF0Y2hlci5jcHAKaW5kZXggYTA0NWM4M2QxOGQ2NmFiZmQw NTAwZjZlZGYwZjZlYjYwYmVkMzM1ZS4uOWMwZDA0MTllMDc1M2YzMWE5OTFlZjNjZjc1NWY5ZDY1 NzdlOGJjOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNzL1dlYlBhZ2UvTW9t ZW50dW1FdmVudERpc3BhdGNoZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vzcy9X ZWJQYWdlL01vbWVudHVtRXZlbnREaXNwYXRjaGVyLmNwcApAQCAtNDE0LDYgKzQxNCw4IEBAIHZv aWQgTW9tZW50dW1FdmVudERpc3BhdGNoZXI6OmVxdWFsaXplVGFpbEdhcHMoKQogCiAgICAgYXV0 byYgdGFibGUgPSBtX2N1cnJlbnRHZXN0dXJlLnRhaWxEZWx0YVRhYmxlOwogICAgIHNpemVfdCBp bml0aWFsVGFibGVTaXplID0gdGFibGUuc2l6ZSgpOworICAgIGlmICghaW5pdGlhbFRhYmxlU2l6 ZSkKKyAgICAgICAgcmV0dXJuOwogCiAgICAgZW51bSBBeGlzIHsgSG9yaXpvbnRhbCwgVmVydGlj YWwgfTsKICAgICBWZWN0b3I8ZmxvYXQ+IGRlbHRhc1syXTsKQEAgLTQyOSwxMSArNDMxLDE5IEBA IHZvaWQgTW9tZW50dW1FdmVudERpc3BhdGNoZXI6OmVxdWFsaXplVGFpbEdhcHMoKQogICAgICAg ICBpZiAoIWZpcnN0WmVyb0luZGV4W1ZlcnRpY2FsXSAmJiAhdGFibGVbaV0uaGVpZ2h0KCkpCiAg ICAgICAgICAgICBmaXJzdFplcm9JbmRleFtWZXJ0aWNhbF0gPSBpOwogICAgIH0KLSAgICAKLSAg ICBpZiAoYXV0byBpbmRleCA9IGZpcnN0WmVyb0luZGV4W0hvcml6b250YWxdKQotICAgICAgICBz dGQ6OnNvcnQoZGVsdGFzW0hvcml6b250YWxdLmJlZ2luKCksIHN0ZDo6bmV4dChkZWx0YXNbSG9y aXpvbnRhbF0uYmVnaW4oKSwgaW5kZXgpKTsKLSAgICBpZiAoYXV0byBpbmRleCA9IGZpcnN0WmVy b0luZGV4W1ZlcnRpY2FsXSkKLSAgICAgICAgc3RkOjpzb3J0KGRlbHRhc1tWZXJ0aWNhbF0uYmVn aW4oKSwgc3RkOjpuZXh0KGRlbHRhc1tWZXJ0aWNhbF0uYmVnaW4oKSwgaW5kZXgpKTsKKworICAg IGF1dG8gc29ydERlbHRhcyA9IFsmXSAoQXhpcyBheGlzKSB7CisgICAgICAgIGlmICghZmlyc3Ra ZXJvSW5kZXhbYXhpc10pCisgICAgICAgICAgICByZXR1cm47CisKKyAgICAgICAgaWYgKGRlbHRh c1theGlzXVswXSA+IDApCisgICAgICAgICAgICBzdGQ6OnNvcnQoZGVsdGFzW2F4aXNdLmJlZ2lu KCksIHN0ZDo6bmV4dChkZWx0YXNbYXhpc10uYmVnaW4oKSwgZmlyc3RaZXJvSW5kZXhbYXhpc10p LCBzdGQ6OmdyZWF0ZXI8ZmxvYXQ+KCkpOworICAgICAgICBlbHNlCisgICAgICAgICAgICBzdGQ6 OnNvcnQoZGVsdGFzW2F4aXNdLmJlZ2luKCksIHN0ZDo6bmV4dChkZWx0YXNbYXhpc10uYmVnaW4o KSwgZmlyc3RaZXJvSW5kZXhbYXhpc10pLCBzdGQ6Omxlc3M8ZmxvYXQ+KCkpOworICAgIH07CisK KyAgICBzb3J0RGVsdGFzKEhvcml6b250YWwpOworICAgIHNvcnREZWx0YXMoVmVydGljYWwpOwog CiAgICAgLy8gR2FwU2l6ZSBpcyBhIGNvdW50IG9mIGNvbnRpZ3VvdXMgZnJhbWVzIHdpdGggemVy byBkZWx0YXMuCiAgICAgdHlwZWRlZiB1bnNpZ25lZCBHYXBTaXplWzJdOwo=