[webkit-reviews] review denied: [Bug 220026] Modularize WebKitLegacy.framework : [Attachment 416549] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 18 16:41:04 PST 2020


David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied 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 416549: Patch

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




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


More information about the webkit-reviews mailing list