Bug 210451 – Enable service workers for app-bound domains

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should allow the use of service workers for app-bound domain navigations

[Kate Cheney]

<rdar://problem/61479474>

[Kate Cheney]

Created attachment 396591 [details]
Patch

[Kate Cheney]

Created attachment 396657 [details]
Patch

[youenn fablet]

Comment on attachment 396657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396657&action=review

> Source/WebKit/ChangeLog:10
> +        Service workers should be disabled otherwise.

The goal as I understand it is to allow an app to register service workers from a specific domain.
This could be handled at two point in time:
- when importing service workers. Can we pass that information straight to the network process as part of website data store parameters? If so, the importing of service workers would filter out non app-bound registrations
- when registering a service worker in web process. Registering a service worker from a document not app-bound would be rejected.

It might also be good to only expose the service worker API for apps with entitlements or app bound apps.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:431
> +    auto allowServiceWorkersForAppBoundDomain = loader.parameters().isNavigatingToAppBoundDomain && *loader.parameters().isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes;

s/auto/bool

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1052
> +WebSWServerConnection& NetworkConnectionToWebProcess::swConnection(bool allowServiceWorkersForAppBoundDomain)

We are passing this boolean only for ASSERTs so I am wondering whether this is actually worth it.
Maybe we could just remove the ASSERTs

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:169
> +    WebSWServerConnection& swConnection(bool);

This bool is mysterious.
I would remove it if possible and use otherwise an enum class bool.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:446
> +        addServiceWorkerSession(sessionID);

We might add more than one website data store so this needs to be a map instead of direct NetworkProcess member fields.
Why cannot we reuse m_serviceWorkerInfo?
addServiceWorkerSession is not doing a lot of things except registering in a map and creating a folder if needed.
Maybe we could just remove the if (parentProcessHasServiceWorkerEntitlement()) check.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1554
> +    if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::ServiceWorkerRegistrations) && !sessionID.isEphemeral())

It seems we should do the query to m_serviceWorkerInfo at the end of the if check.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:-267
> -    if (auto* registration = m_loader.connectionToWebProcess().swConnection().server().getRegistration(m_serviceWorkerRegistrationIdentifier))

If we have a registration and are doing a service worker fetch task, we are actually allowed to use service worker.
No need to recompute it here.

[Kate Cheney]

Comment on attachment 396657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396657&action=review

>> Source/WebKit/ChangeLog:10
>> +        Service workers should be disabled otherwise.
> 
> The goal as I understand it is to allow an app to register service workers from a specific domain.
> This could be handled at two point in time:
> - when importing service workers. Can we pass that information straight to the network process as part of website data store parameters? If so, the importing of service workers would filter out non app-bound registrations
> - when registering a service worker in web process. Registering a service worker from a document not app-bound would be rejected.
> 
> It might also be good to only expose the service worker API for apps with entitlements or app bound apps.

Yes, this is correct. This patch checks in both the web process and network process for whether the domain is app-bound. Maybe this isn't necessary, and I'm wondering if we can remove the entitlement check and app-bound domain check from either the network process or the web process completely. Not sure which is better.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:431
>> +    auto allowServiceWorkersForAppBoundDomain = loader.parameters().isNavigatingToAppBoundDomain && *loader.parameters().isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes;
> 
> s/auto/bool

will change.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:1052
>> +WebSWServerConnection& NetworkConnectionToWebProcess::swConnection(bool allowServiceWorkersForAppBoundDomain)
> 
> We are passing this boolean only for ASSERTs so I am wondering whether this is actually worth it.
> Maybe we could just remove the ASSERTs

Ok. I wasn't sure if they were added for some debugging reason.

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:169
>> +    WebSWServerConnection& swConnection(bool);
> 
> This bool is mysterious.
> I would remove it if possible and use otherwise an enum class bool.

Will do, or the parameter is maybe not even needed if we remove the asserts.

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:446
>> +        addServiceWorkerSession(sessionID);
> 
> We might add more than one website data store so this needs to be a map instead of direct NetworkProcess member fields.
> Why cannot we reuse m_serviceWorkerInfo?
> addServiceWorkerSession is not doing a lot of things except registering in a map and creating a folder if needed.
> Maybe we could just remove the if (parentProcessHasServiceWorkerEntitlement()) check.

Ok. So to clarify, we can add a service worker session regardless of whether the entitlement is present?

