[Webkit-unassigned] [Bug 53361] autorelease pools accumulate memory during automatic testing on Webkit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=53361


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80703|review?                     |review-
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2011-02-01 09:20:46 PST ---
(From update of attachment 80703)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.