[webkit-reviews] review denied: [Bug 173833] [iOS] Adjust layer hierarchy to handle frame scrolling : [Attachment 331743] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Frédéric Wang (:fredw)
<fred.wang at free.fr>'s request for review:
Bug 173833: [iOS] Adjust layer hierarchy to handle frame scrolling
https://bugs.webkit.org/show_bug.cgi?id=173833

Attachment 331743: Patch

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




--- Comment #59 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 331743
  --> https://bugs.webkit.org/attachment.cgi?id=331743
Patch

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

> Source/WebCore/page/FrameView.cpp:4461
> +#if PLATFORM(IOS)
> +	       if (!frame().isMainFrame()) {
> +		   if (auto* renderView = frame().contentRenderer()) {
> +		       if (auto* scrolledContentsLayer =
renderView->compositor().rootContentLayer())
> +			  
point.move(roundedIntSize(scrolledContentsLayer->offsetFromRenderer()));
> +		   }
> +	       }
> +#endif

Is this here to handle async scrolling? Normally point conversion uses the
current state of the DOM, and these offsets should already be reflected in
RenderLayer scroll offsets.

> Source/WebCore/page/FrameView.cpp:4492
> +#if PLATFORM(IOS)
> +	       if (!frame().isMainFrame()) {
> +		   if (auto* renderView = frame().contentRenderer()) {
> +		       if (auto* scrolledContentsLayer =
renderView->compositor().rootContentLayer())
> +			  
point.move(-roundedIntSize(scrolledContentsLayer->offsetFromRenderer()));
> +		   }
> +	       }
> +#endif

Ditto.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:188
> +#if PLATFORM(IOS)
> +    if (auto* scrollLayer = scrollLayerForFrameView(frameView))
> +	   scrollLayer->setSize(frameView.sizeForVisibleContent());
> +#endif

Seems wrong to set layer size here; layer size should be set in
RenderLayerBacking::updateGeometry or similar.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:445
> +    scrollLayer->setPosition(FloatPoint());
> +    FloatSize oldScrollLayerOffset = scrollLayer->offsetFromRenderer();
> +    scrollLayer->setOffsetFromRenderer(FloatSize());

This should also happen near RLB::updateGeometry()

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:495
> +#if PLATFORM(IOS)
> +    IntSize scrollSize = frameView.contentsSize();
> +    scrolledContentsLayer->setPosition(FloatPoint());
> +    if (scrollSize != scrolledContentsLayer->size() || scrollOffsetChanged)
> +	   scrolledContentsLayer->setNeedsDisplay();
> +    scrolledContentsLayer->setSize(scrollSize);
> +    LayoutSize scrolledContentsOffset =
-toLayoutSize(frameView.scrollPosition());
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset,
GraphicsLayer::DontSetNeedsDisplay);
> +#endif

Again, I don't think setting geometry should be done here.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:656
> +#if PLATFORM(IOS)
> +    auto* scrolledContentsLayer = rootContentLayerForFrameView(frameView);
> +    ASSERT(scrolledContentsLayer);
> +    scrollLayer->setBoundsOrigin(frameView.scrollPosition());
> +    LayoutSize scrolledContentsOffset =
-toLayoutSize(frameView.scrollPosition());
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset,
GraphicsLayer::DontSetNeedsDisplay);
> +#else

Ditto.

> Source/WebCore/page/scrolling/ScrollingTreeFrameScrollingNode.h:42
> +    virtual bool isMainFrame() { return m_isMainFrame; }

Method should be const.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1378
> +    } else if (boundsOrigin != FloatPoint())

!boundsOrigin.isZero()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3295
> +	       m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(),
*this, GraphicsLayer::Type::Scrolling);

I don't think we should use GraphicsLayer::Type::Scrolling for the main frame.
Does this code run for the main frame?

>
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeRemoteFrameScrollingNo
deIOS.mm:58
> +void ScrollingTreeRemoteFrameScrollingNodeIOS::setIsMainFrame(bool
mainFrame)
> +{
> +    if (mainFrame != isMainFrame())
> +	   m_scrollingNodeDelegate.reset(mainFrame ? nullptr : new
ScrollingTreeScrollingNodeDelegateIOS(*this));
> +}

I don' think a scrolling node would ever dynamically change between a main
frame and non-main-frame node, so I think the "isMainFrame" state should come
in through the constructor.