Bug 230735 – Add a module map file for PrivateFrameworks/WebKitLegacy

• Component: Platform
• Status: RESOLVED FIXED

[Ian Anderson]

Apple internal use needs the WebKitLegacy framework to have modules when installed in PrivateFrameworks.

[Radar WebKit Bug Importer]

<rdar://problem/83750014>

[Ian Anderson]

Created attachment 439814 [details]
Patch

[Ian Anderson]

(In reply to Radar WebKit Bug Importer from comment #1)
> <rdar://problem/83750014>

Duped to rdar://16833552

[David Kilzer (:ddkilzer)]

Comment on attachment 439814 [details]
Patch

r=me once the fix for Bug 230736 lands.

[David Kilzer (:ddkilzer)]

<rdar://problem/16833552>

[David Kilzer (:ddkilzer)]

Comment on attachment 439814 [details]
Patch

Marking this cq- for now as it needs additional side builds before landing.

[David Kilzer (:ddkilzer)]

Comment on attachment 439814 [details]
Patch

Changing to r- until the macOS build issues are resolved.

[Ian Anderson]

Created attachment 439918 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 439918 [details]
Patch

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

r- but r+ with the "#if defined" macro checks fixed.

> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:29
> +#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)

This is formatted a bit odd compared to other places where macro values are checked, and I'm not entirely sure it's doing what's intended, so I'd suggest fixing it:

#if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST

> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:31
> +#elif !TARGET_OS_IPHONE || (defined USE_APPLE_INTERNAL_SDK && USE_APPLE_INTERNAL_SDK)

Same here:

#elif !TARGET_OS_IPHONE || (defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK)

[David Kilzer (:ddkilzer)]

Comment on attachment 439918 [details]
Patch

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

>> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:29
>> +#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)
> 
> This is formatted a bit odd compared to other places where macro values are checked, and I'm not entirely sure it's doing what's intended, so I'd suggest fixing it:
> 
> #if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST

Heh, the original code was the way you wrote it, but let's format it consistently.

>> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:31
>> +#elif !TARGET_OS_IPHONE || (defined USE_APPLE_INTERNAL_SDK && USE_APPLE_INTERNAL_SDK)
> 
> Same here:
> 
> #elif !TARGET_OS_IPHONE || (defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK)

Ditto.

[Ian Anderson]

Comment on attachment 439918 [details]
Patch

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

>> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:29
>> +#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)
> 
> This is formatted a bit odd compared to other places where macro values are checked, and I'm not entirely sure it's doing what's intended, so I'd suggest fixing it:
> 
> #if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST

I did think it looked weird when I copied it, though it does seem to work since the #else doesn't get hit. Should I just change it here or also in WebDownload.h where I copied it?

[David Kilzer (:ddkilzer)]

(In reply to Ian Anderson from comment #11)
> Comment on attachment 439918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439918&action=review
> 
> >> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:29
> >> +#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)
> > 
> > This is formatted a bit odd compared to other places where macro values are checked, and I'm not entirely sure it's doing what's intended, so I'd suggest fixing it:
> > 
> > #if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST
> 
> I did think it looked weird when I copied it, though it does seem to work
> since the #else doesn't get hit. Should I just change it here or also in
> WebDownload.h where I copied it?

Go ahead and change both places as long as your making the change.  Thanks!

[Ian Anderson]

Created attachment 439949 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 439949 [details]
Patch

r=me but cq- until side builds are completed.

[Ian Anderson]

Created attachment 441851 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 441851 [details]
Patch

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

r-, and I think these need to be fixed at minimum:
- Move #include/#import statements inside header guard macros.
- Add comments after #endif where noted.
- Answer question about <objc/NSObject.h>.  (Might be okay—don't change them all yet.)
- Fix ordering of headers where noted.
- Reuse NSURLDownloadSPI.h in WebDownload.h (or was there a reason this code was copied?).

Thanks!

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:-30
> +#import <CoreGraphics/CoreGraphics.h>
>  #import <WebCore/AbstractPasteboard.h>
>  
>  #if TARGET_OS_IOS
>  
> -struct CGSize;

Why not put the #Import inside #if TARGET_OS_IOS/#endif?

> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:-32
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
>  #import <CoreGraphics/CoreGraphics.h>
> -#import <Foundation/Foundation.h>

Why does this need to go outside `#if TARGET_OS_IPHONE`?  This doesn't seem necessary since the entire contents is wrapped in #if TARGET_OS_IPHONE/#endif.

> Source/WebCore/platform/ios/wak/WAKResponder.h:33
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
> -#import "WKTypes.h"
> -#import <Foundation/Foundation.h>
> +#import <WebCore/WKTypes.h>

Why can't this go inside #if TARGET_OS_IPHONE/#endif?

> Source/WebCore/platform/ios/wak/WAKView.h:29
> +#import <Foundation/Foundation.h>

Ditto.

> Source/WebCore/platform/ios/wak/WAKWindow.h:28
> +#import <Foundation/Foundation.h>

Ditto.

> Source/WebKitLegacy/ios/Misc/WebGeolocationCoreLocationProvider.h:62
> +#endif

Should have a comment here:

#endif // defined(__cplusplus)

> Source/WebKitLegacy/mac/DOM/DOMEventListener.h:27
> +#import <objc/NSObject.h>

Is this a public header?  Why wouldn't we #import <Foundation/NSObject.h> instead?

> Source/WebKitLegacy/mac/DOM/DOMNodeFilter.h:27
> +#import <objc/NSObject.h>

Ditto.

> Source/WebKitLegacy/mac/DOM/DOMXPathNSResolver.h:27
> +#import <objc/NSObject.h>

Ditto.

> Source/WebKitLegacy/mac/DOM/WebDOMOperationsPrivate.h:31
> +#import <Foundation/Foundation.h>
>  #import <WebKitLegacy/WebDOMOperations.h>
>  #import <JavaScriptCore/JSBase.h>

The header order should probably be this (since we include the non-private header first?  Or does that give a check-webkit-style error?

#import <WebKitLegacy/WebDOMOperations.h>
#import <Foundation/Foundation.h>
#import <JavaScriptCore/JSBase.h>

> Source/WebKitLegacy/mac/History/WebHistoryItemPrivate.h:30
> +#import <CoreGraphics/CoreGraphics.h>
>  #import <WebKitLegacy/WebHistoryItem.h>

Ditto.

> Source/WebKitLegacy/mac/Misc/WebDownload.h:33
>  #ifndef WebDownload_h
>  #define WebDownload_h

It's a bit strange that there are #import statements above this header guard.  Can they be moved inside the header guard?

> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
> -#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)
> +#if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST
>  #import <CFNetwork/CFNSURLConnection.h>
> -#elif !TARGET_OS_IPHONE || (defined USE_APPLE_INTERNAL_SDK && USE_APPLE_INTERNAL_SDK)
> +#elif !TARGET_OS_IPHONE || (defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK)
>  #import <Foundation/NSURLDownload.h>
>  #else
>  #import <WebKitLegacy/NSURLDownloadSPI.h>

Both WebDownload.h and NSURLDownloadSPI.h are marked as Private headers, so we should be able to replace all of this with just:

#import <WebKitLegacy/NSURLDownloadSPI.h>

Since that has al the same logic as this block of macros.

> Source/WebKitLegacy/mac/Storage/WebDatabaseQuotaManager.h:27
> +#import <objc/NSObject.h>

Why <objc/NSObject.h> instead of <Foundation/NSObject.h>?

> Source/WebKitLegacy/mac/WebView/WebDeviceOrientationProvider.h:26
> +#import <objc/NSObject.h>

Ditto.

> Source/WebKitLegacy/mac/WebView/WebDeviceOrientationProviderMock.h:27
> -#import "WebDeviceOrientationProvider.h"
> +#import <WebKitLegacy/WebDeviceOrientationProvider.h>
> +#import <objc/NSObject.h>

Ditto.

> Source/WebKitLegacy/mac/WebView/WebGeolocationPosition.h:26
> +#import <objc/NSObject.h>

Ditto.

[Ian Anderson]

Comment on attachment 441851 [details]
Patch

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

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:-30
>> -struct CGSize;
> 
> Why not put the #Import inside #if TARGET_OS_IOS/#endif?

It needs to be outside because that's what's pulling in TargetConditionals and defining TARGET_OS_IOS.

>> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:-32
>> -#import <Foundation/Foundation.h>
> 
> Why does this need to go outside `#if TARGET_OS_IPHONE`?  This doesn't seem necessary since the entire contents is wrapped in #if TARGET_OS_IPHONE/#endif.

Same everywhere else, all of these TARGET_OS_ macros were being used undefined (and failing). We can include TargetConditionals explicitly everywhere, but you get it for free from Foundation and CoreGraphics and we need to include those headers anyway, seemed shorter to pull those out of the TARGET_OS_ chunks.

>> Source/WebKitLegacy/mac/DOM/WebDOMOperationsPrivate.h:31
>>  #import <JavaScriptCore/JSBase.h>
> 
> The header order should probably be this (since we include the non-private header first?  Or does that give a check-webkit-style error?
> 
> #import <WebKitLegacy/WebDOMOperations.h>
> #import <Foundation/Foundation.h>
> #import <JavaScriptCore/JSBase.h>

Yeah, check-webkit-style said no.

>> Source/WebKitLegacy/mac/Misc/WebDownload.h:33
>>  #define WebDownload_h
> 
> It's a bit strange that there are #import statements above this header guard.  Can they be moved inside the header guard?

Sure.

>> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
>>  #import <WebKitLegacy/NSURLDownloadSPI.h>
> 
> Both WebDownload.h and NSURLDownloadSPI.h are marked as Private headers, so we should be able to replace all of this with just:
> 
> #import <WebKitLegacy/NSURLDownloadSPI.h>
> 
> Since that has al the same logic as this block of macros.

I did that in one version of this and I'm trying to remember why I didn't leave it that way. I thought someone had told me to not do it like that but I don't see that comment either. Let me try and see what happens.

>> Source/WebKitLegacy/mac/Storage/WebDatabaseQuotaManager.h:27
>> +#import <objc/NSObject.h>
> 
> Why <objc/NSObject.h> instead of <Foundation/NSObject.h>?

Just because it didn't use anything from Foundation. It's unintuitively worse for build performance to import <Foundation/NSObject.h> than it is to import <Foundation/Foundation.h>, but do you want me to switch all these to <Foundation/Foundation.h>?

[Ian Anderson]

Created attachment 442106 [details]
Patch

[Ian Anderson]

Comment on attachment 441851 [details]
Patch

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

>>> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
>>>  #import <WebKitLegacy/NSURLDownloadSPI.h>
>> 
>> Both WebDownload.h and NSURLDownloadSPI.h are marked as Private headers, so we should be able to replace all of this with just:
>> 
>> #import <WebKitLegacy/NSURLDownloadSPI.h>
>> 
>> Since that has al the same logic as this block of macros.
> 
> I did that in one version of this and I'm trying to remember why I didn't leave it that way. I thought someone had told me to not do it like that but I don't see that comment either. Let me try and see what happens.

Oh now I remember. WebDownload.h is a public header in WebKit so it can't unconditionally import NSURLDownloadSPI.h because that's a private header. (That's going to be a problem later when WebKit gets fully modularized...)

[Ian Anderson]

Created attachment 442115 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 441851 [details]
Patch

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

>>> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:-30
>>> -struct CGSize;
>> 
>> Why not put the #Import inside #if TARGET_OS_IOS/#endif?
> 
> It needs to be outside because that's what's pulling in TargetConditionals and defining TARGET_OS_IOS.

Okay, then why not just import <TargetConditions.h> instead and leave `struct CGSize` unchanged?

The reason we avoided including framework headers was to improve compile times, so this kind of change will undo that work.

I'd really rather see <TargetConditionals.h> imported instead.

>>> Source/WebKitLegacy/mac/DOM/WebDOMOperationsPrivate.h:31
>>>  #import <JavaScriptCore/JSBase.h>
>> 
>> The header order should probably be this (since we include the non-private header first?  Or does that give a check-webkit-style error?
>> 
>> #import <WebKitLegacy/WebDOMOperations.h>
>> #import <Foundation/Foundation.h>
>> #import <JavaScriptCore/JSBase.h>
> 
> Yeah, check-webkit-style said no.

That's probably a bug in check-webkit-style.  You don't have to fix it--I'm just noting it for reference.

>>>> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
>>>>  #import <WebKitLegacy/NSURLDownloadSPI.h>
>>> 
>>> Both WebDownload.h and NSURLDownloadSPI.h are marked as Private headers, so we should be able to replace all of this with just:
>>> 
>>> #import <WebKitLegacy/NSURLDownloadSPI.h>
>>> 
>>> Since that has al the same logic as this block of macros.
>> 
>> I did that in one version of this and I'm trying to remember why I didn't leave it that way. I thought someone had told me to not do it like that but I don't see that comment either. Let me try and see what happens.
> 
> Oh now I remember. WebDownload.h is a public header in WebKit so it can't unconditionally import NSURLDownloadSPI.h because that's a private header. (That's going to be a problem later when WebKit gets fully modularized...)

Ah, thanks for the reminder.  It's weird that it's marked Private in the Xcode project, though?!

$ grep "WebDownload.h in Headers" Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj 
		939810770824BF01008DF038 /* WebDownload.h in Headers */ = {isa = PBXBuildFile; fileRef = 6578F5DE045F817400000128 /* WebDownload.h */; settings = {ATTRIBUTES = (Private, ); }; };
				939810770824BF01008DF038 /* WebDownload.h in Headers */,

>>> Source/WebKitLegacy/mac/Storage/WebDatabaseQuotaManager.h:27
>>> +#import <objc/NSObject.h>
>> 
>> Why <objc/NSObject.h> instead of <Foundation/NSObject.h>?
> 
> Just because it didn't use anything from Foundation. It's unintuitively worse for build performance to import <Foundation/NSObject.h> than it is to import <Foundation/Foundation.h>, but do you want me to switch all these to <Foundation/Foundation.h>?

Ah, okay, as long as <objc/NSObject.h> is a public header, this seems fine as it aligns with the goal of not including framework headers if not needed.

Thanks!

[David Kilzer (:ddkilzer)]

Comment on attachment 442115 [details]
Patch

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

r- for the following:
- Import <TargetConditions.h> instead of CoreGraphics.h or Foundation.h if that's all that's needed.
- Figure out if WebDownload.h is Public or Private header.  (Xcode says it's Private, which means it could import NSURLDownloadSPI.h.)

> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:-32
> +#import <CoreGraphics/CoreGraphics.h>
> +
>  #if TARGET_OS_IPHONE
>  #import <WebCore/AbstractPasteboard.h>
>  #endif
>  
>  #if TARGET_OS_IOS
>  
> -struct CGSize;

Let's `#import <TargetConditionals.h>` instead of including the whole framework header, and add the `strict CGSize;` statement back.

> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:-32
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
>  #import <CoreGraphics/CoreGraphics.h>
> -#import <Foundation/Foundation.h>

To be explicit about why it's outside the `#if TARGET_OS_IPHONE`, I'd rather `#import <TargetConditionals.h>` here instead of moving the Foundation header.

> Source/WebCore/platform/ios/wak/WAKResponder.h:33
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
> -#import "WKTypes.h"
> -#import <Foundation/Foundation.h>
> +#import <WebCore/WKTypes.h>

Ditto.

> Source/WebCore/platform/ios/wak/WAKView.h:35
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
> -#import "WAKAppKitStubs.h"
> -#import "WAKResponder.h"
> -#import <Foundation/Foundation.h>
>  #import <CoreGraphics/CoreGraphics.h>
> +#import <WebCore/WAKAppKitStubs.h>
> +#import <WebCore/WAKResponder.h>

Ditto.

> Source/WebCore/platform/ios/wak/WAKWindow.h:35
> +#import <Foundation/Foundation.h>
> +
>  #if TARGET_OS_IPHONE
>  
> -#import "WAKAppKitStubs.h"
> -#import "WAKView.h"
> -#import "WKContentObservation.h"
>  #import <CoreGraphics/CoreGraphics.h>
> -#import <Foundation/Foundation.h>
> +#import <WebCore/WAKAppKitStubs.h>
> +#import <WebCore/WAKView.h>
> +#import <WebCore/WKContentObservation.h>

Ditto.

> Source/WebKitLegacy/ios/Misc/WebGeolocationCoreLocationProvider.h:28
> -@class CLLocationManager;
> -@class NSString;
> +#import <Foundation/Foundation.h>

Why is Foundation.h needed here?  (Don't necessarily need to change it—just curious.)

Oh, if it's to inherit from NSObject, then that's fine.  (No reply needed.)

> Source/WebKitLegacy/mac/DOM/WebDOMOperationsPrivate.h:29
> +#import <Foundation/Foundation.h>

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/History/WebHistoryItemPrivate.h:29
> +#import <CoreGraphics/CoreGraphics.h>

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/Misc/NSURLDownloadSPI.h:26
> +#import <Foundation/Foundation.h>

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/Misc/WebCache.h:27
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <Foundation/Foundation.h>

If CoreGraphics.h is just for TargetConditionals.h, lets import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
> -#if (defined TARGET_OS_MACCATALYST && TARGET_OS_MACCATALYST)
> +#import <Foundation/Foundation.h>
> +#import <WebKitLegacy/WebKitAvailability.h>
> +
> +#if defined(TARGET_OS_MACCATALYST) && TARGET_OS_MACCATALYST
>  #import <CFNetwork/CFNSURLConnection.h>
> -#elif !TARGET_OS_IPHONE || (defined USE_APPLE_INTERNAL_SDK && USE_APPLE_INTERNAL_SDK)
> +#elif !TARGET_OS_IPHONE || (defined(USE_APPLE_INTERNAL_SDK) && USE_APPLE_INTERNAL_SDK)
>  #import <Foundation/NSURLDownload.h>
>  #else
>  #import <WebKitLegacy/NSURLDownloadSPI.h>

I commented about this on the older patch, but are we _sure_ that WebDownload.h is a PUBLIC header?  The Xcode project says it's PRIVATE:

$ grep "WebDownload.h in Headers" Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj 
		939810770824BF01008DF038 /* WebDownload.h in Headers */ = {isa = PBXBuildFile; fileRef = 6578F5DE045F817400000128 /* WebDownload.h */; settings = {ATTRIBUTES = (Private, ); }; };
				939810770824BF01008DF038 /* WebDownload.h in Headers */,

If it is really public, it should probably be fixed in the Xcode project, too.

> Source/WebKitLegacy/mac/Plugins/WebPlugin.h:29
> +#import <CoreGraphics/CoreGraphics.h>

If this is just for <TargetConditionals.h>, let's import that directly instead.

> Source/WebKitLegacy/mac/Storage/WebDatabaseManagerPrivate.h:29
> +#import <Foundation/Foundation.h>

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/WebCoreSupport/WebCreateFragmentInternal.h:30
> +#import <Foundation/Foundation.h>

Could probably be replaced with:

@class NSAttributedString;

> Source/WebKitLegacy/mac/WebCoreSupport/WebSecurityOriginPrivate.h:29
> +#import <Foundation/Foundation.h>

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

> Source/WebKitLegacy/mac/WebView/WebResourceLoadDelegatePrivate.h:-35
> +#import <Foundation/Foundation.h>
>  #import <TargetConditionals.h>
>  
>  @class WebView;
>  @class WebDataSource;
> -@class NSURLAuthenticationChallenge;
> -@class NSURLResponse;
> -@class NSURLRequest;

If this is for TargetConditionals.h, let's import TargetConditions.h directly instead.

[Ian Anderson]

Comment on attachment 442115 [details]
Patch

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

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.h:-32
>> -struct CGSize;
> 
> Let's `#import <TargetConditionals.h>` instead of including the whole framework header, and add the `strict CGSize;` statement back.

Discussed in Slack, the compiler was complaining about not including the header in local submodule visibility mode.

>> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:-32
>> -#import <Foundation/Foundation.h>
> 
> To be explicit about why it's outside the `#if TARGET_OS_IPHONE`, I'd rather `#import <TargetConditionals.h>` here instead of moving the Foundation header.

Also discussed in Slack, we decided it's ok like this. (But if you change your mind, I'm good with adding TargetConditionals on top and leaving Foundation where it is.)

>> Source/WebKitLegacy/mac/Misc/WebDownload.h:40
>>  #import <WebKitLegacy/NSURLDownloadSPI.h>
> 
> I commented about this on the older patch, but are we _sure_ that WebDownload.h is a PUBLIC header?  The Xcode project says it's PRIVATE:
> 
> $ grep "WebDownload.h in Headers" Source/WebKitLegacy/WebKitLegacy.xcodeproj/project.pbxproj 
> 		939810770824BF01008DF038 /* WebDownload.h in Headers */ = {isa = PBXBuildFile; fileRef = 6578F5DE045F817400000128 /* WebDownload.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 				939810770824BF01008DF038 /* WebDownload.h in Headers */,
> 
> If it is really public, it should probably be fixed in the Xcode project, too.

It's a public header in WebKit on macOS, and a private header in WebKitLegacy. Very confusing.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebCreateFragmentInternal.h:30
>> +#import <Foundation/Foundation.h>
> 
> Could probably be replaced with:
> 
> @class NSAttributedString;

We could. IMO it's bad form to forward declare interfaces you don't own though. Plus everyone that includes this header is going to have to import Foundation anyway.

[David Kilzer (:ddkilzer)]

Comment on attachment 442115 [details]
Patch

Okay, r+ for the patch.

Setting cq- as a reminder that this needs an internal build first before we can commit it.

[David Kilzer (:ddkilzer)]

Comment on attachment 442115 [details]
Patch

Per internal discussion, this has been sufficiently tested on internal builds, so marking cq+.

[EWS]

Committed r284713 (243428@main): <https://commits.webkit.org/243428@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442115 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 232187

[Ian Anderson]

Created attachment 442230 [details]
Patch

[Ian Anderson]

Created attachment 442233 [details]
Patch

[EWS]

Committed r284736 (243445@main): <https://commits.webkit.org/243445@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442233 [details].