[webkit-reviews] review granted: [Bug 193665] [iOS] Tiles not created in large scrollable iframes : [Attachment 361020] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 193665: [iOS] Tiles not created in large scrollable iframes
https://bugs.webkit.org/show_bug.cgi?id=193665

Attachment 361020: patch

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




--- Comment #22 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 361020
  --> https://bugs.webkit.org/attachment.cgi?id=361020
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/ScrollingTreeScrollingNodeDelegateI
OS.mm:219
> +    if
(scrollingStateNode.hasChangedProperty(ScrollingStateScrollingNode::ScrollConta
inerLayer))
> +	   m_scrollLayer = scrollingStateNode.scrollContainerLayer();

Much less confusing.