[webkit-reviews] review granted: [Bug 206884] Asynchronous scrolling of overflow element can enter a recursive loop : [Attachment 389138] Patch

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 206884: Asynchronous scrolling of overflow element can enter a recursive
loop
https://bugs.webkit.org/show_bug.cgi?id=206884

Attachment 389138: Patch

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




--- Comment #6 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 389138
  --> https://bugs.webkit.org/attachment.cgi?id=389138
Patch

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

> Source/WebCore/ChangeLog:9
> +	   while perform asynchronous programmatic scrolling for overflow
elements. In order to break

performing*

> LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html:35
> +    }, `Element async scroll by ${scrollFunction}() shouldn't crash`);

What is the "crash" exactly?

I'm not sure we have a notion of "crashtests" in WPT (yet). Is the goal of this
test to only reproduce the issue rather than to do conformance testing? If so,
maybe we should just have a traditional webkit test without
assert/promise_test, with the minimal repro code and with the text "This test
passes if it does not crash".

> LayoutTests/fast/scrolling/ios/resources/scroll-behavior.js:1
> +function observeScrolling(elements, callback) {

I guess it's ok, but only resetNode/scrollNode seem to be used? I wonder if we
really want all of these or just inline the needed code in the test? Especially
if you put only the minimal code in the html file...