Bug 184664 – [macOS] Don't establish unneeded Dock connections

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

The WebContent and Plugin processes don't need to interact with the dock (this type of stuff is all handled in the UIProcess).

Tell AppKit that we don't want to use a dock connection so that we never both opening one up.

[Brent Fulgham]

<rdar://problem/16863698>

[Brent Fulgham]

Created attachment 338038 [details]
Patch

[Brent Fulgham]

Looks like PluginProcess also needs:

#import <pal/spi/mac/NSApplicationSPI.h>

[Per Arne Vollan]

Comment on attachment 338038 [details]
Patch

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

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:527
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400

Is PLATFORM(MAC) needed here?

[Brent Fulgham]

Comment on attachment 338038 [details]
Patch

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

>> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:527
>> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> 
> Is PLATFORM(MAC) needed here?

Oh, probably not.

It also needs this:
#import <pal/spi/mac/NSApplicationSPI.h>

[Brent Fulgham]

Created attachment 338054 [details]
Patch (Part 1)

[WebKit Commit Bot]

Comment on attachment 338054 [details]
Patch (Part 1)

Clearing flags on attachment: 338054

Committed r230689: <https://trac.webkit.org/changeset/230689>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

No issue for the network process? Maybe that one doesn’t use NSApplication?

[Brent Fulgham]

(In reply to Darin Adler from comment #9)
> No issue for the network process? Maybe that one doesn’t use NSApplication?

Correct. The Network and Storage processes init with 'SetApplicationIsDaemon', which bypass NSApplication and all kinds of other launch cruft.

[Brent Fulgham]

Reopened: This isn't quite right for the Plugin process.

[Brent Fulgham]

Created attachment 338079 [details]
Patch (Part 2)

[David Kilzer (:ddkilzer)]

Comment on attachment 338079 [details]
Patch (Part 2)

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

r- to fix build failure in XPCServiceMain.mm, and fixing WebContentService.xcconfig for non-macOS platforms.

> Source/WebKit/Configurations/PluginService.32.xcconfig:38
> +OTHER_CFLAGS = $(inherited) -D WK_LINKS_TO_APPKIT=1

Nit: missing semi-colon at end of line.

Consider using WK_APPKIT_CFLAGS here as well for consistency.

> Source/WebKit/Configurations/PluginService.64.xcconfig:35
> +OTHER_CFLAGS = $(inherited) -D WK_LINKS_TO_APPKIT=1

Nit: missing semi-colon at end of line.

Consider using WK_APPKIT_CFLAGS here as well for consistency.

> Source/WebKit/Configurations/WebContentService.xcconfig:44
> +OTHER_CFLAGS = $(inherited) -D WK_LINKS_TO_APPKIT=1

Don't you want something like this since this xcconfig file is used by both iOS and macOS?

WK_APPKIT_CFLAGS = $(WK_APPKIT_CFLAGS_$(WK_PLATFORM_NAME));
WK_APPKIT_CFLAGS_macosx = -D WK_LINKS_TO_APPKIT=1;

OTHER_CFLAGS = $(inherited) $(WK_APPKIT_CFLAGS);

This probably isn't needed (except for consistency) in the PluginService.NN.xcconfig files.

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:34
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 && defined(WK_LINKS_TO_APPKIT) && WK_LINKS_TO_APPKIT

You do need PLATFORM(MAC) here according to the EWS build failure because __MAC_OS_X_VERSION_MIN_REQUIRED is undefined on iOS and WebKit builds with -Wundef and -Werror:

#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400 && defined(WK_LINKS_TO_APPKIT) && WK_LINKS_TO_APPKIT

You don't need PLATFORM(MAC) below because it's already inside an #if PLATFORM(MAC)/#endif pair.

Comment on attachment 338079 [details]
Patch (Part 2)

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

>> Source/WebKit/Configurations/PluginService.32.xcconfig:38
>> +OTHER_CFLAGS = $(inherited) -D WK_LINKS_TO_APPKIT=1
> 
> Nit: missing semi-colon at end of line.
> 
> Consider using WK_APPKIT_CFLAGS here as well for consistency.

If you wanted to control this using preprocessor definitions, then the right build setting to use would be GCC_PREPROCESSOR_DEFINITIONS, along the line of
    GCC_PREPROCESSOR_DEFINITIONS = $(inherited) USE_APPKIT;
(which would then allow you to use the USE() macro from WTF. Seeing how this is only used for the one source file, you could also put -DUSE_APPKIT in the Compiler Flags column of the Compile Sources build phase for the relevant targets in Xcode’s project editor.

However, the effort to determine this at build time is probably unnecessary. The most straightforward way I can think of doing this is, under an #if PLATFORM(MAC) guard, to do
    if (Class nsApplicationClass = NSClassFromString(@"NSApplication"))
        [nsApplicationClass _preventDockConnections];

[Brent Fulgham]

Created attachment 338117 [details]
Patch

[Per Arne Vollan]

Comment on attachment 338117 [details]
Patch

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

R=me.

> Source/WebKit/PluginProcess/mac/PluginProcessMac.mm:-532
> -#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> -    // We don't need to talk to the dock.
> -    if ([NSApplication respondsToSelector:@selector(_preventDockConnections)])
> -        [NSApplication _preventDockConnections];
> -#endif

Should we also remove this code from the WebContent process?

[Brent Fulgham]

I don't understand why I'm getting this error inside a #if PLATFORM(MAC) guard when building for iOS-simulator.

Volumes/Data/EWS/WebKit/Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:34:5: error: '__MAC_OS_X_VERSION_MIN_REQUIRED' is not defined, evaluates to 0 [-Werror,-Wundef]
#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
    ^
1 error generated.

[Per Arne Vollan]

Comment on attachment 338117 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:34
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400

I think #if PLATFORM(MAC) should be added here.

[Per Arne Vollan]

Comment on attachment 338117 [details]
Patch

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

>> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:34
>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
> 
> I think #if PLATFORM(MAC) should be added here.

It seems this #if directive is not already inside a #if PLATFORM(MAC) directive.

[Brent Fulgham]

Committed r230714: <https://trac.webkit.org/changeset/230714>