Bug 220915

Summary: Use ScrollSnapOffsetsInfo in the scrolling tree
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: ScrollingAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, ews-watchlist, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 145099    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Martin Robinson 2021-01-25 04:04:44 PST
This bugs track using ScrollSnapOffsetsInfo to abstract away details of the scroll snap offsets in the scrolling tree.
Comment 1 Martin Robinson 2021-01-25 04:16:41 PST
Created attachment 418271 [details]
Patch
Comment 2 Martin Robinson 2021-01-25 04:33:55 PST
Created attachment 418274 [details]
Patch
Comment 3 Martin Robinson 2021-01-25 04:46:29 PST
Created attachment 418275 [details]
Patch
Comment 4 Martin Robinson 2021-01-25 05:39:49 PST
Created attachment 418279 [details]
Patch
Comment 5 Martin Robinson 2021-01-25 07:25:25 PST
Created attachment 418290 [details]
Patch
Comment 6 Simon Fraser (smfr) 2021-01-25 10:10:06 PST
Comment on attachment 418290 [details]
Patch

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

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:118
> +        auto convertOffsetsToFloats = [deviceScaleFactor](const Vector<LayoutUnit>& offsets, Vector<float>& floatOffsets)
> +        {
> +            floatOffsets.reserveInitialCapacity(offsets.size());
> +            for (auto& offset : offsets)
> +                floatOffsets.uncheckedAppend(roundToDevicePixel(offset, deviceScaleFactor, false));
> +        };
> +
> +        auto convertOffsetRangesToFloat = [deviceScaleFactor](const Vector<ScrollOffsetRange<LayoutUnit>>& offsetRanges, Vector<ScrollOffsetRange<float>>& floatOffsetRanges)
> +        {
> +            floatOffsetRanges.reserveInitialCapacity(offsetRanges.size());
> +            for (auto& range : offsetRanges)
> +                floatOffsetRanges.uncheckedAppend({ roundToDevicePixel(range.start, deviceScaleFactor, false), roundToDevicePixel(range.end, deviceScaleFactor, false) });
> +        };
> +
> +        convertOffsetsToFloats(offsetInfo->horizontalSnapOffsets, floatInfo.horizontalSnapOffsets);
> +        convertOffsetsToFloats(offsetInfo->verticalSnapOffsets, floatInfo.verticalSnapOffsets);
> +        convertOffsetRangesToFloat(offsetInfo->horizontalSnapOffsetRanges, floatInfo.horizontalSnapOffsetRanges);
> +        convertOffsetRangesToFloat(offsetInfo->verticalSnapOffsetRanges, floatInfo.verticalSnapOffsetRanges);

All this conversion should be in some helpers on ScrollSnapOffsetsInfo. Can we do template specialization goop to just have it be a special operator=?
Comment 7 Martin Robinson 2021-01-26 06:17:10 PST
Created attachment 418411 [details]
Patch
Comment 8 Martin Robinson 2021-01-26 06:18:23 PST
(In reply to Simon Fraser (smfr) from comment #6)

> All this conversion should be in some helpers on ScrollSnapOffsetsInfo. Can
> we do template specialization goop to just have it be a special operator=?

Thanks for the review. I've moved all the conversion functions to 
ScrollSnapOffsetsInfo. I'm using a templated method, because we need to pass in an argument for the device scale when converting from LayoutUnits to floats. The method is called convertUnits.
Comment 9 Simon Fraser (smfr) 2021-01-26 10:13:52 PST
Comment on attachment 418411 [details]
Patch

Nice.
Comment 10 EWS 2021-01-27 00:41:48 PST
Committed r271937: <https://trac.webkit.org/changeset/271937>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418411 [details].
Comment 11 Radar WebKit Bug Importer 2021-01-27 00:42:17 PST
<rdar://problem/73652510>