Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move some WebKit code to WebKit framework #21140

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Dec 1, 2023

676f48d

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/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Configurations/WebKit.xcconfig:
* Source/WebKit/Modules/iOS_Private.modulemap:
* 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/AuxiliaryProcessExtensionBridge.h:
* Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift:
(Grant.invalidate):
(NetworkingProcessExtension.grant(_:name:)):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

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

7fc0483

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

@pvollan pvollan requested a review from cdumez as a code owner December 1, 2023 03:54
@pvollan pvollan self-assigned this Dec 1, 2023
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Dec 1, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 409ec4b to 11531db Compare December 1, 2023 05:15
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 11531db to f51acb3 Compare December 1, 2023 14:44
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me, but I'm less confident about the hard-coded iPhone OS release numbers. I'm approving, but would appreciate it if you could check with @emw-apple before landing.

SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.0*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.1*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.2*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.3*] = ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern of hard-coding specific iOS releases seems unusual. I'd prefer for @emw-apple to review these bits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my request, actually. We need to enable this functionality for all builds after iOS 17.3. The more normal thing to do is to use build setting macros from WebKitTargetConditionals.xcconfig, but we generally only define those for major OS releases.

@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 42987e4 to 183187d Compare December 1, 2023 20:18
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Dec 1, 2023
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 183187d to 70e275a Compare December 1, 2023 20:46
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 70e275a to e2df860 Compare December 1, 2023 20:51
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from e2df860 to 876f446 Compare December 1, 2023 21:02
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 876f446 to 49019d9 Compare December 1, 2023 21:29
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 49019d9 to b7d9f6d Compare December 1, 2023 21:58
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from b7d9f6d to 7421567 Compare December 1, 2023 22:04
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 7421567 to e376dd9 Compare December 1, 2023 22:15
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.0*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.1*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.2*] = ;
SWIFT_OBJC_BRIDGING_HEADER_YES[sdk=iphone*17.3*] = ;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my request, actually. We need to enable this functionality for all builds after iOS 17.3. The more normal thing to do is to use build setting macros from WebKitTargetConditionals.xcconfig, but we generally only define those for major OS releases.


- (void)setSharedInstance:(WKProcessExtension*)instance
{
NSLog(@"WKProcessExtension setSharedInstance %@", instance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This and the other NSLog calls in this file feel like debugging statements to me, that probably shouldn't make it to production and will only cause log spam. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Thanks for reviewing!

Comment on lines +64 to +71
override func grant(_ domain: String, name: String) -> Any {
do {
let grant = try self._request(capabilities: _Capabilities.assertion(domain, name))
return Grant(inner: grant)
} catch {
return WKGrant()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having an abstract -[WKProcessExtension grant:name:] method that you override from Swift, you could declare this extension method as @objc. Then, from NetworkProcessCocoa.mm, you could do a respondsToSelector(@selector(grant:name:)) to check that the method exists (assertion-failing if it does not), then call it.

That might be easier to follow than the abstract method, which should never be called, but has to be written nonetheless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, although I think it also would be good to avoid the respondsToSelector call. I went ahead and landed as-is, but please let me know if you still think this should be changed, and I can follow up in a new patch :)

@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from e376dd9 to c45ea31 Compare December 3, 2023 20:32
@pvollan pvollan force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from c45ea31 to 7fc0483 Compare December 4, 2023 14:08
@pvollan pvollan added the merge-queue Applied to send a pull request to merge-queue label Dec 4, 2023
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/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Configurations/WebKit.xcconfig:
* Source/WebKit/Modules/iOS_Private.modulemap:
* 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/AuxiliaryProcessExtensionBridge.h:
* Source/WebKit/Shared/AuxiliaryProcessExtensions/NetworkingProcessExtension.swift:
(Grant.invalidate):
(NetworkingProcessExtension.grant(_:name:)):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/271478@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Move-some-WebKit-code-to-WebKit-framework branch from 7fc0483 to 676f48d Compare December 4, 2023 16:04
@webkit-commit-queue webkit-commit-queue merged commit 676f48d into WebKit:main Dec 4, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 271478@main (676f48d): https://commits.webkit.org/271478@main

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

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
6 participants