[Webkit-unassigned] [Bug 230735] Add a module map file for PrivateFrameworks/WebKitLegacy

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=230735

--- Comment #21 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 441851
  --> https://bugs.webkit.org/attachment.cgi?id=441851
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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211022/a2bfef25/attachment.htm>