Bug 199209 - [Catalina] Enable WebKit build
Summary: [Catalina] Enable WebKit build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on: 199279 199705
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-25 15:57 PDT by Jonathan Bedard
Modified: 2019-07-11 03:50 PDT (History)
14 users (show)

See Also:


Attachments
patch-1 (11.72 KB, patch)
2019-06-25 16:44 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-2 (11.73 KB, patch)
2019-06-25 16:53 PDT, Jonathan Bedard
jbedard: commit-queue-
Details | Formatted Diff | Diff
patch-3 (11.99 KB, patch)
2019-06-26 09:18 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-4 (11.99 KB, patch)
2019-06-26 09:35 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-5 (12.05 KB, patch)
2019-06-26 10:06 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-6 (12.17 KB, patch)
2019-06-26 16:59 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-7 (14.38 KB, patch)
2019-07-03 13:04 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
patch-8 (13.56 KB, patch)
2019-07-03 15:08 PDT, Jonathan Bedard
darin: review+
Details | Formatted Diff | Diff
patch-for-landing (13.46 KB, patch)
2019-07-03 15:42 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2019-06-25 15:57:21 PDT
WebKit should build with the public Catalina SDK.
Comment 1 Jonathan Bedard 2019-06-25 16:44:04 PDT
Created attachment 372875 [details]
patch-1
Comment 2 Build Bot 2019-06-25 16:46:05 PDT
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.
Comment 3 Jonathan Bedard 2019-06-25 16:53:11 PDT
Created attachment 372876 [details]
patch-2
Comment 4 Jonathan Bedard 2019-06-25 16:59:15 PDT
This will block enabling the WebKit build on iOS 13, which will (as usual) be a much larger change.
Comment 5 Jiewen Tan 2019-06-25 17:06:25 PDT
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?
Comment 6 Jonathan Bedard 2019-06-25 17:59:18 PDT
Comment on attachment 372876 [details]
patch-2

Need to fix the Mojave build
Comment 7 Jonathan Bedard 2019-06-26 08:58:45 PDT
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.
Comment 8 Jonathan Bedard 2019-06-26 09:18:44 PDT
Created attachment 372928 [details]
patch-3
Comment 9 Build Bot 2019-06-26 09:20:46 PDT
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.
Comment 10 Jonathan Bedard 2019-06-26 09:35:57 PDT
Created attachment 372930 [details]
patch-4
Comment 11 Jonathan Bedard 2019-06-26 10:06:00 PDT
Created attachment 372933 [details]
patch-5
Comment 12 Build Bot 2019-06-26 11:11:38 PDT
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.
Comment 13 Radar WebKit Bug Importer 2019-06-26 16:29:12 PDT
<rdar://problem/52217849>
Comment 14 Jonathan Bedard 2019-06-26 16:59:43 PDT
Created attachment 372970 [details]
patch-6

Got this one building on Internal Catalina too.
Comment 15 Darin Adler 2019-06-26 18:45:14 PDT
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.
Comment 16 Jonathan Bedard 2019-06-26 19:18:14 PDT
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.
Comment 17 Tim Horton 2019-06-27 11:05:57 PDT
(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
Comment 18 Tim Horton 2019-06-27 11:08:13 PDT
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...
Comment 19 Timothy Hatcher 2019-06-27 11:21:20 PDT
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.
Comment 20 Jonathan Bedard 2019-07-03 13:03:34 PDT
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.
Comment 21 Jonathan Bedard 2019-07-03 13:04:21 PDT
Created attachment 373407 [details]
patch-7
Comment 22 Jonathan Bedard 2019-07-03 13:43:38 PDT
Comment on attachment 373407 [details]
patch-7

Running tests locally now, things build and binaries appear to work.
Comment 23 Darin Adler 2019-07-03 14:42:57 PDT
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.
Comment 24 Jonathan Bedard 2019-07-03 15:08:41 PDT
Created attachment 373418 [details]
patch-8
Comment 25 Darin Adler 2019-07-03 15:13:45 PDT
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.
Comment 26 Jonathan Bedard 2019-07-03 15:42:02 PDT
Created attachment 373425 [details]
patch-for-landing
Comment 27 WebKit Commit Bot 2019-07-03 16:26:10 PDT
Comment on attachment 373425 [details]
patch-for-landing

Clearing flags on attachment: 373425

Committed r247117: <https://trac.webkit.org/changeset/247117>
Comment 28 WebKit Commit Bot 2019-07-03 16:26:12 PDT
All reviewed patches have been landed.  Closing bug.