WebKit should build with the public Catalina SDK.
Created attachment 372875 [details] patch-1
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.
Created attachment 372876 [details] patch-2
This will block enabling the WebKit build on iOS 13, which will (as usual) be a much larger change.
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 on attachment 372876 [details] patch-2 Need to fix the Mojave build
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.
Created attachment 372928 [details] patch-3
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.
Created attachment 372930 [details] patch-4
Created attachment 372933 [details] patch-5
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.
<rdar://problem/52217849>
Created attachment 372970 [details] patch-6 Got this one building on Internal Catalina too.
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 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.
(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 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 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 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.
Created attachment 373407 [details] patch-7
Comment on attachment 373407 [details] patch-7 Running tests locally now, things build and binaries appear to work.
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.
Created attachment 373418 [details] patch-8
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.
Created attachment 373425 [details] patch-for-landing
Comment on attachment 373425 [details] patch-for-landing Clearing flags on attachment: 373425 Committed r247117: <https://trac.webkit.org/changeset/247117>
All reviewed patches have been landed. Closing bug.