[webkit-reviews] review granted: [Bug 211895] [Wheel event region] Invalidation when changing listeners on elements : [Attachment 399362] 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 211895: [Wheel event region] Invalidation when changing listeners on
elements
https://bugs.webkit.org/show_bug.cgi?id=211895

Attachment 399362: patch

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




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

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

>
LayoutTests/fast/scrolling/mac/wheel-event-listener-region-element-invalidation
.html:20
> +

extra line

>
LayoutTests/fast/scrolling/mac/wheel-event-listener-region-element-invalidation
.html:29
> +    function listener() {
> +	   results.textContent += 'wheel\n';
> +    }

This is never called, right? It could be an empty function.

>
LayoutTests/fast/scrolling/mac/wheel-event-listener-region-element-invalidation
.html:33
> +    if (window.internals)
> +	   results.textContent += internals.layerTreeAsText(document,
internals.LAYER_TREE_INCLUDES_EVENT_REGION |
internals.LAYER_TREE_INCLUDES_ROOT_LAYER_PROPERTIES);

Beware of setting element contents here then fetching layers again later;
layout will change making the two trees hard to compare.

I prefer to store the first layer tree in a variable then set textContent once
at the end.

>
LayoutTests/fast/scrolling/mac/wheel-event-listener-region-element-invalidation
.html:36
> +    passive.addEventListener('wheel', () => { results.textContent +=
'passive wheel\n' }, { passive: true });

Ditto

> Source/WebCore/rendering/RenderLayer.cpp:-6973
> -    // FIXME: This should not be conditioned on PLATFORM(IOS_FAMILY). See
<https://webkit.org/b/210216>.

Can https://webkit.org/b/210216 be closed after this?

> Source/WebCore/rendering/RenderLayer.cpp:6973
> +#if ENABLE(ASYNC_SCROLLING)

This doesn't feel like quite the right #ifdef; isn't it also if we have touch
events or editable regions?