| Summary: | Add a module map file for PrivateFrameworks/WebKitLegacy | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ian Anderson <iana> | ||||||||||||||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | ap, bfulgham, commit-queue, ddkilzer, jbedard, mitz, thorton, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | iOS 14 | ||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=220026 | ||||||||||||||||||||
| Bug Depends on: | 230736, 232187 | ||||||||||||||||||||
| Bug Blocks: | 232190 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Ian Anderson
2021-09-23 20:09:14 PDT
Created attachment 439814 [details]
Patch
(In reply to Radar WebKit Bug Importer from comment #1) > <rdar://problem/83750014> Duped to rdar://16833552 Comment on attachment 439814 [details] Patch r=me once the fix for Bug 230736 lands. Comment on attachment 439814 [details]
Patch
Marking this cq- for now as it needs additional side builds before landing.
Comment on attachment 439814 [details]
Patch
Changing to r- until the macOS build issues are resolved.
Created attachment 439918 [details]
Patch
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) 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. 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? (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! Created attachment 439949 [details]
Patch
Comment on attachment 439949 [details]
Patch
r=me but cq- until side builds are completed.
Created attachment 441851 [details]
Patch
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. 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>? Created attachment 442106 [details]
Patch
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...) Created attachment 442115 [details]
Patch
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! 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. 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. 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.
Comment on attachment 442115 [details]
Patch
Per internal discussion, this has been sufficiently tested on internal builds, so marking cq+.
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]. Re-opened since this is blocked by bug 232187 Created attachment 442230 [details]
Patch
Created attachment 442233 [details]
Patch
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]. |