Move some WebKit code to WebKit framework #21058

pvollan · 2023-11-29T19:17:42Z

`c0b5c33`

Move some WebKit code to WebKit framework
https://bugs.webkit.org/show_bug.cgi?id=265324
rdar://118776213

Reviewed by Brent Fulgham.

Move some WebKit code related to WebKit process extensions to WebKit framework. This change enables
us to use new assertion API to take out an assertion on the Networking process when holding locked
files, which is also included in this patch.

* Source/WebKit/NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::setIsHoldingLockedFiles):
* Source/WebKit/NetworkProcess/NetworkProcess.h:
* Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:
(WebKit::NetworkProcess::aqcuireLockedFileGrant):
(WebKit::NetworkProcess::invalidateGrant):
(WebKit::NetworkProcess::hasAcquiredGrant const):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift:
(NetworkingProcessExtension.handle(_:)):
* Source/WebKit/Shared/AuxiliaryProcessExtensions/WebKitProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift.
(WKProcessExtension.sharedInstance):
(WKNetworkingProcessExtension.handle(_:)):
(WKNetworkingProcessExtension.grant(_:name:)):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/271326@main

aaecd27

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-11-29T19:17:52Z

EWS run on previous version of this PR (hash ab980c1)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	⏳ 🛠 wincairo
	~~🛠 ios-sim~~	⏳ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	⏳ 🧪 api-wpe
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	✅ 🛠 gtk
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim	~~🧪 mac-wk2-stress~~
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-29T19:41:00Z

