[webkit-reviews] review granted: [Bug 197457] The JS wrapper of target in an ResizeObserverEntry should not get collected : [Attachment 370681] Patch

Tue May 28 16:38:20 PDT 2019

Ryosuke Niwa <rniwa at webkit.org> has granted cathiechen
<cathiechen at igalia.com>'s request for review:
Bug 197457: The JS wrapper of target in an ResizeObserverEntry should not get
collected
https://bugs.webkit.org/show_bug.cgi?id=197457

Attachment 370681: Patch

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

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

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

> Source/WebCore/ChangeLog:21
> +	   * bindings/js/JSResizeObserverEntryCustom.cpp: Copied from
Source/WebCore/page/ResizeObserverEntry.idl.

This cpp file isn't a copy of idl file??

> Source/WebCore/page/ResizeObserver.cpp:-177
> -    m_observations.clear();
> -    m_activeObservations.clear();

Please clarify why we're no longer clearing m_observations.clear and 
m_activeObservations here.

> LayoutTests/resize-observer/element-leak.html:13
> +if (window.testRunner)
> +    testRunner.dumpAsText();

No need to call dumpAsText if you're including testharness.js /
testharnessreport.js

> LayoutTests/resize-observer/element-leak.html:23
> +	       if (internals && !internals.isDocumentAlive(documentIdentifier))
{
> +		   clearInterval(handle);

Seems like this test always requires internals?
In that case, it seems like we don't even need to use testharness.js.

> LayoutTests/resize-observer/element-leak.html:32
> +    let test = async_test('test0: Test elements leak');

Can we use promise_test instead?

> LayoutTests/resize-observer/element-leak.html:55
> +	   log('timeout: ');
> +	   test.done();
> +    }, 5000);

Just specify timeout to promise_test instead.

> LayoutTests/resize-observer/element-leak.html:62
> +test0();

Ugh... can we give this more descriptive name like runTest?

>
LayoutTests/resize-observer/resize-observer-entry-keeps-js-wrapper-of-target-al
ive.html:46
> +	   document.querySelectorAll('div').forEach(target => {
target.remove(); });

No need for { ~; }. You can just write `target => target.remove()` here.

>
LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.ht
ml:33
> +    document.querySelectorAll('div').forEach(target => { target.remove();
});

Ditto.

>
LayoutTests/resize-observer/resize-observer-keeps-js-wrapper-of-target-alive.ht
ml:41
> +    document.querySelectorAll('div').forEach(target => {
observer.observe(target); });

Ditto.

> LayoutTests/resize-observer/resources/element-leak-frame.html:1
> +<body></body>

Missing DOCTYPE.

> LayoutTests/resize-observer/resources/element-leak-frame.html:7
> +var ro = new ResizeObserver( entries => {

Please spell out words such as resizeObserver.

> LayoutTests/resize-observer/resources/element-leak-frame.html:8
> +    for (let entry of entries) {

No curly braces around a single line statements.