[webkit-reviews] review granted: [Bug 220026] Modularize WebKitLegacy.framework : [Attachment 417248] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 8 11:02:11 PST 2021


Darin Adler <darin at apple.com> has granted Keith Rollin <krollin at apple.com>'s
request for review:
Bug 220026: Modularize WebKitLegacy.framework
https://bugs.webkit.org/show_bug.cgi?id=220026

Attachment 417248: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 417248
  --> https://bugs.webkit.org/attachment.cgi?id=417248
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.


More information about the webkit-reviews mailing list