Bug 230736 - WebKitLegacy's module map files don't get installed
Summary: WebKitLegacy's module map files don't get installed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 220026
Blocks: 230735
  Show dependency treegraph
 
Reported: 2021-09-23 20:52 PDT by Ian Anderson
Modified: 2021-10-01 14:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (39.99 KB, patch)
2021-09-23 20:55 PDT, Ian Anderson
no flags Details | Formatted Diff | Diff
Patch (40.04 KB, patch)
2021-09-23 22:12 PDT, Ian Anderson
no flags Details | Formatted Diff | Diff
Patch (37.50 KB, patch)
2021-10-01 13:16 PDT, Ian Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Anderson 2021-09-23 20:52:27 PDT
WebKitLegacy's module map files don't get installed
Comment 1 Ian Anderson 2021-09-23 20:55:29 PDT
Created attachment 439121 [details]
Patch
Comment 2 Ian Anderson 2021-09-23 21:11:48 PDT
r271607 has too many problems and should just be reverted.

1. Its modules don't actually get installed because it doesn't make a public module map and there isn't a WebKitLegacy.h that Xcode can use to automatically generate one. If there's no public module then the private one won't get installed (and clang wouldn't use it even if it was installed).
2. It tries to make a module for WebKitLegacy on macOS, which won't work because WebKitLegacy is nested in WebKit.
3. It turns on the modules verifier which isn't supported with public Xcode and makes the project fail to build.
4. It has some unrelated changes like WebBackForwardListPrivate.h changing the @param to the wrong name.
5. It adds private umbrella headers to WebKitLegacy which aren’t necessary and have to be maintained by hand.
6. It doesn't fix all of the header errors that would make the module fail.

So I think the best thing to do is revert and start over.
Comment 3 Ian Anderson 2021-09-23 22:12:18 PDT
Created attachment 439122 [details]
Patch
Comment 4 Ian Anderson 2021-09-30 20:23:33 PDT
rdar://83478496
Comment 5 Jonathan Bedard 2021-10-01 08:20:19 PDT
Comment on attachment 439122 [details]
Patch

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

> Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:133
> +#if ENABLE_ORIENTATION_EVENTS

Should be ENABLE(ORIENTATION_EVENTS)

> Source/WebKitLegacy/mac/WebView/WebHTMLViewPrivate.h:116
> +#if ENABLE_NETSCAPE_PLUGIN_API

Should be ENABLE(NETSCAPE_PLUGIN_API)
Comment 6 David Kilzer (:ddkilzer) 2021-10-01 12:25:53 PDT
Comment on attachment 439122 [details]
Patch

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

r-, but just don't revert the two `#if defined(...) & ...` changes.

>> Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:133
>> +#if ENABLE_ORIENTATION_EVENTS
> 
> Should be ENABLE(ORIENTATION_EVENTS)

Actually, we can't use ENABLE(ORIENTATION_EVENTS) here because this is a public or private header, and we can't assume the ENABLE() macro is defined here when this is included outside of a WebKit build.

We should NOT revert this change, or the other one like it below.

>> Source/WebKitLegacy/mac/WebView/WebHTMLViewPrivate.h:116
>> +#if ENABLE_NETSCAPE_PLUGIN_API
> 
> Should be ENABLE(NETSCAPE_PLUGIN_API)

Actually, we can't use ENABLE(NETSCAPE_PLUGIN_API) here because this is a public or private header, and we can't assume the ENABLE() macro is defined here when this is included outside of a WebKit build.

