[webkit-reviews] review denied: [Bug 193447] Limit user-agent interactions based on the touch-action property : [Attachment 359260] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antoine Quint
<graouts at apple.com>'s request for review:
Bug 193447: Limit user-agent interactions based on the touch-action property
https://bugs.webkit.org/show_bug.cgi?id=193447

Attachment 359260: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:4151
> +    if (auto* page = this->page()) {
> +	   if (auto* frameView = view()) {
> +	       if (auto* scrollingCoordinator = page->scrollingCoordinator())
> +		  
scrollingCoordinator->frameViewEventTrackingRegionsChanged(*frameView);
> +	   }
> +    }

Should this really be inside ENABLE(TOUCH_EVENTS)?

> Source/WebCore/dom/Document.cpp:8676
> +#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS_FAMILY)

These are normally the other way around. Also this entire function is inside
#if ENABLE(TOUCH_EVENTS).

> Source/WebCore/dom/Element.cpp:4125
> +#if ENABLE(TOUCH_EVENTS)

Why aren't your changes inside ENABLE(POINTER_EVENTS)?

> Source/WebCore/dom/Element.cpp:4126
> +OptionSet<TouchAction> Element::computedTouchActions() const

This function has a tree walk up to the root element. That seems really
expensive.

> Source/WebCore/dom/Element.cpp:4129
> +    for (auto* element = this; element; element =
parentCrossingFrameBoundaries(element)) {

parentCrossingFrameBoundaries seems wrong here.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2587
> +	   if ([_contentView preventsPanningInXAxis])
> +	       targetContentOffset->x = scrollView.contentOffset.x;
> +	   if ([_contentView preventsPanningInYAxis])
> +	       targetContentOffset->y = scrollView.contentOffset.y;

What happens if you don't have this code?

>
Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:215
> +

Blank line.

>
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateI
OS.mm:76
> +	       bool canPanX = true;
> +	       bool canPanY = true;

These are unused.

>
Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeScrollingNodeDelegateI
OS.mm:147
> +    bool canPanX = true;
> +    bool canPanY = true;

These are unused.

> Source/WebKit/UIProcess/WebPageProxy.h:685
> +bool isScrollingOrZooming() const { return m_isScrollingOrZooming; }

Indentation.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1160
> +		   [_singleTapGestureRecognizer setEnabled:NO];
> +		   [_nonBlockingDoubleTapGestureRecognizer setEnabled:NO];
> +		   [_twoFingerDoubleTapGestureRecognizer setEnabled:NO];
> +		   [_highlightLongPressGestureRecognizer setEnabled:NO];
> +		   [_twoFingerSingleTapGestureRecognizer setEnabled:NO];
> +		   [_stylusSingleTapGestureRecognizer setEnabled:NO];
> +		   [_doubleTapGestureRecognizer setEnabled:NO];
> +		   [_longPressGestureRecognizer setEnabled:NO];

Where do they get re-enabled?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1190
> +- (void)_resetPanningPreventionFlags

You need to call this from -cleanupInteraction too to clean up state when a web
process crashes or is swapped.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:838
> +Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>>
WebPageProxy::touchActionsForTouchEvent(const WebTouchEvent& event)

Vector<std::pair<OptionSet<WebCore::TouchAction>, WebCore::ScrollingNodeID>> is
gross. At least typedef it.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:841
> +    process().sendSync(Messages::WebPage::TouchActionsForTouchEvent(event),
Messages::WebPage::TouchActionsForTouchEvent::Reply(touchActions), m_pageID);

This is going to be a massive new source of UI process hangs.