Some Apple internal clients need to use Appfail to build due to redeclared AppKit types in WebKitLegacy
Created attachment 455741 [details] Patch
Created attachment 455742 [details] Patch
<rdar://problem/90497026>
Comment on attachment 455742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455742&action=review r=me, but please swap the logic as noted before landing. Also, make sure this builds on iOS before landing. It's unlikely that this patch caused layout test failures, either. > Source/WebKitLegacy/mac/Misc/WebNSViewExtras.h:48 > +#if !TARGET_OS_IPHONE > @interface NSView (WebExtras) > +#else > +@interface WAKView (WebExtras) > +#endif Would be slightly better to reverse the logic here and use `#if TARGET_OS_IPHONE/#else/#endif` instead. > Source/WebKitLegacy/mac/Misc/WebNSViewExtras.h:55 > +#if !TARGET_OS_IPHONE > - (NSView *)_web_superviewOfClass:(Class)viewClass; > +#else > +- (WAKView *)_web_superviewOfClass:(Class)viewClass; > +#endif Ditto. > Source/WebKitLegacy/mac/WebView/WebDocumentPrivate.h:81 > +#if !TARGET_OS_IPHONE > - (NSView *)selectionView; > +#else > +- (WAKView *)selectionView; > +#endif Ditto. > Source/WebKitLegacy/mac/WebView/WebFrameView.h:53 > +#if !TARGET_OS_IPHONE > @interface WebFrameView : NSView > +#else > +@interface WebFrameView : WAKView > +#endif Ditto. > Source/WebKitLegacy/mac/WebView/WebFrameView.h:74 > +#if !TARGET_OS_IPHONE > @property (nonatomic, readonly, strong) NSView<WebDocumentView> *documentView; > +#else > +@property (nonatomic, readonly, strong) WAKView<WebDocumentView> *documentView; > +#endif Ditto. > Source/WebKitLegacy/mac/WebView/WebHTMLViewPrivate.h:118 > +#if !TARGET_OS_IPHONE > - (NSView *)_compositingLayersHostingView; > +#else > +- (WAKView *)_compositingLayersHostingView; > +#endif Ditto. > Source/WebKitLegacy/mac/WebView/WebView.h:133 > +#if !TARGET_OS_IPHONE > @interface WebView : NSView > +#else > +@interface WebView : WAKView > +#endif Ditto.
Created attachment 455831 [details] Patch
Comment on attachment 455742 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455742&action=review >> Source/WebKitLegacy/mac/Misc/WebNSViewExtras.h:48 >> +#endif > > Would be slightly better to reverse the logic here and use `#if TARGET_OS_IPHONE/#else/#endif` instead. Sure. I was kind of trying to match the files that had other !TARGET_OS_IPHONE in them, but I prefer it your way too!
> Some Apple internal Mac Catalyst clients need to use both AppKit and WebKitLegacy. This is never going to work though, with or without this patch? Basic functionality like copy/pasting or running JavaScript is implemented in ways that relies on this never happening.
Tim tells me that I'm not correct, because macCatalyst version of WebKit ends up being used from AppKit. Has someone actually built the public SDK though, to confirm that it's not changing?
Comment on attachment 455831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455831&action=review > Source/WebKitLegacy/mac/Misc/WebDownload.h:87 > +#if TARGET_OS_IPHONE > +- (WAKWindow *)downloadWindowForAuthenticationSheet:(WebDownload *)download; > +#else > - (NSWindow *)downloadWindowForAuthenticationSheet:(WebDownload *)download; > +#endif I'm surprised this change was needed as this appears at the top of the header file: #if TARGET_OS_IPHONE #import <WebKitLegacy/WAKAppKitStubs.h> #endif Did you do a search and replace when looking for AppKit types to update in WebKitLegacy headers? Note that it is not necessary to fix this before landing this patch, but I think including that header is the canonical way this has been solved in the past.
Committed r291978 (248937@main): <https://commits.webkit.org/248937@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455831 [details].
(In reply to Alexey Proskuryakov from comment #8) > Tim tells me that I'm not correct, because macCatalyst version of WebKit > ends up being used from AppKit. > > Has someone actually built the public SDK though, to confirm that it's not > changing? I tried to use the normal Apple tools to do that, but I couldn't get them to work on WebKit. I have a task to diff the SDKs when this lands in a build.
(In reply to David Kilzer (:ddkilzer) from comment #9) > Comment on attachment 455831 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=455831&action=review > > > Source/WebKitLegacy/mac/Misc/WebDownload.h:87 > > +#if TARGET_OS_IPHONE > > +- (WAKWindow *)downloadWindowForAuthenticationSheet:(WebDownload *)download; > > +#else > > - (NSWindow *)downloadWindowForAuthenticationSheet:(WebDownload *)download; > > +#endif > > I'm surprised this change was needed as this appears at the top of the > header file: > > #if TARGET_OS_IPHONE > #import <WebKitLegacy/WAKAppKitStubs.h> > #endif > > Did you do a search and replace when looking for AppKit types to update in > WebKitLegacy headers? > > Note that it is not necessary to fix this before landing this patch, but I > think including that header is the canonical way this has been solved in the > past. This is needed because `NSWindow` isn't always `#define`d to `WAKWindow` in `TARGET_OS_IPHONE` after this patch. Yes I did a find and replace for the types that used to be always `#define`d in WAKAppKitStubs.h to make these changes. The idea is that if `TARGET_OS_IPHONE`, everywhere in WebKitLegacy that says `NSWindow` is always `WAKWindow`. However, in Mac Catalyst, WebKitLegacy can't always `#define` `NSWindow` to `WAKWindow`, so the type needs to be explicitly declared.