Modularize WebKitLegacy.framework

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Keith Rollin]

Add .modulemap files to WebKitLegacy to help speed up client builds.

[Keith Rollin]

<rdar://57173237>

[Keith Rollin]

Created attachment 416549 [details]
Patch

[Brent Fulgham]

Comment on attachment 416549 [details]
Patch

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

> Source/WebKit/mac/MigrateHeadersFromWebKitLegacy.make:206
> +WEBKIT_LEGACY_PRIVATE_HEADERS = $(addprefix $(PRIVATE_HEADERS_DIR)/, $(filter-out $(WEBKIT_PUBLIC_HEADERS) $(RECENTLY_REMOVED_WEBKIT_LEGACY_PRIVATE_HEADERS) WebKit.h WebKitLegacy_Private.h WebKitLegacy_iOS_Private.h WebKitLegacy_macOS_Private.h, $(notdir $(wildcard $(WEBKIT_LEGACY_PRIVATE_HEADERS_DIR)/*.h))))

Stupid question, but is there a Catalyst version, too? (Probably not?)

[Tim Horton]

Comment on attachment 416549 [details]
Patch

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

> Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:165
> +MODULEMAP_PRIVATE_FILE[sdk=macos*] = $(SRCROOT)/Modules/WebKitLegacy_macOS.private.modulemap;
> +MODULEMAP_PRIVATE_FILE[sdk=iphone*] = $(SRCROOT)/Modules/WebKitLegacy_iOS.private.modulemap;

To Brent's point, it seems highly likely you want to predicate this on WK_IS_COCOA_TOUCH or something intstead, because macCatalyst WebKit is sdk=macos* but builds iOS sources.

[David Kilzer (:ddkilzer)]

Comment on attachment 416549 [details]
Patch

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

Going to mark this as r- because I have too many unanswered questions, and I think we should try to fix the issues instead of excluding the headers.

I might be wrong and it doesn't matter if we exclude these, but I suspect TAPI/InstallAPI is trying to tell us what we need to fix, and we shouldn't ignore them.

> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:15
> +framework module WebKitLegacy_Private {
> +    umbrella header "WebKitLegacy_iOS_Private.h"
> +    exclude header "WebDownload.h" // Includes NSURLDownload.h, which is not modularized.
> +    exclude header "WebKitLegacy_macOS_Private.h" // Used in macOS build
> +
> +    // These files are not included in the module because attempting to do so
> +    // results in build errors. See comments in WebKitLegacy_iOS_Private and
> +    // WebKitLegacy_Private for details.
> +    exclude header "NSURLDownloadSPI.h"
> +    exclude header "WebCreateFragmentInternal.h"
> +    exclude header "WebGeolocationCoreLocationProvider.h"
> +
> +    module * { export * }
> +    export *
> +}

Does excluding these headers negate the benefit of making the module?  I'm not sure what happens if a client uses one of these headers that are outside the module when building.

> Source/WebKitLegacy/Modules/WebKitLegacy_macOS.private.modulemap:14
> +framework module WebKitLegacy_Private {
> +    umbrella header "WebKitLegacy_macOS_Private.h"
> +    exclude header "WebDownload.h" // Includes NSURLDownload.h, which is not modularized.
> +    exclude header "WebKitLegacy_iOS_Private.h" // Used in iOS build
> +
> +    // These files are not included in the module because attempting to do so
> +    // results in build errors. See comments in WebKitLegacy_iOS_Private and
> +    // WebKitLegacy_Private for details.
> +    exclude header "NSURLDownloadSPI.h"
> +    exclude header "WebCreateFragmentInternal.h"
> +
> +    module * { export * }
> +    export *
> +}

Same question above for this macOS module map.

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:171
> +// #import <WebKitLegacy/NSURLDownloadSPI.h> // Skipped in favor of Apple Internal type.

Seems like we should fix this before landing this change.

Can we change this to a Project header (from Private) or delete it from the project altogether?  Are other projects at apple using our NSURLDownloadSPI.h header?

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:180
> +// #import <WebKitLegacy/WebCreateFragmentInternal.h> // Results in "unknown type name 'namespace' error.

In general, Internal headers should not be included in a Private module map, correct?

Do we know why this header is marked Private in Xcode, or whether we can change it to Project?

Seems like we should probably fix this before landing this patch.

Is this telling us that the header is not "Objective-C clean"?

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_iOS_Private.h:51
> +// #import <WebKitLegacy/WebGeolocationCoreLocationProvider.h> // Results in "unknown type name 'namespace' error.

Seems like we should fix this before landing.

There are a lot of WebKitLegacy headers that use "namespace"--are none of them listed here except this one (and WebCreateFragmentInternal.h)?

    $ grep -l -r 'namespace WebCore' Source/WebKitLegacy | egrep '\.h' | grep -v '/win/'

Can we do something like this?

#ifdef __cplusplus
namespace WebCore {
class GeolocationPositionData;
}
using WebCore::GeolocationPositionData;
#else
struct GeolocationPositionData;
#endif

- - (void)positionChanged:(WebCore::GeolocationPositionData&&)position;
+ - (void)positionChanged:(GeolocationPositionData&&)position;

Hmm...not sure Objective-C would like the double '&&' operator, though.

Maybe we need to use a typedef instead?  Not sure what the best analogy for && would be:

#ifdef __cplusplus
namespace WebCore {
class GeolocationPositionData;
}
typedef WebCore::GeolocationPositionData&& WebCoreGeolocationPositionDataReference;
#else
typedef void* WebCoreGeolocationPositionDataReference;
#endif

- - (void)positionChanged:(WebCore::GeolocationPositionData&&)position;
+ - (void)positionChanged:(WebCoreGeolocationPositionDataReference)position;

As before, is this telling us that the header is not "Objective-C clean"?

[Keith Rollin]

(In reply to Tim Horton from comment #4)
> Comment on attachment 416549 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416549&action=review
> 
> > Source/WebKitLegacy/mac/Configurations/WebKitLegacy.xcconfig:165
> > +MODULEMAP_PRIVATE_FILE[sdk=macos*] = $(SRCROOT)/Modules/WebKitLegacy_macOS.private.modulemap;
> > +MODULEMAP_PRIVATE_FILE[sdk=iphone*] = $(SRCROOT)/Modules/WebKitLegacy_iOS.private.modulemap;
> 
> To Brent's point, it seems highly likely you want to predicate this on
> WK_IS_COCOA_TOUCH or something intstead, because macCatalyst WebKit is
> sdk=macos* but builds iOS sources.

I changed it to:

DEFINES_MODULE = YES;
CLANG_MODULES_ENABLE_VERIFIER_TOOL = YES;
MODULEMAP_PRIVATE_FILE = $(MODULEMAP_PRIVATE_FILE_COCOA_TOUCH_$(WK_IS_COCOA_TOUCH));
MODULEMAP_PRIVATE_FILE_COCOA_TOUCH_YES = $(SRCROOT)/Modules/WebKitLegacy_iOS.private.modulemap;
MODULEMAP_PRIVATE_FILE_COCOA_TOUCH_NO = $(SRCROOT)/Modules/WebKitLegacy_macOS.private.modulemap;

[Keith Rollin]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Does excluding these headers negate the benefit of making the module?  I'm
> not sure what happens if a client uses one of these headers that are outside
> the module when building.

Excluding those headers means that the compiler compiles those headers rather than reading any pre-digested module information. UIKit builds without any errors or warnings with these files excluded.

> > Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:171
> > +// #import <WebKitLegacy/NSURLDownloadSPI.h> // Skipped in favor of Apple Internal type.
> 
> Seems like we should fix this before landing this change.
> 
> Can we change this to a Project header (from Private) or delete it from the
> project altogether?  Are other projects at apple using our
> NSURLDownloadSPI.h header?

We can't delete it altogether, since external builds need it. And it's included by WebDownload.h, so, transitively, it seems likely that WebKit1 clients need it.

> > Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:180
> > +// #import <WebKitLegacy/WebCreateFragmentInternal.h> // Results in "unknown type name 'namespace' error.
> 
> In general, Internal headers should not be included in a Private module map,
> correct?

I think that, in general, the rule is that anything in PrivateHeaders should be in the Private module map.

> Do we know why this header is marked Private in Xcode, or whether we can
> change it to Project?

No idea. I'm just taking the project as-is.

> Seems like we should probably fix this before landing this patch.

While this "create WebKitLegacy modulemap" task does seem to shine a light on the set of files WebKitLegacy is exporting, it's not apparent to me that adjusting that set of files is related to this task or should take place before landing this patch. I see it as a separate task, especially since creating modulemaps is low-risk while changing headers or the set of headers is high-risk.

> Is this telling us that the header is not "Objective-C clean"?

I looked into the 'namespace' error some more. The issue was related to our headers being a mix of C++-only and C/C++-clean. Lumping all of those into a single modulemap file without any annotation will cause them to all be treated as C++ files when a [Obj-]C++ file includes any of the headers, and analogously for [Obj-]C files. Thus, when a [Obj-]C files tries to pull in any of its header files, this will indirectly pull in the WebKitLegacy C++ header, leading to the 'namespace' error.

Instead of lumping all the files together and treating them identically, I learned that there's a way of separating the files and marking some as being C++-only. Doing this, I could re-introduce the files I'd excluded. These files will now be ignored when a C file tries to import the module.

[Keith Rollin]

Created attachment 417248 [details]
Patch

[Darin Adler]

Comment on attachment 417248 [details]
Patch

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

To make it easy to keep this working, I think the Private.h files need some documentation at the top about what you should and should not include. People who don’t understand this mechanism will be editing those files and may need help to do this correctly.

I’m wondering where we’d see failures if we get this wrong? Will it be something people outside Apple can see?

> Source/WebKit/ChangeLog:9
> +        Add .modulemap files to WebKitLegacy to help speed up client builds.

Does it help? How much does it speed things up?

> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:4
> +    exclude header "NSURLDownloadSPI.h"             // Conflicts with internal Apple header.

Seems like we could rename this instead.

> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:12
> +        header "WebCreateFragmentInternal.h"

Would like a comment to explain why this is here, I think. There are so many other useful comments in this file, but somehow this one we think goes without saying?

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:171
> +// #import <WebKitLegacy/NSURLDownloadSPI.h> // Skipped in favor of Apple Internal type.

Not sure I understand the word "type" here.

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:180
> +// #import <WebKitLegacy/WebCreateFragmentInternal.h> // Handled in modulemap file due to C++ syntax.

I wish I could understand the precise meaning of the phrase "due to C++ syntax". Is there some problem where modulemap can’t handle C++ at all? Or certain tricky C++ syntax?

> Source/WebKitLegacy/mac/Misc/WebKitLegacy_iOS_Private.h:51
> +// #import <WebKitLegacy/WebGeolocationCoreLocationProvider.h> // Handled in modulemap file due to C++ syntax.

Ditto.

[Darin Adler]

(In reply to Keith Rollin from comment #7)
> > Can we change this to a Project header (from Private) or delete it from the
> > project altogether?  Are other projects at apple using our
> > NSURLDownloadSPI.h header?
> 
> We can't delete it altogether, since external builds need it. And it's
> included by WebDownload.h, so, transitively, it seems likely that WebKit1
> clients need it.

We should not drop this issue forever; someone should follow up. WebKit private header re-exporting SPI from other frameworks is something we want to *not* do.

> > > Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:180
> > > +// #import <WebKitLegacy/WebCreateFragmentInternal.h> // Results in "unknown type name 'namespace' error.
> > 
> > In general, Internal headers should not be included in a Private module map,
> > correct?
> 
> I think that, in general, the rule is that anything in PrivateHeaders should
> be in the Private module map.

Yes, so that header file needs to be renamed if it’s private, not internal.

Dave, I do agree with Keith that this change shouldn’t be delayed until all this is cleaned up, but I agree with you that this should be cleaned up.

[Keith Rollin]

(In reply to Darin Adler from comment #9)
> Comment on attachment 417248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417248&action=review
> 
> To make it easy to keep this working, I think the Private.h files need some
> documentation at the top about what you should and should not include.
> People who don’t understand this mechanism will be editing those files and
> may need help to do this correctly.

OK, I've added comments at the top.

> I’m wondering where we’d see failures if we get this wrong?

It probably depends. There is a build option (which we've enabled) that validates modulemaps, so perhaps that will find issues. I don't know, though, what checks it makes.

It's also possible that there will be build failures in the clients of WebKitLegacy that use modulemaps. Right now, I think that's just UIKit.

The third possibility is that the client builds will succeed, with some definitions coming from the compiled modulemap and some from the headers that weren't included in modulemap definition file.

> Will it be
> something people outside Apple can see?

Only if they enable the use of module maps, and if they build against a locally-built WebKit.

> 
> > Source/WebKit/ChangeLog:9
> > +        Add .modulemap files to WebKitLegacy to help speed up client builds.
> 
> Does it help? How much does it speed things up?

I didn't notice any speedup. The UIKit guys want it anyway, since the modulemap will also help with development of Swift-based projects. (If you were to argue that people shouldn't be writing new Swift code against WebKitLegacy, I'd agree with you.)

> > Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:4
> > +    exclude header "NSURLDownloadSPI.h"             // Conflicts with internal Apple header.
> 
> Seems like we could rename this instead.

The problem isn't with the file name; it's with the file contents. Both it and NSURLDownload.h in the Apple Internal SDK define NSURLDownload and NSURLDownloadDelegate. Any Apple-internal project will include the latter. If the module map were to provide the definition in NSURLDownloadSPI.h, there would be a type conflict, so NSURLDownloadSPI.h is excluded. I've updated the comment to elaborate a bit on this.

> 
> > Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:12
> > +        header "WebCreateFragmentInternal.h"
> 
> Would like a comment to explain why this is here, I think. There are so many
> other useful comments in this file, but somehow this one we think goes
> without saying?

I've added more comments at this location and at the top of the modulemap file.

> 
> > Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:171
> > +// #import <WebKitLegacy/NSURLDownloadSPI.h> // Skipped in favor of Apple Internal type.
> 
> Not sure I understand the word "type" here.

I've elaborated on the comment.

> 
> > Source/WebKitLegacy/mac/Misc/WebKitLegacy_Private.h:180
> > +// #import <WebKitLegacy/WebCreateFragmentInternal.h> // Handled in modulemap file due to C++ syntax.
> 
> I wish I could understand the precise meaning of the phrase "due to C++
> syntax". Is there some problem where modulemap can’t handle C++ at all? Or
> certain tricky C++ syntax?

I've elaborated on the comment.

> 
> > Source/WebKitLegacy/mac/Misc/WebKitLegacy_iOS_Private.h:51
> > +// #import <WebKitLegacy/WebGeolocationCoreLocationProvider.h> // Handled in modulemap file due to C++ syntax.
> 
> Ditto.

Ditto.

[Keith Rollin]

Created attachment 417896 [details]
Patch

[Keith Rollin]

Submitted to the CQ, but there was some internal failure:

<https://ews-build.webkit.org/#/builders/28/builds/8008>

I'll check later to see if it's recovered.

[EWS]

commit-queue failed to commit attachment 417896 [details] to WebKit repository.

[EWS]

commit-queue failed to commit attachment 417896 [details] to WebKit repository.

[EWS]

Committed r271607: <https://trac.webkit.org/changeset/271607>

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