[webkit-reviews] review denied: [Bug 239506] Fix accelerated compositing for null ScrollingCoordinator and overflow scroll in LayerAncestorClippingStack. : [Attachment 458561] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Jigen Zhou
<jigen.zhou at sony.com>'s request for review:
Bug 239506: Fix accelerated compositing for null ScrollingCoordinator and
overflow scroll in LayerAncestorClippingStack.
https://bugs.webkit.org/show_bug.cgi?id=239506

Attachment 458561: Patch

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




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

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

> LayoutTests/compositing/overflow/overflow-scroll-hidden-swap-crash.html:64
> +		   }, 100);
> +		 }, 100);
> +	       }, 100);
> +	     }, 100);
> +	   }, 100);
> +	 }, 100);

This test will take 600ms to run, and seems to be trying to catch
non-deterministic behavior. It needs to run quicker (imagine if all 40,000
tests took > 500ms).

> Source/WebCore/rendering/LayerAncestorClippingStack.cpp:69
> +//
> +// If scrollingCoordinator is null, overflowScrollProxyNodeID should be 0
for all existing entries
> +// of the clipping stack. The non-zero for overflowScrollProxyNodeID can
only possibily be set in
> +// RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole, but it
won't be called for null
> +// scrollingCoordinator.
> +//

Weird for this comment to be here and not next to the relevant code.

> Source/WebCore/rendering/LayerAncestorClippingStack.cpp:147
> +	   if (existingEntry.overflowScrollProxyNodeID &&
existingEntry.clipData.isOverflowScroll && !clipDataEntry.isOverflowScroll) {

If existingEntry.clipData.isOverflowScroll is set, I would expect a non-0
existingEntry.overflowScrollProxyNodeID. When does this happen?