[webkit-reviews] review granted: [Bug 172545] [SVG] Leak in SVGAnimatedListPropertyTearOff : [Attachment 314852] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 172545: [SVG] Leak in SVGAnimatedListPropertyTearOff
https://bugs.webkit.org/show_bug.cgi?id=172545

Attachment 314852: Patch

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




--- Comment #28 from Darin Adler <darin at apple.com> ---
Comment on attachment 314852
  --> https://bugs.webkit.org/attachment.cgi?id=314852
Patch

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

Thanks so much for adding a test. Makes a big difference! I hope that adding
the explicit calls to collect are sufficient.

Maybe we should come back and change numberOfLiveNodes so that it automatically
causes garbage collection. There is no use in counting nodes if you don’t force
collection first. And then we can simplify this test and others. Also seems
like numberOfLiveNodes should be an attribute, not an operation.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:9
> + function log(message) {
> +	var logDiv = document.getElementById('log');
> +	logDiv.appendChild(document.createTextNode(message));
> + }

This function won’t work very well if you call it more than once. All the
messages will be glued together into a single long string with no spaces or
line breaks in between them.

I agree with an earlier commenter’s suggestion that we make this a more normal
test using the script-tests script and macros like shouldBe. Not critical, but
less strange.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:19
> +	elem.setAttributeNS(null,"id", "rect");
> +	elem.setAttributeNS(null,"x",50);
> +	elem.setAttributeNS(null,"y",50);
> +	elem.setAttributeNS(null,"width",50);
> +	elem.setAttributeNS(null,"height",50);
> +	elem.setAttributeNS(null,"fill", "blue");

These could use normal setAttribute; no need to use setAttributeNS.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:38
> + var originalLiveElements = 0;

No need for this global variable.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:45
> +	if (!window.testRunner) {
> +	    log("This test requires testRunner");
> +	    return;
> +	}

What this test actually requires is window.internals and window.GCController,
not window.testRunner; but I suppose that’s a pedantic distinction. The use of
testRunner.dumpAsText() is completely optional, but the use of
numberOfLiveNodes and GCController is not.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:48
> +	testRunner.waitUntilDone();

This is not needed for a test that runs synchronously like this one does.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:51
> +	originalLiveElements = window.internals.numberOfLiveNodes();

Can just use internals, no need for window.internals. This can just be a "var",
no need to use a global variable, since this is a synchronous test.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:58
> +	var delta = window.internals.numberOfLiveNodes() -
originalLiveElements;

Ditto.

> LayoutTests/svg/animations/animation-leak-list-property-instances.html:64
> +	testRunner.notifyDone();

This is not needed for a test that runs synchronously like this one does.