[webkit-reviews] review granted: [Bug 210041] Should find touch-action elements inside non-composited iframes : [Attachment 395815] Patch and tests

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 210041: Should find touch-action elements inside non-composited iframes
https://bugs.webkit.org/show_bug.cgi?id=210041

Attachment 395815: Patch and tests

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




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

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

> Source/WebCore/dom/Document.cpp:4237
> +void Document::invalidateEventRegionsForIFrame(HTMLFrameOwnerElement&
element)

This might be a Frame too so call it invalidateEventRegionsForFrame()

> Source/WebCore/dom/Document.cpp:4247
> +    if (auto* ownerElement = this->ownerElement())
> +	  
ownerElement->document().invalidateEventRegionsForIFrame(*ownerElement);

You should always find a layer above (the RenderVIew always has one) so I don't
think you need this clause, and it takes brainspace.

> Source/WebCore/page/Frame.cpp:309
> +#if PLATFORM(IOS)

We don't like platform #ifdefs.

Also async overflow scrolling on macOS uses event regions so this is wrong.

> Source/WebCore/page/Frame.cpp:314
> +    if
(!m_doc->renderView()->compositor().viewNeedsToInvalidateEventRegionOfEnclosing
CompositingLayerForRepaint())

Should probably null-check m_doc->renderView().

> Source/WebCore/page/Frame.cpp:317
> +    if (m_ownerElement)
> +	  
m_ownerElement->document().invalidateEventRegionsForIFrame(*m_ownerElement);

Actually why doesn't compositor() just do this part.

> Source/WebCore/page/FrameView.cpp:1265
> +    frame().invalidateContentEventRegionsIfNeeded();

Please move this down next to invalidateRenderingDependentRegions() because
they are related.

> Source/WebCore/rendering/RenderLayer.cpp:7011
>  #if PLATFORM(IOS_FAMILY)

Possible a wrong #ifdef.

> Source/WebCore/rendering/RenderLayer.cpp:7032
> +	  
compositingLayer->renderer().view().setNeedsEventRegionUpdateForNonCompositedIF
rame();

compositingLayer->renderer().view() is always just renderer().view(); fetch it
once.

> Source/WebCore/rendering/RenderLayer.cpp:7035
> +	   compositingLayer->compositor().scheduleCompositingLayerUpdate();

This can be view->compositior()

> Source/WebCore/rendering/RenderLayer.h:928
> +    bool invalidateEventRegion(EventRegionInvalidationReason);

What does the return value mean? Add a comment.