Bug 233993 - Momentum Event Dispatcher: Momentum tail should have montonically decreasing deltas and tail gaps
Summary: Momentum Event Dispatcher: Momentum tail should have montonically decreasing ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-08 00:31 PST by Tim Horton
Modified: 2021-12-08 11:58 PST (History)
2 users (show)

See Also:


Attachments
Patch (15.69 KB, patch)
2021-12-08 00:32 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (16.13 KB, patch)
2021-12-08 00:34 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (16.12 KB, patch)
2021-12-08 00:38 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (16.94 KB, patch)
2021-12-08 01:51 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (17.71 KB, patch)
2021-12-08 11:46 PST, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2021-12-08 00:31:39 PST
Momentum Event Dispatcher: Momentum tail should have montonically decreasing deltas and tail gaps
Comment 1 Tim Horton 2021-12-08 00:32:13 PST
Created attachment 446327 [details]
Patch
Comment 2 Tim Horton 2021-12-08 00:32:15 PST
<rdar://problem/86118367>
Comment 3 Tim Horton 2021-12-08 00:34:01 PST
Created attachment 446328 [details]
Patch
Comment 4 Tim Horton 2021-12-08 00:38:22 PST
Created attachment 446329 [details]
Patch
Comment 5 Tim Horton 2021-12-08 01:51:12 PST
Created attachment 446335 [details]
Patch
Comment 6 Simon Fraser (smfr) 2021-12-08 10:36:57 PST
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?
Comment 7 Tim Horton 2021-12-08 10:39:48 PST
(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!
Comment 8 Tim Horton 2021-12-08 11:46:44 PST
Created attachment 446399 [details]
Patch
Comment 9 Tim Horton 2021-12-08 11:58:33 PST
https://trac.webkit.org/changeset/286671/webkit