Momentum Generator: Scroll tail hiccup only when scrolling up on 60Hz displays
Created attachment 446611 [details] Patch
Created attachment 446613 [details] Patch
https://trac.webkit.org/changeset/286805/webkit
<rdar://problem/86292894>
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: 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.
(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.
(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.
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.
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 >>> + 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".