[webkit-reviews] review granted: [Bug 196536] Compute accurate regions for touch-action : [Attachment 366611] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 196536: Compute accurate regions for touch-action
https://bugs.webkit.org/show_bug.cgi?id=196536

Attachment 366611: patch

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




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

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

> Source/WebCore/css/StyleResolver.cpp:849
> +    if (effectiveTouchActions.contains(TouchAction::None))
> +	   return { TouchAction::None };

So "none" on an ancestor clobbers any value on descendants? That's weird.

>> Source/WebCore/css/StyleResolver.cpp:853
>> +	    return elementTouchActions;
> 
> I don't understand the intent of this code. What is an "effective
touch-action"?

I'm a bit confused by this code too. I think it needs a link to spec text.

> Source/WebCore/css/StyleResolver.cpp:1107
> +	  
style.setEffectiveTouchActions(computeEffectiveTouchActions(style.touchActions(
), style.effectiveTouchActions()));

This might be clearer if the second param used
parentStyle.effectiveTouchActions()

> Source/WebCore/rendering/PaintInfo.h:46
> +class TouchActionRegion;

Needs if ENABLE(POINTER_EVENTS) guard

> Source/WebCore/rendering/PaintInfo.h:135
> +    TouchActionRegion* touchActionRegion { nullptr };

Needs if ENABLE(POINTER_EVENTS) guard

> Source/WebCore/rendering/RenderLayer.cpp:4873
> +	   paintInfo.touchActionRegion = localPaintingInfo.touchActionRegion;

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/RenderLayer.h:81
> +class TouchActionRegion;

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/RenderLayer.h:918
> +	   TouchActionRegion* touchActionRegion { nullptr };

if ENABLE(POINTER_EVENTS)

> Source/WebCore/rendering/TouchActionRegion.cpp:73
> +constexpr const char* toString(TouchAction touchAction)

Or you could just implement TextStream& operator<<(TextStream&, TouchAction)

> Source/WebCore/rendering/style/RenderStyle.h:709
> +    OptionSet<TouchAction> effectiveTouchActions() const { return
OptionSet<TouchAction>::fromRaw(m_rareInheritedData->effectiveTouchActions); }

I think this justifies a comment saying that "touch-action is not an inherited
property, but we fake one with "effectiveTouchActions" so we can blah blah".

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:373
> +	   && effectiveTouchActions == o.effectiveTouchActions

Do we want this to affect operator==? Will that trigger unwanted
layouts/repaints?