EWS run on previous version of this PR (hash 7ace39f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	⏳ 🧪 api-mac	~~🧪 api-wpe~~
	⏳ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	✅ 🛠 gtk
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	⏳ 🧪 mac-wk2	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 api-gtk
	⏳ 🛠 tv-sim
	⏳ 🛠 watch
	✅ 🛠 watch-sim

emw-apple

I'm not thrilled about adding Swift code into the main WebKit target at this time, for a couple of reasons:

There's a build time cost of turning WebKit into a three-stage compilation process. By adding Swift to the target, we have to, in sequence, compile the module interface; then the swift code; then the rest of the code.
This change is iOS only, so it would modify the build in a way that's not uniform across the Xcode-based platforms.

emw-apple · 2023-11-29T19:36:56Z

Source/WebKit/Configurations/WebKit.xcconfig

+EXCLUDED_EXTENSION_SOURCE_FILE_NAMES = Shared/AuxiliaryProcessExtensions/*;
+EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;
+EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;


I'd recommend inverting this and adding some extra boilerplate to prevent this from being immediately out of date in future iOS versions.

Suggested change

EXCLUDED_EXTENSION_SOURCE_FILE_NAMES = Shared/AuxiliaryProcessExtensions/*;

EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;

EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;

EXCLUDED_EXTENSION_SOURCE_FILE_NAMES = Shared/AuxiliaryProcessExtensions/*;

// already defined further down…

INCLUDED_SOURCE_FILE_NAMES = $(INCLUDED_EXTENSION_SOURCE_FILE_NAMES_$(USE_INTERNAL_SDK));

INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*] = Shared/AuxiliaryProcessExtensions/*;

INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.0.internal] = ;

INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.1.internal] = ;

INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.2.internal] = ;

INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.3.internal] = ;```

Ah, that's a good point. I have updated the patch with this change.

Thanks for reviewing!

Sorry, I didn't mean for you to write "already defined further down…" literally. There is already a INCLUDED_SOURCE_FILE_NAMES definition in the file, so this change needs to be incorporated to it.

Good point, I realized that when I built locally :)

Thanks for reviewing!

emw-apple · 2023-11-29T19:39:24Z

Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm

@@ -54,6 +54,11 @@
 #import <pal/spi/cocoa/NEFilterSourceSPI.h>
 #endif

+#if USE(EXTENSIONKIT) && __has_include(<WebKit-Swift.h>)


Why? It seems like USE(EXTENSIONKIT) implies that we're compiling swift code and generating the Swift header.

emw-apple · 2023-11-29T19:39:37Z

Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm

+#if USE(RUNNINGBOARD) && USE(EXTENSIONKIT)
+bool NetworkProcess::aqcuireLockedFileGrant()
+{
+#if __has_include(<WebKit-Swift.h>)


This was resolved by your request to invert the logic above.

webkit-early-warning-system · 2023-11-29T20:15:44Z

EWS run on previous version of this PR (hash 92cb264)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt		✅ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	⏳ 🧪 mac-wk2	~~🧪 gtk-wk2~~
	⏳ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv-sim
	⏳ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2023-11-29T21:40:58Z

EWS run on previous version of this PR (hash 8e040af)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 gtk
⏳ 🛠 🧪 jsc	✅ 🧪 api-ios	~~🧪 mac-wk2~~	~~🧪 gtk-wk2~~
✅ 🛠 🧪 jsc-arm64	⏳ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	~~🧪 api-gtk~~
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-armv7
	⏳ 🛠 watch		❌ 🧪 jsc-armv7-tests
	✅ 🛠 watch-sim

emw-apple · 2023-11-29T22:07:18Z

Source/WebKit/Configurations/BaseExtension.xcconfig

 SWIFT_OBJC_BRIDGING_HEADER[sdk=iphoneos17.4.internal] = Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h;
+SWIFT_OBJC_BRIDGING_HEADER[sdk=iphonesimulator17.4.internal] = Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h;


I think you need to do a similar dance here: make this setting the default, and then override it to empty on 17.0-17.3.

emw-apple · 2023-11-29T22:11:56Z

Source/WebKit/Configurations/BaseExtension.xcconfig

 EXCLUDED_SOURCE_FILE_NAMES = *;
 EXCLUDED_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;
+EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;


Ditto. Here, it's may be to do something like:

Suggested change

EXCLUDED_SOURCE_FILE_NAMES = *;

EXCLUDED_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;

EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;

EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(USE_INTERNAL_SDK));;

EXCLUDED_SOURCE_FILE_NAMES_ = *;

EXCLUDED_SOURCE_FILE_NAMES_YES = *;

EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*] = ;

EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.0*] = *;

EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.1*] = *;

EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.2*] = *;

EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.3*] = *;

Ah, good catch, will fix!

webkit-early-warning-system · 2023-11-29T22:41:30Z

EWS run on current version of this PR (hash aaecd27)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 gtk
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 api-gtk
	✅ 🛠 tv-sim
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

emw-apple

Per Arne and I talked and we're going to land this as-is, with a plan to remove the Swift code from the WebKit target as a follow-up.

One benefit of this is that it'll help quantify the cost of introducing Swift to the main target.

https://bugs.webkit.org/show_bug.cgi?id=265324 rdar://118776213 Reviewed by Brent Fulgham. Move some WebKit code related to WebKit process extensions to WebKit framework. This change enables us to use new assertion API to take out an assertion on the Networking process when holding locked files, which is also included in this patch. * Source/WebKit/NetworkProcess/NetworkProcess.cpp: (WebKit::NetworkProcess::setIsHoldingLockedFiles): * Source/WebKit/NetworkProcess/NetworkProcess.h: * Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm: (WebKit::NetworkProcess::aqcuireLockedFileGrant): (WebKit::NetworkProcess::invalidateGrant): (WebKit::NetworkProcess::hasAcquiredGrant const): * Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift: (NetworkingProcessExtension.handle(_:)): * Source/WebKit/Shared/AuxiliaryProcessExtensions/WebKitProcessExtension.swift: Copied from Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift. (WKProcessExtension.sharedInstance): (WKNetworkingProcessExtension.handle(_:)): (WKNetworkingProcessExtension.grant(_:name:)): * Source/WebKit/WebKit.xcodeproj/project.pbxproj: Canonical link: https://commits.webkit.org/271326@main

webkit-commit-queue · 2023-11-30T04:48:48Z

Committed 271326@main (c0b5c33): https://commits.webkit.org/271326@main

Reviewed commits have been landed. Closing PR #21058 and removing active labels.

pvollan requested a review from cdumez as a code owner November 29, 2023 19:17

pvollan self-assigned this Nov 29, 2023

pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Nov 29, 2023

pvollan requested review from brentfulgham and emw-apple November 29, 2023 19:18

pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from ab980c1 to 7ace39f Compare November 29, 2023 19:40

emw-apple reviewed Nov 29, 2023

View reviewed changes

pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 7ace39f to 92cb264 Compare November 29, 2023 20:15

pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 92cb264 to 8e040af Compare November 29, 2023 21:40

emw-apple reviewed Nov 29, 2023

View reviewed changes

pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 8e040af to aaecd27 Compare November 29, 2023 22:41

pvollan requested a review from emw-apple November 29, 2023 23:11

emw-apple approved these changes Nov 30, 2023

View reviewed changes

pvollan added the merge-queue Applied to send a pull request to merge-queue label Nov 30, 2023

webkit-commit-queue force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from aaecd27 to c0b5c33 Compare November 30, 2023 04:48

webkit-commit-queue merged commit c0b5c33 into WebKit:main Nov 30, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some WebKit code to WebKit framework #21058

Move some WebKit code to WebKit framework #21058

pvollan commented Nov 29, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited

emw-apple left a comment

emw-apple Nov 29, 2023

pvollan Nov 29, 2023

emw-apple Nov 29, 2023

pvollan Nov 29, 2023

emw-apple Nov 29, 2023

emw-apple Nov 29, 2023

pvollan Nov 29, 2023

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited

emw-apple Nov 29, 2023

pvollan Nov 29, 2023

emw-apple Nov 29, 2023

pvollan Nov 29, 2023

webkit-early-warning-system commented Nov 29, 2023 •

edited

emw-apple left a comment

webkit-commit-queue commented Nov 30, 2023

-EXCLUDED_EXTENSION_SOURCE_FILE_NAMES = Shared/AuxiliaryProcessExtensions/*;
-EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;
-EXCLUDED_EXTENSION_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;
+EXCLUDED_EXTENSION_SOURCE_FILE_NAMES = Shared/AuxiliaryProcessExtensions/*;
+// already defined further down…
+INCLUDED_SOURCE_FILE_NAMES = $(INCLUDED_EXTENSION_SOURCE_FILE_NAMES_$(USE_INTERNAL_SDK));
+INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*] = Shared/AuxiliaryProcessExtensions/*;
+INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.0.internal] = ;
+INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.1.internal] = ;
+INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.2.internal] = ;
+INCLUDED_EXTENSION_SOURCE_FILE_NAMES_YES[sdk=iphone*17.3.internal] = ;```

		SWIFT_OBJC_BRIDGING_HEADER[sdk=iphoneos17.4.internal] = Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h;
		SWIFT_OBJC_BRIDGING_HEADER[sdk=iphonesimulator17.4.internal] = Shared/AuxiliaryProcessExtensions/AuxiliaryProcessExtensionBridge.h;

-EXCLUDED_SOURCE_FILE_NAMES = *;
-EXCLUDED_SOURCE_FILE_NAMES[sdk=iphoneos17.4.internal] = ;
-EXCLUDED_SOURCE_FILE_NAMES[sdk=iphonesimulator17.4.internal] = ;
+EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_$(USE_INTERNAL_SDK));;
+EXCLUDED_SOURCE_FILE_NAMES_ = *;
+EXCLUDED_SOURCE_FILE_NAMES_YES = *;
+EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*] = ;
+EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.0*] = *;
+EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.1*] = *;
+EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.2*] = *;
+EXCLUDED_SOURCE_FILE_NAMES_YES[sdk=iphone*17.3*] = *;

Move some WebKit code to WebKit framework #21058

Move some WebKit code to WebKit framework #21058

Conversation

pvollan commented Nov 29, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 29, 2023 • edited

webkit-early-warning-system commented Nov 29, 2023 • edited

emw-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 29, 2023 • edited

webkit-early-warning-system commented Nov 29, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 29, 2023 • edited

emw-apple left a comment

Choose a reason for hiding this comment

webkit-commit-queue commented Nov 30, 2023

pvollan commented Nov 29, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited

webkit-early-warning-system commented Nov 29, 2023 •

edited