[webkit-reviews] review granted: [Bug 107216] Don't initialize AppKit for processes that don't use it : [Attachment 183353] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 107216: Don't initialize AppKit for processes that don't use it
https://bugs.webkit.org/show_bug.cgi?id=107216

Attachment 183353: Patch
https://bugs.webkit.org/attachment.cgi?id=183353&action=review

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


r=me, but please double-check [[NSApplication sharedApplication]
_installAutoreleasePoolsOnCurrentThreadIfNecessary] usage. It looks very
suspicious to me.

> Source/WebKit2/PluginProcess/mac/PluginProcessMainMac.mm:65
> +	   [NSApplication sharedApplication];
> +
> +	   // Installs autorelease pools on the current CFRunLoop which
prevents memory from accumulating between user events.
> +	   // FIXME: Remove when <rdar://problem/8929426> is fixed.
> +	   [[NSApplication sharedApplication]
_installAutoreleasePoolsOnCurrentThreadIfNecessary];

This code is confusing.

1. We call [NSApplication sharedApplication] twice, without any clear reason.
There was a previously a FIXME about WebCore::RunLoop limitations that sounded
obsolete (NSApp is equivalent to [NSApplication sharedApplication], and RunLoop
uses NSApp).

2. PluginProcess doesn't use a plain CFRunLoop, so the comment is not
immediately helpful at best, or misleading otherwise. Perhaps we actually need
this call in processes that do not use NSApplication run loop, not in those
that do? But do we even have a "current CFRunLoop" here?

> Source/WebKit2/PluginProcess/mac/PluginProcessMainMac.mm:71
> +	       // We are never going to get to the actual initialization, so
initialize WebKit2 now.

What is "actual initialization"?

> Source/WebKit2/WebProcess/mac/WebProcessMainMac.mm:42
>  #import <wtf/text/WTFString.h>
> +#import <WebCore/RunLoop.h>

Alphabetic ordering.

> Source/WebKit2/WebProcess/mac/WebProcessServiceEntryPoints.mm:85
> +    WebCore::RunLoop::setUseApplicationRunLoopOnMainRunLoop();

Can we be "using namespace WebCore" in this file?