Bug 238368 – Some Apple internal clients fail to build due to redeclared AppKit types in WebKitLegacy

• Component: Platform
• Status: RESOLVED FIXED

[Ian Anderson]

Some Apple internal clients need to use Appfail to build due to redeclared AppKit types in WebKitLegacy

[Ian Anderson]

Created attachment 455741 [details]
Patch

[Ian Anderson]

Created attachment 455742 [details]
Patch

[Ian Anderson]

<rdar://problem/90497026>

[David Kilzer (:ddkilzer)]

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.

[Ian Anderson]

Created attachment 455831 [details]
Patch

[Ian Anderson]

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!

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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?

[David Kilzer (:ddkilzer)]

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.

[EWS]

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].

[Ian Anderson]

(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.

[Ian Anderson]

(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.