Bug 226757 – [Cocoa] Upstream GroupActivitiesCoordinator

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[Cocoa] Upstream GroupActivitiesCoordinator

[Radar WebKit Bug Importer]

<rdar://problem/78984282>

[Jer Noble]

Created attachment 430812 [details]
Patch

[Jer Noble]

Created attachment 430813 [details]
Patch

[Sam Weinig]

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.

[Sam Weinig]

Comment on attachment 430813 [details]
Patch

r-ing due to the concerns around use of a new language.

[Sam Weinig]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 430906 [details]
Patch

[Alex Christensen]

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.

[Jer Noble]

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

[Alex Christensen]

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

[Jer Noble]

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

[Jer Noble]

Created attachment 430973 [details]
Patch

[Sam Weinig]

Comment on attachment 430973 [details]
Patch

What is the plan for testing this?

[Sam Weinig]

Erg, didn't realize there was still a discussion going on about the soft-linking. Sorry.

[Jer Noble]

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

[Eric Carlson]

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

[Jer Noble]

Committed r279133: https://trac.webkit.org/r279133

[Jer Noble]

Follow-up build fix: Committed r279147

[Jer Noble]

Yet another follow-up build fix: Committed r279151