Bug 230736 – WebKitLegacy's module map files don't get installed

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Ian Anderson]

WebKitLegacy's module map files don't get installed

[Ian Anderson]

Created attachment 439121 [details]
Patch

[Ian Anderson]

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.

[Ian Anderson]

Created attachment 439122 [details]
Patch

[Ian Anderson]

rdar://83478496

[Jonathan Bedard]

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)

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

(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.

[Ian Anderson]

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)

[Ian Anderson]

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.

[Ian Anderson]

Created attachment 439901 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 439901 [details]
Patch

r=me

[EWS]

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].