[Cocoa] Upstream GroupActivitiesCoordinator
<rdar://problem/78984282>
Created attachment 430812 [details] Patch
Created attachment 430813 [details] Patch
Comment on attachment 430813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430813&action=review > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSession.mm:27 > +#import "config.h" > +#import "GroupSession.h" Extra spaces. > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSession.mm:35 > +Ref<GroupSession> GroupSession::create(RetainPtr<WKGroupSession>&& session) These names seem too generic. I think we should make these quite a bit specific to the use case. > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSessionNotifier.mm:58 > +static void* WebKitSwiftLibrary() > +{ > + static void* dylib = [] { > + // Start by searching for the library in DYLD_LIBRARY_PATH: > + if (void *result = dlopen("libWebKitSwift.dylib", RTLD_NOW)) > + return result; > + > + // Then search in the Frameworks/ directory of the currently loaded version of WebKit.framework: > + Dl_info info { }; > + if (dladdr((const void*)&WebKitSwiftLibrary, &info) && strlen(info.dli_fname)) { > + auto webkitFrameworkDirectory = WTF::FileSystemImpl::parentPath(info.dli_fname); > + auto dylibPath = WTF::FileSystemImpl::pathByAppendingComponent(webkitFrameworkDirectory, "Frameworks/libWebKitSwift.dylib"); > + if (void* result = dlopen(dylibPath.utf8().data(), RTLD_NOW)) > + return result; > + } > + > + // Finally, search at the installed system path: > + if (void *result = dlopen("/System/Library/Frameworks/WebKit.framework/Frameworks/libWebKitSwift.dylib", RTLD_NOW)) > + return result; > + > + return (void *)nullptr; > + }(); > + return dylib; > +} Given this is a generic linking of libWebKitSwift.dylib, and could be used for other things in the future, this should probably go in its own .h/.cpp file. > Source/WebKit/UIProcess/Cocoa/GroupActivities/WKGroupSession.swift:30 > +import AVFoundation > +import Combine > +@_spi(Safari) import GroupActivities > +import Foundation > + We don't allow Swift in the WebKit project. This probably needs to be discussed on webkit-dev to determine if it makes sense to allow this addition.
Comment on attachment 430813 [details] Patch r-ing due to the concerns around use of a new language.
Comment on attachment 430813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430813&action=review >> Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSession.mm:35 >> +Ref<GroupSession> GroupSession::create(RetainPtr<WKGroupSession>&& session) > > These names seem too generic. I think we should make these quite a bit specific to the use case. Jer and I discussed this and think that using the prefix GroupActivity or maybe GroupActivities for these classes will help quite a bit. So this will become GroupActivitySession or GroupActivitiesSession.
(In reply to Sam Weinig from comment #4) > Comment on attachment 430813 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430813&action=review > > > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSession.mm:27 > > +#import "config.h" > > +#import "GroupSession.h" > > Extra spaces. Deleted. > > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSession.mm:35 > > +Ref<GroupSession> GroupSession::create(RetainPtr<WKGroupSession>&& session) > > These names seem too generic. I think we should make these quite a bit > specific to the use case. I made all the classes in this area match, with a prefix of GroupActivities, which is the name of the underlying framework. > > Source/WebKit/UIProcess/Cocoa/GroupActivities/GroupSessionNotifier.mm:58 > > +static void* WebKitSwiftLibrary() > > +{ > > + static void* dylib = [] { > > + // Start by searching for the library in DYLD_LIBRARY_PATH: > > + if (void *result = dlopen("libWebKitSwift.dylib", RTLD_NOW)) > > + return result; > > + > > + // Then search in the Frameworks/ directory of the currently loaded version of WebKit.framework: > > + Dl_info info { }; > > + if (dladdr((const void*)&WebKitSwiftLibrary, &info) && strlen(info.dli_fname)) { > > + auto webkitFrameworkDirectory = WTF::FileSystemImpl::parentPath(info.dli_fname); > > + auto dylibPath = WTF::FileSystemImpl::pathByAppendingComponent(webkitFrameworkDirectory, "Frameworks/libWebKitSwift.dylib"); > > + if (void* result = dlopen(dylibPath.utf8().data(), RTLD_NOW)) > > + return result; > > + } > > + > > + // Finally, search at the installed system path: > > + if (void *result = dlopen("/System/Library/Frameworks/WebKit.framework/Frameworks/libWebKitSwift.dylib", RTLD_NOW)) > > + return result; > > + > > + return (void *)nullptr; > > + }(); > > + return dylib; > > +} > > Given this is a generic linking of libWebKitSwift.dylib, and could be used > for other things in the future, this should probably go in its own .h/.cpp > file. Moved this into WebKitSwiftSoftLink.h/mm > > Source/WebKit/UIProcess/Cocoa/GroupActivities/WKGroupSession.swift:30 > > +import AVFoundation > > +import Combine > > +@_spi(Safari) import GroupActivities > > +import Foundation > > + > > We don't allow Swift in the WebKit project. This probably needs to be > discussed on webkit-dev to determine if it makes sense to allow this > addition. I'll start a thread on webkit-dev with the caveat that this is merely bridging code for a Swift-API.
Created attachment 430906 [details] Patch
Comment on attachment 430906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430906&action=review > Source/WebKit/UIProcess/Cocoa/WebKitSwiftSoftLink.mm:51 > + if ((library = dlopen("/System/Library/Frameworks/WebKit.framework/Frameworks/libWebKitSwift.dylib", RTLD_NOW))) Could we not hard-code this path into the binary? I'm imagining lots of problems with INSTALL_PATH being different in places like our catalyst build.
(In reply to Alex Christensen from comment #9) > Comment on attachment 430906 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=430906&action=review > > > Source/WebKit/UIProcess/Cocoa/WebKitSwiftSoftLink.mm:51 > > + if ((library = dlopen("/System/Library/Frameworks/WebKit.framework/Frameworks/libWebKitSwift.dylib", RTLD_NOW))) > > Could we not hard-code this path into the binary? I'm imagining lots of > problems with INSTALL_PATH being different in places like our catalyst build. The Catalyst and Staging paths should be covered by the "look in the Frameworks/ directory of the current-loaded WebKit binary" case above. But because we have to soft-link this dylib rather than strong- or weak-link, the mechanisms we would usually use for locating dylibs won't work here. I'm open to other ideas however.
Comment on attachment 430906 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430906&action=review >>> Source/WebKit/UIProcess/Cocoa/WebKitSwiftSoftLink.mm:51 >>> + if ((library = dlopen("/System/Library/Frameworks/WebKit.framework/Frameworks/libWebKitSwift.dylib", RTLD_NOW))) >> >> Could we not hard-code this path into the binary? I'm imagining lots of problems with INSTALL_PATH being different in places like our catalyst build. > > The Catalyst and Staging paths should be covered by the "look in the Frameworks/ directory of the current-loaded WebKit binary" case above. But because we have to soft-link this dylib rather than strong- or weak-link, the mechanisms we would usually use for locating dylibs won't work here. I'm open to other ideas however. Would it work if we just removed this and only searched in the two previous locations? I'm just worried about us loading the wrong version in Catalyst builds or in Safari Tech Preview
(In reply to Alex Christensen from comment #11) > Would it work if we just removed this and only searched in the two previous > locations? I'm just worried about us loading the wrong version in Catalyst > builds or in Safari Tech Preview I think the fallback could be safely removed; this would mean that certain features would fail to work if the .dylib was missing, but the entire point of this .dylib is to handle that scenario, for things like BaseOS installs. I'll remove it.
Created attachment 430973 [details] Patch
Comment on attachment 430973 [details] Patch What is the plan for testing this?
Erg, didn't realize there was still a discussion going on about the soft-linking. Sorry.
(In reply to Sam Weinig from comment #15) > Erg, didn't realize there was still a discussion going on about the > soft-linking. Sorry. No, I took Alex's suggestion and implemented it. The current patch reflects that. (In reply to Sam Weinig from comment #14) > Comment on attachment 430973 [details] > Patch > > What is the plan for testing this? Manual testing only, as it involves a separate device and a FaceTime connection to be functional. We have mock versions of the coordinator's private objects and layout tests based on those mock objects.
(In reply to Jer Noble from comment #16) > > (In reply to Sam Weinig from comment #14) > > Comment on attachment 430973 [details] > > Patch > > > > What is the plan for testing this? > > Manual testing only, as it involves a separate device and a FaceTime > connection to be functional. We have mock versions of the coordinator's > private objects and layout tests based on those mock objects. And a fairly extensive API test.
Committed r279133: https://trac.webkit.org/r279133
Follow-up build fix: Committed r279147
Yet another follow-up build fix: Committed r279151