[webkit-reviews] review granted: [Bug 218477] Scroll position can get reset after programmatic scroll : [Attachment 413005] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 3 02:52:37 PST 2020


Antti Koivisto <koivisto at iki.fi> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 218477: Scroll position can get reset after programmatic scroll
https://bugs.webkit.org/show_bug.cgi?id=218477

Attachment 413005: Patch

https://bugs.webkit.org/attachment.cgi?id=413005&action=review




--- Comment #4 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 413005
  --> https://bugs.webkit.org/attachment.cgi?id=413005
Patch

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

> Source/WebCore/page/scrolling/ScrollingTree.cpp:405
> +//	 LOG(Scrolling, "\nScrollingTree %p applyLayerPositions (main thread
%d)", this, isMainThread());

Stray //

> Source/WebCore/page/scrolling/ScrollingTree.cpp:582
> +    LockHolder locker(m_pendingScrollUpdatesLock);

I prefer

auto locker = holdLock(m_pendingScrollUpdatesLock);

> Source/WebCore/page/scrolling/ScrollingTree.h:221
> +	   ScrollUpdate() = default;
> +	   ScrollUpdate(ScrollingNodeID scrollingNodeID, FloatPoint point,
Optional<FloatPoint> viewportOrigin, ScrollingLayerPositionAction udpateAction)
> +	       : nodeID(scrollingNodeID)
> +	       , scrollPosition(point)
> +	       , layoutViewportOrigin(viewportOrigin)
> +	       , updateLayerPositionAction(udpateAction)
> +	   { }

You can probably delete these explicit constructors and the code will still
compile as-is.

> Source/WebCore/page/scrolling/ScrollingTree.h:225
> +	   FloatPoint scrollPosition;
> +	   Optional<FloatPoint> layoutViewportOrigin;

...except these may need { }.


More information about the webkit-reviews mailing list