Do we need to check parentProcessHasServiceWorkerEntitlement() in the network process at all if we check it in the web process (from your first comment I am interpreting that if we restrict it in the web process we don't need to also check for the entitlement in the network process).

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1554
>> +    if (!path.isEmpty() && websiteDataTypes.contains(WebsiteDataType::ServiceWorkerRegistrations) && !sessionID.isEphemeral())
> 
> It seems we should do the query to m_serviceWorkerInfo at the end of the if check.

Ok, why is that better than the beginning?

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:-267
>> -    if (auto* registration = m_loader.connectionToWebProcess().swConnection().server().getRegistration(m_serviceWorkerRegistrationIdentifier))
> 
> If we have a registration and are doing a service worker fetch task, we are actually allowed to use service worker.
> No need to recompute it here.

Ok. So this can be if (auto* registration = m_loader.connectionToWebProcess().swConnection(true).server().getRegistration(m_serviceWorkerRegistrationIdentifier)) ? Again, maybe not needed if we remove the ASSERTs.

[youenn fablet]

(In reply to katherine_cheney from comment #5)
> Comment on attachment 396657 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396657&action=review
> 
> >> Source/WebKit/ChangeLog:10
> >> +        Service workers should be disabled otherwise.
> > 
> > The goal as I understand it is to allow an app to register service workers from a specific domain.
> > This could be handled at two point in time:
> > - when importing service workers. Can we pass that information straight to the network process as part of website data store parameters? If so, the importing of service workers would filter out non app-bound registrations
> > - when registering a service worker in web process. Registering a service worker from a document not app-bound would be rejected.
> > 
> > It might also be good to only expose the service worker API for apps with entitlements or app bound apps.
> 
> Yes, this is correct. This patch checks in both the web process and network
> process for whether the domain is app-bound. Maybe this isn't necessary, and
> I'm wondering if we can remove the entitlement check and app-bound domain
> check from either the network process or the web process completely. Not
> sure which is better.

Ideally, we should be able to keep everything in UIProcess and NetworkProcess, UIProcess to set the runtime flag and NetworkProcess to filter registrations.
- If no entitlement, no service worker API exposed, no service worker imported, ServiceWorker mode should be none for all loads
- If app bound, network process gets the list of allowed service worker registration domains and will reject any imported registration/new registration that is not in the list
- If entitlement, all domains are allowed.

If we can pass the list of allowed domains to NetworkProcess/SWServer, we could do the checks in SWServer::scheduleJob and SWServer:: addRegistrationFromStore.

[Kate Cheney]

(In reply to youenn fablet from comment #6)
> (In reply to katherine_cheney from comment #5)
> > Comment on attachment 396657 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396657&action=review
> > 
> > >> Source/WebKit/ChangeLog:10
> > >> +        Service workers should be disabled otherwise.
> > > 
> > > The goal as I understand it is to allow an app to register service workers from a specific domain.
> > > This could be handled at two point in time:
> > > - when importing service workers. Can we pass that information straight to the network process as part of website data store parameters? If so, the importing of service workers would filter out non app-bound registrations
> > > - when registering a service worker in web process. Registering a service worker from a document not app-bound would be rejected.
> > > 
> > > It might also be good to only expose the service worker API for apps with entitlements or app bound apps.
> > 
> > Yes, this is correct. This patch checks in both the web process and network
> > process for whether the domain is app-bound. Maybe this isn't necessary, and
> > I'm wondering if we can remove the entitlement check and app-bound domain
> > check from either the network process or the web process completely. Not
> > sure which is better.
> 
> Ideally, we should be able to keep everything in UIProcess and
> NetworkProcess, UIProcess to set the runtime flag and NetworkProcess to
> filter registrations.
> - If no entitlement, no service worker API exposed, no service worker
> imported, ServiceWorker mode should be none for all loads

If there is no entitlement in the UIProcess, don't we also need to check for an app-bound domain to expose SWs for them? Or is this not needed?

> - If app bound, network process gets the list of allowed service worker
> registration domains and will reject any imported registration/new
> registration that is not in the list
> - If entitlement, all domains are allowed.
> 
> If we can pass the list of allowed domains to NetworkProcess/SWServer, we
> could do the checks in SWServer::scheduleJob and SWServer::
> addRegistrationFromStore.

OK. I'll look into this. My only concern is this requires IPC from the UIProcess to get these domains. Is this simpler than checking for an app-bound domain in NetworkConnectionToWebProcess::createFetchTask(), where the information already exists to determine whether it is app bound?

[youenn fablet]

(In reply to katherine_cheney from comment #7)
> (In reply to youenn fablet from comment #6)
> > (In reply to katherine_cheney from comment #5)
> > > Comment on attachment 396657 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=396657&action=review
> > > 
> > > >> Source/WebKit/ChangeLog:10
> > > >> +        Service workers should be disabled otherwise.
> > > > 
> > > > The goal as I understand it is to allow an app to register service workers from a specific domain.
> > > > This could be handled at two point in time:
> > > > - when importing service workers. Can we pass that information straight to the network process as part of website data store parameters? If so, the importing of service workers would filter out non app-bound registrations
> > > > - when registering a service worker in web process. Registering a service worker from a document not app-bound would be rejected.
> > > > 
> > > > It might also be good to only expose the service worker API for apps with entitlements or app bound apps.
> > > 
> > > Yes, this is correct. This patch checks in both the web process and network
> > > process for whether the domain is app-bound. Maybe this isn't necessary, and
> > > I'm wondering if we can remove the entitlement check and app-bound domain
> > > check from either the network process or the web process completely. Not
> > > sure which is better.
> > 
> > Ideally, we should be able to keep everything in UIProcess and
> > NetworkProcess, UIProcess to set the runtime flag and NetworkProcess to
> > filter registrations.
> > - If no entitlement, no service worker API exposed, no service worker
> > imported, ServiceWorker mode should be none for all loads
> 
> If there is no entitlement in the UIProcess, don't we also need to check for
> an app-bound domain to expose SWs for them? Or is this not needed?

I think it is simpler to expose service worker API to all pages rendered by an app-bound page. Bur registration will fail for some domains, maybe as if quota for those domains was zero.

[Kate Cheney]

Created attachment 396701 [details]
Patch

[Kate Cheney]

Created attachment 396704 [details]
Patch

[Kate Cheney]

Ok, this patch is much simpler after making some of your suggestions.

I am not sure I fully understand what you had in mind, but this patch enables the service worker runtime flag in the UI Process if the entitlement is present or the navigation is app-bound. It disables it if the entitlement is not present and the navigation is not app-bound.

In the Network process, it checks for an app-bound domain in NetworkConnectionToWebProcess (where the information is already available) and does not allow a service worker load if the entitlement is not present and the domain is not app-bound. I thought this was simpler than trying to send the appBoundDomains list over from the UI Process and storing it in the Network Process when the information was already there on a per-load basis.

[youenn fablet]

Comment on attachment 396704 [details]
Patch

This looks indeed better.

If we can, I would like to see whether we can pass to NetworkProcess, as part of WebsiteDataStore parameters, the app bound domain information.
SWServer would then be taught about the domains for which service worker registrations would be allowed (SWServer::scheduleJob and SWServer::addRegistrationFromStore).

The additional benefit is that we would no longer need the network process to trust web process it is providing the right isNavigatingToAppBoundDomain information.

View in context: https://bugs.webkit.org/attachment.cgi?id=396704&action=review

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:428
>  

We are currently creating a SWServer at creation of the NetworkConnectionToWebProcess (see establishSWServerConnection) based on parent entitlement.
We should probably update that code if we want things to work.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:431
> +    auto allowServiceWorkersForAppBoundDomain = loader.parameters().isNavigatingToAppBoundDomain && *loader.parameters().isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes;

Are we correctly updating loader.parameters().isNavigatingToAppBoundDomain upon redirection?

Another approach would be to set loadParameters.serviceWorkersMode to ServiceWorkerMode::None in WebLoaderStrategy based on navigation app bound value.
No change needed here or we could then return early here by checking the mode if we do not want to create a swConnection().

Ultimately, it seems better though to leave that code as is and instead control which service workers can be registered/imported.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3154
> +#endif

The issue here is that the preferences are process wide.
If we end up with two pages on the same process, the preference will be set correctly for one page, not the other.

Instead, we could have a simple approach where the API is available but service worker registration would fail for some domains, as if quota is null.

Also, if a top level page is app bound, I am not sure we want third party iframes to have access to service workers.
It would be nice to have a test for that.

On the other side of it, if we have something like a control scheme top level frame which embeds a frame from the app bound domain, we might want to allow service worker?

[Kate Cheney]

(In reply to youenn fablet from comment #12)
> Comment on attachment 396704 [details]
> Patch
> 

> This looks indeed better.
> 
> If we can, I would like to see whether we can pass to NetworkProcess, as
> part of WebsiteDataStore parameters, the app bound domain information.
> SWServer would then be taught about the domains for which service worker
> registrations would be allowed (SWServer::scheduleJob and
> SWServer::addRegistrationFromStore).

The app bound domain information is read off the background thread, and would require a completion handler in WebsiteDataStore::platformSetNetworkParameters, which does not seem feasible because of all the call sites for this function. I think passing this to the network process would require separate IPC and a completion handler somewhere in NetworkConnectionToWebProcess when creating the SW server.

> 
> The additional benefit is that we would no longer need the network process
> to trust web process it is providing the right isNavigatingToAppBoundDomain
> information.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396704&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:428
> >  
> 
> We are currently creating a SWServer at creation of the
> NetworkConnectionToWebProcess (see establishSWServerConnection) based on
> parent entitlement.
> We should probably update that code if we want things to work.

When calling swConnection(), establishSWServerConnection() will be called if there is no connection. So we can remove this call from the constructor, but I assumed eagerly establishing the connection had some benefits.

> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:431
> > +    auto allowServiceWorkersForAppBoundDomain = loader.parameters().isNavigatingToAppBoundDomain && *loader.parameters().isNavigatingToAppBoundDomain == NavigatingToAppBoundDomain::Yes;
> 
> Are we correctly updating loader.parameters().isNavigatingToAppBoundDomain
> upon redirection?
> 

From testing this, I think so. But where can I look for redirects in code so I can make sure?

> Another approach would be to set loadParameters.serviceWorkersMode to
> ServiceWorkerMode::None in WebLoaderStrategy based on navigation app bound
> value.
> No change needed here or we could then return early here by checking the
> mode if we do not want to create a swConnection().
> 

This seems like a good idea.

> Ultimately, it seems better though to leave that code as is and instead
> control which service workers can be registered/imported.

If I can figure out a way to easily send the app bound domain information to the network process, I will try to do this. As mentioned above, I'm not sure it is possible in WebsiteDataStoreParameters.

> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3154
> > +#endif
> 
> The issue here is that the preferences are process wide.
> If we end up with two pages on the same process, the preference will be set
> correctly for one page, not the other.

This is news to me. Good to know.

> 
> Instead, we could have a simple approach where the API is available but
> service worker registration would fail for some domains, as if quota is null.
> 

I agree that would be simpler. Does this mean I can remove all changes from WebPageProxy.cpp/Webpage.cpp?

> Also, if a top level page is app bound, I am not sure we want third party
> iframes to have access to service workers.
> It would be nice to have a test for that.
> 

Good idea. I'll add a test for this.

> On the other side of it, if we have something like a control scheme top
> level frame which embeds a frame from the app bound domain, we might want to
> allow service worker?

This is also an interesting idea. It might be better in a followup patch.



Thank you for the comments!

[Kate Cheney]

(In reply to katherine_cheney from comment #13)
> (In reply to youenn fablet from comment #12)
> > Comment on attachment 396704 [details]
> > Patch
> > 
> 
> > This looks indeed better.
> > 
> > If we can, I would like to see whether we can pass to NetworkProcess, as
> > part of WebsiteDataStore parameters, the app bound domain information.
> > SWServer would then be taught about the domains for which service worker
> > registrations would be allowed (SWServer::scheduleJob and
> > SWServer::addRegistrationFromStore).
> 
> The app bound domain information is read off the background thread, and
> would require a completion handler in
> WebsiteDataStore::platformSetNetworkParameters, which does not seem feasible
> because of all the call sites for this function. I think passing this to the
> network process would require separate IPC and a completion handler
> somewhere in NetworkConnectionToWebProcess when creating the SW server.
> 
**I meant NetworkProcess when creating the SW server.

[Brent Fulgham]

Comment on attachment 396704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396704&action=review

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:3154
>>> +#endif
>> 
>> The issue here is that the preferences are process wide.
>> If we end up with two pages on the same process, the preference will be set correctly for one page, not the other.
>> 
>> Instead, we could have a simple approach where the API is available but service worker registration would fail for some domains, as if quota is null.
>> 
>> Also, if a top level page is app bound, I am not sure we want third party iframes to have access to service workers.
>> It would be nice to have a test for that.
>> 
>> On the other side of it, if we have something like a control scheme top level frame which embeds a frame from the app bound domain, we might want to allow service worker?
> 
> This is news to me. Good to know.

I don't think child frames within app bound domains is in scope for this change, but an interesting idea for a future improvement.

[Kate Cheney]

Created attachment 396786 [details]
Patch

[Kate Cheney]

OK, I think this patch is getting closer. The SW API is now available for all WebViews, but in SWServer the service worker registration is filtered by the list of app-bound domains (or the presence of the entitlement).

[youenn fablet]

Comment on attachment 396786 [details]
Patch

Overall looks good to me, a few comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=396786&action=review

> Source/WebKit/ChangeLog:9
> +        Exposes the service worker API to all WebViews and only restricts access

I don't think we want to expose the service worker API for apps that are neither appboud nor without entitlement.

> Source/WebCore/workers/service/server/SWServer.cpp:159
> +    getAppBoundDomains([this, weakThis = makeWeakPtr(this), data = WTFMove(data)] () mutable {

check weakThis

> Source/WebCore/workers/service/server/SWServer.cpp:331
> +void SWServer::getAppBoundDomains(CompletionHandler<void()>&& completionHandler)

Sounds good like this.
Another approach would be something like: void SWServer::validateRegistrationDomain(const URL&, CompletionHandler<void(bool)>&&). or CompletionHandler<void(Error)>&&

> Source/WebCore/workers/service/server/SWServer.cpp:333
> +    if (!m_appBoundDomains.isEmpty()) {

No need to fetch if we have the entitlement.
Also, we should fetch these values only once, which might not be the case if m_appBoundDomains is empty.

> Source/WebCore/workers/service/server/SWServer.cpp:338
> +    m_appBoundDomainsCallback([this, weakThis = makeWeakPtr(this), completionHandler = WTFMove(completionHandler)](auto&& appBoundDomains) mutable {

Add a weakThis check.

> Source/WebCore/workers/service/server/SWServer.cpp:349
> +    getAppBoundDomains([this, weakThis = makeWeakPtr(this), jobData = WTFMove(jobData)] () mutable {

weakThis check

> Source/WebCore/workers/service/server/SWServer.cpp:350
> +        if (m_hasServiceWorkerEntitlement || m_appBoundDomains.contains(WebCore::RegistrableDomain(jobData.scriptURL))) {

If this is not allowed, we should probably fail the job and add a test displaying the error the page will see.

> Source/WebCore/workers/service/server/SWServer.h:129
> +    using AppBoundDomainsCallback = Function<void(CompletionHandler<void(HashSet<WebCore::RegistrableDomain>&&)>&&)>;

Should be a CompletionHandler I believe.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:453
>  #endif

replace by #else #endif

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:527
> +    pageConfiguration->preferences()->setServiceWorkerEntitlementDisabledForTesting(!![_configuration preferences]._serviceWorkerEntitlementDisabledForTesting);

I think we want to keep something like:
if (!WTF::processHasEntitlement("com.apple.developer.WebKit.ServiceWorkers") && !isAppBound)
     pageConfiguration->preferences()->setServiceWorkersEnabled(false);

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1468
> +            completionHandler(WTFMove(appBoundDomainsCopy));

Can we just write completionHandler(appBoundDomains)?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1469
> +        });

return early.

[Kate Cheney]

(In reply to youenn fablet from comment #18)
> Comment on attachment 396786 [details]
> Patch
> 
> Overall looks good to me, a few comments.
> 

Thank you. Working on these changes now.

Two questions:

1) Exposing the service worker API is just setting the runtimeEnabledFeature flag, correct?

2) If we expose the SW APIs to pages which are app-bound or have the entitlement, non-app bound service worker requests will never make it to the SWServer check, so isn't it unnecessary to have both the API gated and the network process check?

(I am unsure how I would have a test which checks the SWServer error message if the API is disabled for SW loads).

[Kate Cheney]

Comment on attachment 396786 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396786&action=review

>> Source/WebCore/workers/service/server/SWServer.cpp:333
>> +    if (!m_appBoundDomains.isEmpty()) {
> 
> No need to fetch if we have the entitlement.
> Also, we should fetch these values only once, which might not be the case if m_appBoundDomains is empty.

Good catch

>> Source/WebCore/workers/service/server/SWServer.h:129
>> +    using AppBoundDomainsCallback = Function<void(CompletionHandler<void(HashSet<WebCore::RegistrableDomain>&&)>&&)>;
> 
> Should be a CompletionHandler I believe.

Not sure what you mean here. I think it needs to be a function so it can take the sessionID from the Network Process?

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1468
>> +            completionHandler(WTFMove(appBoundDomainsCopy));
> 
> Can we just write completionHandler(appBoundDomains)?

I don't think so, because appBoundDomains is a reference to a class member variable. I am not an expert at WTFMove() though.

[Kate Cheney]

Created attachment 396814 [details]
Patch

[Kate Cheney]

Created attachment 396816 [details]
Patch

[Kate Cheney]

Created attachment 396818 [details]
Patch

[Brent Fulgham]

Comment on attachment 396818 [details]
Patch

Thanks for taking this on -- I think this looks great! We can polish more this week.

[EWS]

Committed r260303: <https://trac.webkit.org/changeset/260303>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396818 [details].

[Brent Fulgham]

Note: A MacCatalyst build fix was needed:

https://trac.webkit.org/changeset/260372/webkit

[Kate Cheney]

Note: Another MacCatalyst build fix was needed

https://trac.webkit.org/changeset/260405/webkit