[webkit-reviews] review denied: [Bug 53361] autorelease pools accumulate memory during automatic testing on Webkit2 : [Attachment 80703] new patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Stephanie Lewis <slewis at apple.com>'s
request for review:
Bug 53361: autorelease pools accumulate memory during automatic testing on
Webkit2
https://bugs.webkit.org/show_bug.cgi?id=53361

Attachment 80703: new patch
https://bugs.webkit.org/attachment.cgi?id=80703&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80703&action=review

> Source/WebKit2/Platform/mac/RunLoopMac.mm:30
> +static const int wrapRunLoopWithAutoreleasePoolsPriority = 1000000;

Alexey has pointed out to me many times that we can just use "const int" for
such things, because these constants have internal linkage even if you don’t
specify static. So now I point it out to you.

The type of this constant should be CFIndex, not int.

We need a comment here explaining why this is a good value to use for priority.


> Source/WebKit2/Platform/mac/RunLoopMac.mm:50
> +    default:
> +	   break;

Can you leave this out? What good does the default here do?

> Source/WebKit2/Platform/mac/RunLoopMac.mm:121
> +    if (!m_currentRunLoopEnterObserver) {

We prefer early return to nesting the entire function inside an if. So this
should just say:

    if (m_currentRunLoopEnterObserver) {
	ASSERT(m_currentRunLoopExitObserver);
	return;
    }

And then the rest of the function should not be nested.

>> Source/WebKit2/Platform/mac/RunLoopMac.mm:126
>> +	    // Install a repeating observer to fire on entry to the run current
run loop with a early ordering like -wrapRunLoopWithAutoreleasePoolsOrder. 
CFRunLoop gives highest priority to those observers close to minus infinity.
> 
> Please don't use two spaces between sentences.

It would be clearer to have two priority constants, one for enter and one for
exit, rather than using a single constant, sometimes with a minus sign.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:148
> +void RunLoop::uninstallAutoreleasePoolsOnCurrentRunLoop(void)

No need for the (void) here. That’s a plain C idiom, and never needs to be used
in C++. Just () works.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:153
> +    if (m_currentRunLoopEnterObserver)
> +	   m_currentRunLoopEnterObserver.clear();
> +    if (m_currentRunLoopExitObserver)
> +	   m_currentRunLoopExitObserver.clear();

The best style here would be:

    m_currentRunLoopEnterObserver = nullptr;
    m_currentRunLoopExitObserver = nullptr;

There is no need to check for null before calling clear(), since it does so
already, and assignment to nullptr is preferred over calls to clear().

This function is named “uninstall” but the code here simply releases the
observers, and does not remove them from the run loop. I expect that won’t work
correctly. I believe we need calls to CFRunLoopRemoveObserver here.