[webkit-reviews] review denied: [Bug 184664] [macOS] Don't establish unneeded Dock connections : [Attachment 338079] Patch (Part 2)

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 184664: [macOS] Don't establish unneeded Dock connections
https://bugs.webkit.org/show_bug.cgi?id=184664

Attachment 338079: Patch (Part 2)

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




--- Comment #13 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 338079
  --> https://bugs.webkit.org/attachment.cgi?id=338079
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.