Bug 199209 – [Catalina] Enable WebKit build

• Component: Platform
• Status: RESOLVED FIXED

[Jonathan Bedard]

WebKit should build with the public Catalina SDK.

[Jonathan Bedard]

Created attachment 372875 [details]
patch-1

[EWS Watchlist]

Attachment 372875 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/spi/cocoa/SecuritySPI.h:60:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 372876 [details]
patch-2

[Jonathan Bedard]

This will block enabling the WebKit build on iOS 13, which will (as usual) be a much larger change.

[Jiewen Tan]

Comment on attachment 372876 [details]
patch-2

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

The AppSSO part looks good to me.

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:202
> +#if HAVE(APP_SSO)

Good catch!

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:30
> +#include <pal/spi/cocoa/AppSSOSPI.h>

Good catch!

> Tools/TestWebKitAPI/Configurations/Base.xcconfig:118
> +WK_PRIVATE_FRAMEWORKS_DIR = $(WK_PRIVATE_FRAMEWORKS_DIR_$(USE_INTERNAL_SDK));

Do we need to remove the LDFLAG WK_AUTHKIT_LDFLAGS in TestWebKitAPI.xcconfig?

Also, maybe we should move this to TestWebKitAPI.xcconfig?

[Jonathan Bedard]

Comment on attachment 372876 [details]
patch-2

Need to fix the Mojave build

[Jonathan Bedard]

Comment on attachment 372876 [details]
patch-2

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

>> Tools/TestWebKitAPI/Configurations/Base.xcconfig:118
>> +WK_PRIVATE_FRAMEWORKS_DIR = $(WK_PRIVATE_FRAMEWORKS_DIR_$(USE_INTERNAL_SDK));
> 
> Do we need to remove the LDFLAG WK_AUTHKIT_LDFLAGS in TestWebKitAPI.xcconfig?
> 
> Also, maybe we should move this to TestWebKitAPI.xcconfig?

I think we want to keep the LDFLAGs for TestWebKitAPI...otherwise I don't think we could actually run AuthKit tests.

[Jonathan Bedard]

Created attachment 372928 [details]
patch-3

[EWS Watchlist]

Attachment 372928 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:210:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:222:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jonathan Bedard]

Created attachment 372930 [details]
patch-4

[Jonathan Bedard]

Created attachment 372933 [details]
patch-5

[EWS Watchlist]

Attachment 372933 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/mac/WebView/WebView.mm:360:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:210:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:223:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 3 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/52217849>

[Jonathan Bedard]

Created attachment 372970 [details]
patch-6

Got this one building on Internal Catalina too.

[Darin Adler]

Comment on attachment 372970 [details]
patch-6

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

> Source/WTF/wtf/spi/cocoa/SecuritySPI.h:61
> +#define _SECURITY_SECTRUSTEDAPPLICATION_H_

Need a comment explaining why this is needed. It’s normal to have this file define things. But it’s quite another thing, and much more advanced, to have it define header guards to work around unavailability macros. Also seems to me that needs to be done in the prefix, not the SPI header.

> Source/WebCore/WebCorePrefix.h:154
> +#if !USE(APPLE_INTERNAL_SDK)
> +#define _SECURITY_SECTRUSTEDAPPLICATION_H_
> +#endif

Need a comment explaining why this is valuable and needed. Not just in the change log.

Given this, do we also need the #define in SecuritySPI.h above? I presume/hope not.

> Source/WebKit/WebKit2Prefix.h:41
> +#define _SECURITY_SECTASK_H_

Need a comment explaining why this is valuable and needed. Not just in the change log.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:215
> +#if !USE(APPLE_INTERNAL_SDK) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500
> +{
> +@protected
> +    NSArray<NSView *> *_subviews;
> +}
> +#endif

Surprised a protected field is SPI. And surprised that defining it in a category does the right thing. Are you sure this technique is correct? Gets a binary that works, not just compiles?

Also, if we have something this tricky, I think it eventually needs to move into an SPI header, not just in a .mm file, and especially not in two different .mm files.

[Jonathan Bedard]

Comment on attachment 372970 [details]
patch-6

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

>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:215
>> +#endif
> 
> Surprised a protected field is SPI. And surprised that defining it in a category does the right thing. Are you sure this technique is correct? Gets a binary that works, not just compiles?
> 
> Also, if we have something this tricky, I think it eventually needs to move into an SPI header, not just in a .mm file, and especially not in two different .mm files.

As far as I can tell, the binaries work, but I only ran API tests.

I would like to know if anyone else knows more about these protected NSView and NSWindow bits, because what's even more surprising is that these were available in the Mojave headers (if you look, usage of _subviews and _borderView isn't protected by Catalina macros). This seems to get everything working, but I agree that it doesn't seem quite right.

[Tim Horton]

