Bug 234104 – Momentum Generator: Scroll tail hiccup only when scrolling up on 60Hz displays

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tim Horton]

Momentum Generator: Scroll tail hiccup only when scrolling up on 60Hz displays

[Tim Horton]

Created attachment 446611 [details]
Patch

[Tim Horton]

Created attachment 446613 [details]
Patch

[Tim Horton]

https://trac.webkit.org/changeset/286805/webkit

[Radar WebKit Bug Importer]

<rdar://problem/86292894>

[Darin Adler]

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.

[Tim Horton]

(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.

[Darin Adler]

(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.

[Darin Adler]

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.

[Darin Adler]

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".