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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> 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 80539: patch
https://bugs.webkit.org/attachment.cgi?id=80539&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=80539&action=review

Looks good. I have many style nitpicks though, seems worth another quick review
round.

> Source/WebKit2/Platform/RunLoop.h:33
>  #include <wtf/PassOwnPtr.h>

PLease fix alphabetic sorting.

> Source/WebKit2/Platform/RunLoop.h:143
> +    RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopEnterObserverRef;
> +    RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopExitObserverRef;

WebKit style is m_currentRunLoopEnter/ExitObserver (lower case first letter,
and there is no need for "Ref" suffix.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:30
> +#define WRAP_RUN_LOOP_WITH_AUTORELEASE_POOLS_OBSERVER_ORDER 1000000

We prefer consts to macros.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:36
> +    switch (activity) {
> +	   case kCFRunLoopEntry:

We don't indent case statements.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:43
> +		   const NSAutoreleasePool* pool = static_cast<const
NSAutoreleasePool*>(CFArrayGetValueAtIndex(autoreleasePoolStack, count-1));

Please also use const_cast to remove the const. There should be a space around
"-" in count-1.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:45
> +		   CFArrayRemoveValueAtIndex(autoreleasePoolStack, count-1);

There should be a space around "-" in count-1.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:118
> +// Set up an Autorelease Pool on the main RunLoop
> +// FIXME: remove when <rdar://problem/8929426> is fixed.
> +void RunLoop::installAutoreleasePoolsOnCurrentRunLoop()

Does this function work on the main, or on current run loop? Please bring
comments and function name in sync.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:123
> +	   CFRunLoopObserverRef newRunLoopEnterObserverRef = 0;

Please don't use local CF objects without RetainPtr. I think that here, you
could just assign to m_currentRunLoopEnterObserver immediately.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:125
> +	   CFRunLoopObserverRef newRunLoopExitObserverRef = 0;

Ditto.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:131
> +	   newRunLoopEnterObserverContext.info	   =
CFArrayCreateMutable(kCFAllocatorSystemDefault /* Don't retain the autorelease
pool tokens. */,0,0);
> +	   newRunLoopEnterObserverContext.retain   = CFRetain;	       //
retain the array
> +	   newRunLoopEnterObserverContext.release  = CFRelease;        //
release the array when the context info goes away.
> +	   newRunLoopEnterObserverContext.copyDescription = CFCopyDescription;

Please don't align "=" signs or comments. There should be spaces in ",0,0".

> Source/WebKit2/Platform/mac/RunLoopMac.mm:134
> +	   // Install a repeating observer to fire on exit of the current run
loop with an very late ordering like
kWrapRunLoopWithAutoreleasePoolObserverOrder.  CFRunLoop gives lowest priority
to those observers close to positive infinity.

We don't put two spaces between sentences.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:138
> +	   newRunLoopExitObserverContext.version  = 0;
> +	   newRunLoopExitObserverContext.info	  =
newRunLoopEnterObserverContext.info;
> +	   newRunLoopExitObserverContext.retain   = CFRetain;	      // retain
the array
> +	   newRunLoopExitObserverContext.release  = CFRelease;	      //
release the array when the context info goes away.

Please don't align.

> Source/WebKit2/Platform/mac/RunLoopMac.mm:142
> +	   CFRelease((CFMutableArrayRef)newRunLoopEnterObserverContext.info);  
     // owned by the observer. (only one release is needed since Enter/Exit
point to the same instance).

One space before "//". Please fix grammar in the comment, too.

I would have put newRunLoopEnterObserverContext.info into a local RetainPtr
variable to avoid the need for explicit CFRelease.