[webkit-reviews] review granted: [Bug 60305] Separate scrolling code out of RenderLayer : [Attachment 417666] Patch, v23

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 60305: Separate scrolling code out of RenderLayer
https://bugs.webkit.org/show_bug.cgi?id=60305

Attachment 417666: Patch, v23

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




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

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

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:90
> +    auto& renderer = m_layer.renderer();
> +    ASSERT(m_registeredScrollableArea ==
renderer.view().frameView().containsScrollableArea(this));
> +
> +    if (m_registeredScrollableArea)
> +	   renderer.view().frameView().removeScrollableArea(this);
> +
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    unregisterAsTouchEventListenerForScrolling();
> +#endif
> +    if (Element* element = renderer.element())
> +	   element->setSavedLayerScrollPosition(m_scrollPosition);
> +
> +    destroyScrollbar(HorizontalScrollbar);
> +    destroyScrollbar(VerticalScrollbar);
> +
> +    if (auto* scrollingCoordinator = renderer.page().scrollingCoordinator())
> +	   scrollingCoordinator->willDestroyScrollableArea(*this);
> +
> +    clearScrollCorner();
> +    clearResizer();

Doing all this in the destructor makes me a bit nervous. Can we do it in a
"clear" function that's called from RenderLayer?

> Source/WebCore/rendering/RenderLayerScrollableArea.cpp:106
> +void RenderLayerScrollableArea::storeScrollPosition()
> +{
> +    auto* element = m_layer.renderer().element();
> +    if (!element)
> +	   return;
> +
> +    if (m_layer.renderBox()) {
> +	   // We save and restore only the scrollOffset as the other scroll
values are recalculated.
> +	   m_scrollPosition = element->savedLayerScrollPosition();
> +	   if (!m_scrollPosition.isZero())
> +	       scrollAnimator().setCurrentPosition(m_scrollPosition);
> +    }
> +
> +    element->setSavedLayerScrollPosition(IntPoint());

"Store" is ambiguous here. It's more like "restore".

> Source/WebCore/rendering/RenderLayerScrollableArea.h:277
> +    // Minimum padding on x86_64 (4 bytes), after m_scrollHeight)
> +    bool m_scrollDimensionsDirty : 1;
> +    bool m_inOverflowRelayout : 1;
> +    bool m_registeredScrollableArea : 1;
> +    bool m_hasCompositedScrollableOverflow : 1;
> +
> +#if PLATFORM(IOS_FAMILY)
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    bool m_registeredAsTouchEventListenerForScrolling : 1;
> +#endif
> +    bool m_adjustForIOSCaretWhenScrolling : 1;
> +#endif
> +    bool m_requiresScrollPositionReconciliation : 1;
> +    bool m_containsDirtyOverlayScrollbars : 1;
> +    bool m_updatingMarqueePosition : 1;

It's probably not worth using a bitset here for memory saving any more; if
these are plain bools you can use initializers.