[webkit-reviews] review granted: [Bug 186436] [Cocoa] Remove all uses of NSAutoreleasePool as part of preparation for ARC : [Attachment 342292] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Darin Adler
<darin at apple.com>'s request for review:
Bug 186436: [Cocoa] Remove all uses of NSAutoreleasePool as part of preparation
for ARC
https://bugs.webkit.org/show_bug.cgi?id=186436

Attachment 342292: Patch

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




--- Comment #2 from Anders Carlsson <andersca at apple.com> ---
Comment on attachment 342292
  --> https://bugs.webkit.org/attachment.cgi?id=342292
Patch

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

> Source/WTF/wtf/AutodrainedPool.h:37
> +// This class allows non-Objective-C C++ code to create an autorelease pool.
> +// It should not be used in Objective-C++ code; instead @autoreleasepool
should be used.
> +// It can be used in cross-platform code; will compile down to nothing for
non-Cocoa platforms.

Can you enforce this by using #error if __OBJC__ is defined?

> Source/WTF/wtf/AutodrainedPoolMac.mm:37
> +    : m_pool(NSPushAutoreleasePool(0))

{ } initialization?

> Source/WebKitLegacy/mac/History/WebHistory.mm:601
> +    // FIXME: If we can test and see good performance draining the
autorelease pool every time through the loop,
> +    // instead of once every 50 iterations, then we should switch to
@autoreleasepool and get rid of the use of
> +    // FoundationSPI.h in this source file.

I'd go a step further and just use @autoreleasepool here - it should be fast
enough and given that WebKit Legacy is deprecated I don't think it matters even
if it's a little slower.

> Source/WebKitLegacy/mac/WebView/WebView.mm:7908
> +	       // FIXME: If we can test and see good performance draining the
autorelease pool every time
> +	       // through the loop, instead of once every 10 iterations, then
we should switch to
> +	       // @autoreleasepool and get rid of the use of FoundationSPI.h in
this source file.

Even more true here, since this is SPI that Safari no longer uses - not sure if
anyone else does.