We should NOT revert this change, or the other one like it above.
Comment 7 David Kilzer (:ddkilzer) 2021-10-01 12:26:20 PDT
(In reply to David Kilzer (:ddkilzer) from comment #6)
> Comment on attachment 439122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439122&action=review
> 
> r-, but just don't revert the two `#if defined(...) & ...` changes.

I'll give an r+ after making that change.
Comment 8 David Kilzer (:ddkilzer) 2021-10-01 12:32:29 PDT
Comment on attachment 439122 [details]
Patch

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

> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:-8
> -    umbrella header "WebKitLegacy_iOS_Private.h"

BTW, why are umbrella headers the wrong approach here?

> Source/WebKitLegacy/mac/History/WebBackForwardListPrivate.h:37
> -    @param item The entry to remove.  Cannot be the current item.
> +    @param entry The entry to remove.  Cannot be the current item.

This change should be kept as well.
Comment 9 David Kilzer (:ddkilzer) 2021-10-01 12:34:42 PDT
Comment on attachment 439122 [details]
Patch

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

>> Source/WebKitLegacy/mac/History/WebBackForwardListPrivate.h:37
>> +    @param entry The entry to remove.  Cannot be the current item.
> 
> This change should be kept as well.

Oops, you're right, this should be reverted.
Comment 10 David Kilzer (:ddkilzer) 2021-10-01 12:35:27 PDT
Comment on attachment 439122 [details]
Patch

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

>> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:-8
>> -    umbrella header "WebKitLegacy_iOS_Private.h"
> 
> BTW, why are umbrella headers the wrong approach here?

Ah, answered here in Comment #0:

    5. It adds private umbrella headers to WebKitLegacy which aren’t necessary and have to be maintained by hand.
Comment 11 David Kilzer (:ddkilzer) 2021-10-01 12:36:35 PDT
(In reply to David Kilzer (:ddkilzer) from comment #10)
> Comment on attachment 439122 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439122&action=review
> 
> >> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:-8
> >> -    umbrella header "WebKitLegacy_iOS_Private.h"
> > 
> > BTW, why are umbrella headers the wrong approach here?
> 
> Ah, answered here in Comment #0:
> 
>     5. It adds private umbrella headers to WebKitLegacy which aren’t
> necessary and have to be maintained by hand.

Oops, that's Comment #2.

But don't revert the "#if defined(ENABLE_...) && ENABLE..." changes, and we should be good to go.
Comment 12 Ian Anderson 2021-10-01 12:55:16 PDT
Comment on attachment 439122 [details]
Patch

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

>>>> Source/WebKitLegacy/Modules/WebKitLegacy_iOS.private.modulemap:-8
>>>> -    umbrella header "WebKitLegacy_iOS_Private.h"
>>> 
>>> BTW, why are umbrella headers the wrong approach here?
>> 
>> Ah, answered here in Comment #0:
>> 
>>     5. It adds private umbrella headers to WebKitLegacy which aren’t necessary and have to be maintained by hand.
> 
> Oops, that's Comment #2.
> 
> But don't revert the "#if defined(ENABLE_...) && ENABLE..." changes, and we should be good to go.

They're not the "wrong" approach exactly, but if you don't want them outside of modules then I don't think you want them for modules either.

>>> Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:133
>>> +#if ENABLE_ORIENTATION_EVENTS
>> 
>> Should be ENABLE(ORIENTATION_EVENTS)
> 
> Actually, we can't use ENABLE(ORIENTATION_EVENTS) here because this is a public or private header, and we can't assume the ENABLE() macro is defined here when this is included outside of a WebKit build.
> 
> We should NOT revert this change, or the other one like it below.

This change is a pure revert, should it have bonus fixes like that?

>>> Source/WebKitLegacy/mac/WebView/WebHTMLViewPrivate.h:116
>>> +#if ENABLE_NETSCAPE_PLUGIN_API
>> 
>> Should be ENABLE(NETSCAPE_PLUGIN_API)
> 
> Actually, we can't use ENABLE(NETSCAPE_PLUGIN_API) here because this is a public or private header, and we can't assume the ENABLE() macro is defined here when this is included outside of a WebKit build.
> 
> We should NOT revert this change, or the other one like it above.

(same here)
Comment 13 Ian Anderson 2021-10-01 12:56:45 PDT
Comment on attachment 439122 [details]
Patch

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

>>>> Source/WebKitLegacy/ios/WebView/WebUIKitDelegate.h:133
>>>> +#if ENABLE_ORIENTATION_EVENTS
>>> 
>>> Should be ENABLE(ORIENTATION_EVENTS)
>> 
>> Actually, we can't use ENABLE(ORIENTATION_EVENTS) here because this is a public or private header, and we can't assume the ENABLE() macro is defined here when this is included outside of a WebKit build.
>> 
>> We should NOT revert this change, or the other one like it below.
> 
> This change is a pure revert, should it have bonus fixes like that?

Ah I see, I'll un-revert those then.
Comment 14 Ian Anderson 2021-10-01 13:16:31 PDT
Created attachment 439901 [details]
Patch
Comment 15 David Kilzer (:ddkilzer) 2021-10-01 14:14:09 PDT
Comment on attachment 439901 [details]
Patch

r=me
Comment 16 EWS 2021-10-01 14:28:18 PDT
Committed r283403 (242406@main): <https://commits.webkit.org/242406@main>

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