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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 7 15:48:07 PDT 2019

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

Attachment 371597: Patch


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

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.

More information about the webkit-reviews mailing list