[webkit-reviews] review requested: [Bug 198666] New Layout Test landed flaky [Mac WK1] resize-observer/element-leak.html is a flaky failure : [Attachment 371647] Patch

[bugzilla-daemon at webkit.org]

cathiechen <cathiechen at igalia.com> has asked  for review:
Bug 198666: New Layout Test landed flaky [Mac WK1]
resize-observer/element-leak.html is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=198666

Attachment 371647: Patch

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




--- Comment #11 from cathiechen <cathiechen at igalia.com> ---
Comment on attachment 371647
  --> https://bugs.webkit.org/attachment.cgi?id=371647
Patch

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

Hi Fred,

Thanks for the review.

>> LayoutTests/ChangeLog:9
>> +	    We don't need 1000 element to confirm the GC problem. So in order
to reduce the fake alarm.
> 
> 1000 elements*
> There should be a comma at the end of the line.
> What is the "fake alarm", a timeout?

Done. Yes, it's a timeout.

>> LayoutTests/resize-observer/resources/element-leak-frame.html:18
>> +var container = document.createElement('p');
> 
> p's content model is "phrasing content", so it is not valid to put a div
element inside.
> https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element
> 
> You can make container a div instead.

Done.

>> LayoutTests/resize-observer/resources/element-leak-frame.html:23
>> +document.body.appendChild(container);
> 
> I don't know if this change is necessary, but it should be explained in the
ChangeLog.

Done. This change could reduce the times to modify DOM tree directly.