[webkit-reviews] review denied: [Bug 173833] [iOS] Adjust layer hierarchy to handle frame scrolling : [Attachment 333586] 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 333586: Patch

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




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

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

> Source/WebCore/page/EventHandler.cpp:1616
> +#if PLATFORM(IOS)
> +    // ScrollView::windowToContents does not take scroll position of
subframes, so we do it here.
> +    if (!frame.isMainFrame())
> +	   return view->viewToContents(view->windowToContents(windowPoint));
> +#endif

It would be best to avoid this #ifdef if we can. There is a problem currently
where delegatesScrolling() currently causes frame scroll positions (maybe only
for the main frame) to be ignored, but we should really fix that because it's
the cause of a number of bugs.

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

I still think it's wrong to set layer sizes here. This should be in
RenderLayerCompositor (where we don't have any code that sets the size of the
scroll layer, but should, probably in
RenderLayerCompositor::frameViewDidChangeSize()).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:439
> +    ASSERT(scrolledContentsLayer);
> +    scrollLayer->setPosition(FloatPoint());
> +    FloatSize oldScrollLayerOffset = scrollLayer->offsetFromRenderer();
> +    scrollLayer->setOffsetFromRenderer(FloatSize());
> +    bool scrollOffsetChanged = oldScrollLayerOffset !=
scrollLayer->offsetFromRenderer();

This stuff should be in RLC or RLB.

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:487
> +    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);

Needs to move elsewhere. RLB is the only class that should be calling
setOffsetFromRenderer()

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:642
> +    scrolledContentsLayer->setOffsetFromRenderer(scrolledContentsOffset,
GraphicsLayer::DontSetNeedsDisplay);

Needs to move elsewhere. RLB is the only class that should be calling
setOffsetFromRenderer()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3296
> +#if PLATFORM(IOS)
> +	       m_scrollLayer = GraphicsLayer::create(graphicsLayerFactory(),
*this, GraphicsLayer::Type::Scrolling);
> +#else

This kind of thing should not be a platform #ifdef. The #ifdef should reflect
the specific reason why the code needs to differ.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingTree.cpp:117
>  #if PLATFORM(IOS)

This is another undesirable platform #ifdef.