Bug 193665

Created attachment 359724 [details] patch

Comment on attachment 359724 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=359724&action=review Some of these changes regarding scroll offset adjustments seem similar to what I have extracted in attachment 359723 [details] from my old patches. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:455 > if (insetClipLayer) Probably we need to update AsyncScrollingCoordinator::updateScrollLayerPosition too.

> Probably we need to update > AsyncScrollingCoordinator::updateScrollLayerPosition too. Yeah, though it would be better to land that change with a test that verifies it.

Created attachment 359729 [details] patch

Created attachment 359730 [details] patch

Comment on attachment 359724 [details] patch I need to think about this a little bit. Would be nice to have macOS and iOS be similar enough that we don't have to have platform #ifdefs here.

We should clarify which layer moves during scrolling. The regular code paths assume that the scrollLayer (in scrolling tree and elsewhere) is the one that moves. UIScrollView code paths assume scrollLayer is the UIScrollView (so just clips) and the scrolledContentsLayer is the one that moves. The code would be much clearer and require fewer exceptions if we (for example) made the clip layer to be the UIScrollView and let scrollLayer be its moving child layer. This would need to be matched in both overflow and frame code.

Comment on attachment 359730 [details] patch Do we want to land this, or fix the scrollLayer/scrolledContentsLayer confusion first?

Let me take a look at fixing the confusion, at least for the frames.

Created attachment 360596 [details] wip This adds a concept of "scroll container layer" to the scrolling tree and uses it for frames. This layer then maps to UIScrollView when async scrolling is enabled. In this setup "scroll layer" is always the layer that moves during scrolling. This matches the existing pure layer based scrolling code and so requires way fewer hacks. Scroll container layer replaces the existing clip layer as it serves that function as well.

Comment on attachment 360596 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=360596&action=review > Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h:-109 > - // This is a clipping layer that will scroll with the page for all y-delta scroll values between 0 > - // and topContentInset(). Once the y-deltas get beyond the content inset point, this layer no longer > - // needs to move. If the topContentInset() is 0, this layer does not need to move at all. This is > - // only used on the Mac. Do we understand the history here, and why your removal doesn't break something? > Source/WebCore/rendering/RenderLayerCompositor.h:564 > + // Layer that moves and clips its content layer. > + RefPtr<GraphicsLayer> m_scrollContainerLayer; The "that moves" is confusing. How does this layer move its content? The moving is done by setting bounds origin or position on m_scrollLayer, right? This layer doesn't control that. > Source/WebCore/rendering/RenderLayerCompositor.h:566 > + // Layer that moves during scrolling. > RefPtr<GraphicsLayer> m_scrollLayer; Can we call this scrolledContentsLayer? That would be more consistent with RenderLayerBacking names.

> Do we understand the history here, and why your removal doesn't break > something? Nothing is really removed here scrollContainerLayer just also serves as insetClipLayer. All the logic around that is unchanged. > The "that moves" is confusing. How does this layer move its content? The > moving is done by setting bounds origin or position on m_scrollLayer, right? > This layer doesn't control that. Yeah, bad comment. > > Source/WebCore/rendering/RenderLayerCompositor.h:566 > > + // Layer that moves during scrolling. > > RefPtr<GraphicsLayer> m_scrollLayer; > > Can we call this scrolledContentsLayer? That would be more consistent with > RenderLayerBacking names. There is existing scrolledContentsLayer in scrolling tree though, which does not map to this layer.

Comment on attachment 360596 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=360596&action=review >> Source/WebCore/page/scrolling/ScrollingStateFrameScrollingNode.h:-109 >> - // only used on the Mac. > > Do we understand the history here, and why your removal doesn't break something? This was added in https://trac.webkit.org/r168244 > Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:59 > + ScrollContainerLayer, We should rename the layer on the base class to be just "layer", not "ScrollLayer", and we should actually set it to the main graphicsLayer() for scrolling nodes (but the scrolling tree may never use it). >> Source/WebCore/rendering/RenderLayerCompositor.h:564 >> + RefPtr<GraphicsLayer> m_scrollContainerLayer; > > The "that moves" is confusing. How does this layer move its content? The moving is done by setting bounds origin or position on m_scrollLayer, right? This layer doesn't control that. For iOS, the moving is done by setting boundsOrigin on this layer. For macOS, we set the position of m_scrollLayer. I think the best option is to hide that difference, and just make sure that code that moves layers around has access to both these layers.

Comment on attachment 360596 [details] wip Attachment 360596 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10955383 New failing tests: compositing/tiling/tiled-drawing-async-frame-scrolling.html

Created attachment 360604 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Created attachment 361002 [details] patch

Created attachment 361006 [details] patch

Comment on attachment 361006 [details] patch Attachment 361006 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11014492 New failing tests: compositing/tiling/tiled-drawing-async-frame-scrolling.html scrollingcoordinator/scrolling-tree/fixed-inside-frame.html editing/pasteboard/smart-paste-007.html editing/pasteboard/smart-paste-008.html

Created attachment 361010 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Created attachment 361020 [details] patch

editing/pasteboard failures are unrelated to the patch

Comment on attachment 361020 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=361020&action=review r=me but at least rename the setScrollPositionToScrollingLayer/syncScrollPositionToScrollingLayer functions, and maybe move them. > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=193665 And rdar://problem/47743603. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:473 > +void AsyncScrollingCoordinator::setScrollPositionToScrollingLayer(FrameView& frameView) setScrollPositionForScrollingLayer? Setting the position "to" a layer doesn't make sense. Also we're trying to avoid the term "scrolling layer". For overflow, I implemented this equivalent in RenderLayerBacking::setLocationOfScrolledContents(ScrollOffset scrollOffset, ScrollingLayerPositionAction setOrSync). Maybe this code should live in RenderLayerCompositor? Another option is to give the ScrollingStateTreeNodes this responsibility. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:485 > +void AsyncScrollingCoordinator::syncScrollPositionToScrollingLayer(FrameView& frameView) Also needs better name/better home. > Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateIOS.mm:219 > + if (scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ScrollContainerLayer)) > + m_scrollLayer = scrollingStateNode.scrollContainerLayer(); Much less confusing.

Comment on attachment 361020 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=361020&action=review >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:473 >> +void AsyncScrollingCoordinator::setScrollPositionToScrollingLayer(FrameView& frameView) > > setScrollPositionForScrollingLayer? Setting the position "to" a layer doesn't make sense. Also we're trying to avoid the term "scrolling layer". > > For overflow, I implemented this equivalent in RenderLayerBacking::setLocationOfScrolledContents(ScrollOffset scrollOffset, ScrollingLayerPositionAction setOrSync). Maybe this code should live in RenderLayerCompositor? > > Another option is to give the ScrollingStateTreeNodes this responsibility. I combined the set/sync functions and just called it reconcileScrollPosition. Maybe the shared function should be a static function hanging from say GraphicsLayer and taking both the container and contents layers as parameters?

Created attachment 361040 [details] patch

Comment on attachment 361040 [details] patch Clearing flags on attachment: 361040 Committed r240916: <https://trac.webkit.org/changeset/240916>

All reviewed patches have been landed. Closing bug.

<rdar://problem/47779728>