WebKitLegacy's module map files don't get installed
Created attachment 439121 [details] Patch
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.
Created attachment 439122 [details] Patch
rdar://83478496
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 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.
(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 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 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 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.
(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 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 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.
Created attachment 439901 [details] Patch
Comment on attachment 439901 [details] Patch r=me
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].