[webkit-reviews] review granted: [Bug 197908] resize-observer/element-leak.html fails on Windows platform : [Attachment 371597] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 197908: resize-observer/element-leak.html fails on Windows platform
https://bugs.webkit.org/show_bug.cgi?id=197908

Attachment 371597: Patch

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




--- Comment #19 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 371597
  --> https://bugs.webkit.org/attachment.cgi?id=371597
Patch

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

r=me assuming you've verified that the test still catches the original GC bug
(you can confirm this by reverting your code change temporarily and make sure
the test fails)

> LayoutTests/ChangeLog:3
> +	   Make element-leak.html tests 20 iframes

Les go back to the original title. That describes more of the symptom.
Telling what change we're making doesn't explain why, and we generally prefer
why over what comments.

> LayoutTests/resize-observer/element-leak.html:18
> +	       let ifrm = document.createElement("iframe");

Please spell out iframe.

> LayoutTests/resize-observer/element-leak.html:20
> +	       ifrm.setAttribute("src", "resources/element-leak-frame.html");

You can use iframe.src = ~ instead.

> LayoutTests/resize-observer/element-leak.html:25
> +	   let notifyTimer = setTimeout(() => reject("Waiting Notified timed
out."), 10000);

There is no need to have a separate timer like this. testharness.js has a
timeout of its own.
If any, pass an option to promise_test(~) to adjust the timeout.