Bug 107216

Summary: Don't initialize AppKit for processes that don't use it
Product: WebKit Reporter: Sam Weinig <sam>
Component: WebKit2Assignee: Sam Weinig <sam>
Severity: Normal CC: ap
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch ap: review+

Description Sam Weinig 2013-01-17 19:29:52 PST
Don't initialize AppKit for processes that don't use it
Comment 1 Sam Weinig 2013-01-17 19:35:00 PST
Created attachment 183353 [details]
Comment 2 Alexey Proskuryakov 2013-01-18 00:24:46 PST
Comment on attachment 183353 [details]

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?
Comment 3 Alexey Proskuryakov 2013-01-18 00:26:28 PST
Also, can we ASSERT in RunLoop::setUseApplicationRunLoopOnMainRunLoop() now?
Comment 4 Sam Weinig 2013-01-18 18:55:28 PST
Committed r140230: <http://trac.webkit.org/changeset/140230>