(In reply to Jonathan Bedard from comment #16)
> I would like to know if anyone else knows more about these protected NSView
> and NSWindow bits, because what's even more surprising is that these were
> available in the Mojave headers (if you look, usage of _subviews and

not surprising, they're trying to tighten it up. +xenon who I think knows things

[Tim Horton]

Comment on attachment 372970 [details]
patch-6

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

>>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:215
>>> +#endif
>> 
>> Surprised a protected field is SPI. And surprised that defining it in a category does the right thing. Are you sure this technique is correct? Gets a binary that works, not just compiles?
>> 
>> Also, if we have something this tricky, I think it eventually needs to move into an SPI header, not just in a .mm file, and especially not in two different .mm files.
> 
> As far as I can tell, the binaries work, but I only ran API tests.
> 
> I would like to know if anyone else knows more about these protected NSView and NSWindow bits, because what's even more surprising is that these were available in the Mojave headers (if you look, usage of _subviews and _borderView isn't protected by Catalina macros). This seems to get everything working, but I agree that it doesn't seem quite right.

Agree with Darin, I don't /think/ this will work right? But the fallout could be subtle. You could test the functionality of your faux _subviews in a tiny test app to see...

[Timothy Hatcher]

Comment on attachment 372970 [details]
patch-6

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

>>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:215
>>> +#endif
>> 
>> Surprised a protected field is SPI. And surprised that defining it in a category does the right thing. Are you sure this technique is correct? Gets a binary that works, not just compiles?
>> 
>> Also, if we have something this tricky, I think it eventually needs to move into an SPI header, not just in a .mm file, and especially not in two different .mm files.
> 
> As far as I can tell, the binaries work, but I only ran API tests.
> 
> I would like to know if anyone else knows more about these protected NSView and NSWindow bits, because what's even more surprising is that these were available in the Mojave headers (if you look, usage of _subviews and _borderView isn't protected by Catalina macros). This seems to get everything working, but I agree that it doesn't seem quite right.

The _subviews ivar is still there, just not published in the public header anymore. It is published in the internal SDK version of the header. However, putting this in a category likely builds but fails to do what we expect.

I forgot we asked AppKit to add a method to let us set this ivar (and they added it in Catalina). We should use that instead. I'll post a patch for that on bug 199279.

[Jonathan Bedard]

Comment on attachment 372970 [details]
patch-6

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

>>>>> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:215
>>>>> +#endif
>>>> 
>>>> Surprised a protected field is SPI. And surprised that defining it in a category does the right thing. Are you sure this technique is correct? Gets a binary that works, not just compiles?
>>>> 
>>>> Also, if we have something this tricky, I think it eventually needs to move into an SPI header, not just in a .mm file, and especially not in two different .mm files.
>>> 
>>> As far as I can tell, the binaries work, but I only ran API tests.
>>> 
>>> I would like to know if anyone else knows more about these protected NSView and NSWindow bits, because what's even more surprising is that these were available in the Mojave headers (if you look, usage of _subviews and _borderView isn't protected by Catalina macros). This seems to get everything working, but I agree that it doesn't seem quite right.
>> 
>> Agree with Darin, I don't /think/ this will work right? But the fallout could be subtle. You could test the functionality of your faux _subviews in a tiny test app to see...
> 
> The _subviews ivar is still there, just not published in the public header anymore. It is published in the internal SDK version of the header. However, putting this in a category likely builds but fails to do what we expect.
> 
> I forgot we asked AppKit to add a method to let us set this ivar (and they added it in Catalina). We should use that instead. I'll post a patch for that on bug 199279.

r246905 doesn't fix the problem in WebView.mm....I think, as Darin said, this needs to go into a SPI header.

[Jonathan Bedard]

Created attachment 373407 [details]
patch-7

[Jonathan Bedard]

Comment on attachment 373407 [details]
patch-7

Running tests locally now, things build and binaries appear to work.

[Darin Adler]

Comment on attachment 373407 [details]
patch-7

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

> Source/WebCore/PAL/pal/spi/mac/NSViewSPI.h:58
> +#if !HAVE(SUBVIEWS_IVAR_SPI)
> +@implementation NSView (SubviewsIvar)
> +
> +- (void)_setSubviewsIvar:(NSMutableArray<__kindof NSView *> *)subviews {
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +    _subviews = subviews;
> +    ALLOW_DEPRECATED_DECLARATIONS_END
> +}
> +
> +- (NSMutableArray<__kindof NSView *> *)_subviewsIvar {
> +    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
> +    return (NSMutableArray *)_subviews;
> +    ALLOW_DEPRECATED_DECLARATIONS_END
> +}
> +
> +@end
> +#endif

This part must not be in a header. It needs to be in a .m or .mm file. Perhaps we can leave it in WebHTMLView.mm for now.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5812
>      ALLOW_DEPRECATED_DECLARATIONS_BEGIN

Should remove this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5817
>      ALLOW_DEPRECATED_DECLARATIONS_END

And this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5821
>      ALLOW_DEPRECATED_DECLARATIONS_BEGIN

And this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:5824
>      ALLOW_DEPRECATED_DECLARATIONS_END

And this.

[Jonathan Bedard]

Created attachment 373418 [details]
patch-8

[Darin Adler]

Comment on attachment 373418 [details]
patch-8

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

> Source/WTF/ChangeLog:9
> +        * wtf/spi/cocoa/SecuritySPI.h: Ensure that we define SecTrustedApplicationCreateFromPath before
> +        other headers declare it unavailable on Mac.

I don’t think "before" is correct in this change log comment. There’s no ordering issue. We prevent it from being defined as unavailable with a different technique, it’s not about ordering.

[Jonathan Bedard]

Created attachment 373425 [details]
patch-for-landing

[WebKit Commit Bot]

Comment on attachment 373425 [details]
patch-for-landing

Clearing flags on attachment: 373425

Committed r247117: <https://trac.webkit.org/changeset/247117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.