Momentum Event Dispatcher: Momentum tail should have montonically decreasing deltas and tail gaps

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Momentum Event Dispatcher: Momentum tail should have montonically decreasing deltas and tail gaps

[Tim Horton]

Created attachment 446327 [details]
Patch

[Tim Horton]

<rdar://problem/86118367>

[Tim Horton]

Created attachment 446328 [details]
Patch

[Tim Horton]

Created attachment 446329 [details]
Patch

[Tim Horton]

Created attachment 446335 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 446335 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=446335&action=review

> Source/WebKit/ChangeLog:24
> +        Sort the deltas up until the first zero to ensure a lack of unexpected
> +        perceptual acceleration, and then inject skipped frames in order to
> +        ensure that frames that have effective scroll movement (non-zero deltas)
> +        are always spaced equally-or-further apart than earlier ones, but
> +        never closer together (which, again, would be percieved as acceleration).

I want this as a code comment.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:381
> +        const float tailStartUnacceleratedDelta = 6.f;

Justify the 6 ?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:414
> +    deltas[0].reserveInitialCapacity(initialTableSize);
> +    deltas[1].reserveInitialCapacity(initialTableSize);

Maybe a constexpr for 0 (horizontal) and 1 (vertical). Or use ScrollEventAxis.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:420
> +    if (auto firstZeroIndex = deltas[0].find(0))

You could look for zeros in the loop above, saving a vector traversal.

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:427
> +    typedef unsigned GapSize[2];
> +    GapSize minimumGap = { 0, 0 };
> +    GapSize currentGap = { 0, 0 };

It wasn't clear to me what GapSize is about. Maybe a comment saying it's a count of contiguous frames with zero delta?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:437
> +            remainingGapToGenerate[axis]--;
> +            currentGap[axis]++;

pre-increment/decrement

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:443
> +            minimumGap[axis] = std::max(minimumGap[axis], currentGap[axis]);

It's call minimumGap but you're using std::max?

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:447
> +                remainingGapToGenerate[axis]--;
> +                currentGap[axis]++;

pre

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:453
> +            currentGap[axis]++;

pre

> Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:137
> +        Seconds tailStartTime;

Either keep the name and use MonotonicTime or keep Seconds and put "delay" or "offset" in the name?

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 446335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446335&action=review
> 
> > Source/WebKit/ChangeLog:24
> > +        Sort the deltas up until the first zero to ensure a lack of unexpected
> > +        perceptual acceleration, and then inject skipped frames in order to
> > +        ensure that frames that have effective scroll movement (non-zero deltas)
> > +        are always spaced equally-or-further apart than earlier ones, but
> > +        never closer together (which, again, would be percieved as acceleration).
> 
> I want this as a code comment.

Good idea.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:381
> > +        const float tailStartUnacceleratedDelta = 6.f;
> 
> Justify the 6 ?

Cannot, I kidnapped it from elsewhere.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:414
> > +    deltas[0].reserveInitialCapacity(initialTableSize);
> > +    deltas[1].reserveInitialCapacity(initialTableSize);
> 
> Maybe a constexpr for 0 (horizontal) and 1 (vertical). Or use
> ScrollEventAxis.

Fair.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:420
> > +    if (auto firstZeroIndex = deltas[0].find(0))
> 
> You could look for zeros in the loop above, saving a vector traversal.

True! Will do.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.cpp:443
> > +            minimumGap[axis] = std::max(minimumGap[axis], currentGap[axis]);
> 
> It's call minimumGap but you're using std::max?

It is the minimum gap that we will enforce going forward, based on the maximum of all previously seen gaps.

> > Source/WebKit/WebProcess/WebPage/MomentumEventDispatcher.h:137
> > +        Seconds tailStartTime;
> 
> Either keep the name and use MonotonicTime or keep Seconds and put "delay"
> or "offset" in the name?

Good point!

[Tim Horton]

Created attachment 446399 [details]
Patch

[Tim Horton]

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