Bug 213573

Summary: Allow service workers for web browsers
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, bfulgham, wilander, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Kate Cheney 2020-06-24 13:45:46 PDT
We should allow service workers for full web browsing applications
Comment 1 Kate Cheney 2020-06-24 13:46:16 PDT
<rdar://problem/64712630>
Comment 2 Kate Cheney 2020-06-24 14:16:43 PDT
Created attachment 402685 [details]
Patch
Comment 3 youenn fablet 2020-06-25 01:22:33 PDT
Comment on attachment 402685 [details]
Patch

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

> Source/WebKit/ChangeLog:32
> +        may run both browser and non-browser tests.

It is not great that we have to add all these TestWebKitAPI checks.

Can we just update parentProcessHasServiceWorkerEntitlement to check for both com.apple.developer.WebKit.ServiceWorkers and com.apple.developer.web-browser entitlements?
These might be very small changes and manual testing might be good enough. Maybe we could have mini browser have this com.apple.developer.web-browser entitlement btw.
Comment 4 Kate Cheney 2020-06-25 06:52:13 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 402685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402685&action=review
> 
> > Source/WebKit/ChangeLog:32
> > +        may run both browser and non-browser tests.
> 
> It is not great that we have to add all these TestWebKitAPI checks.
> 
> Can we just update parentProcessHasServiceWorkerEntitlement to check for
> both com.apple.developer.WebKit.ServiceWorkers and
> com.apple.developer.web-browser entitlements?
> These might be very small changes and manual testing might be good enough.
> Maybe we could have mini browser have this com.apple.developer.web-browser
> entitlement btw.

Yes, combining would work. If we are happy with manual testing then I can also remove the TestWebKitAPI checks. I originally did not give mini browser the entitlement because we have tests which need to not be considered browsers. But I can probably disable the entitlement for those tests just like with the service worker entitlement.
Comment 5 youenn fablet 2020-06-25 07:00:04 PDT
Let's see how the simple combining patch will be.
If it is very small, then manual testing might be good enough.
Comment 6 Kate Cheney 2020-06-25 10:24:20 PDT
Created attachment 402745 [details]
Patch
Comment 7 Brent Fulgham 2020-06-25 10:46:02 PDT
Comment on attachment 402745 [details]
Patch

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

> Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:84
> +    static bool hasEntitlement = WTF::hasEntitlement(parentProcessConnection()->xpcConnection(), "com.apple.developer.WebKit.ServiceWorkers") || WTF::hasEntitlement(parentProcessConnection()->xpcConnection(), "com.apple.developer.web-browser");

As Kate pointed out elsewhere, we can probably now get rid of the ServiceWorker entitlement (in a separate patch!)

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:525
> +    bool isFullBrowser = WTF::processHasEntitlement("com.apple.developer.web-browser");

Why isn't this just:

bool hasServiceWorkerEntitlement = (WTF::processHasEntitlement("com.apple.developer.WebKit.ServiceWorkers") || WTF::processHasEntitlement("com.apple.developer.web-browser")) && ![_configuration preferences]._serviceWorkerEntitlementDisabledForTesting;

... And then get rid of the 'isFullBrowser' variable?

This seems to say that we would not run full web-browser tests where the SW stuff is disabled for testing.
Comment 8 Kate Cheney 2020-06-25 11:05:00 PDT
(In reply to Brent Fulgham from comment #7)
> Comment on attachment 402745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402745&action=review
> 
> > Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm:84
> > +    static bool hasEntitlement = WTF::hasEntitlement(parentProcessConnection()->xpcConnection(), "com.apple.developer.WebKit.ServiceWorkers") || WTF::hasEntitlement(parentProcessConnection()->xpcConnection(), "com.apple.developer.web-browser");
> 
> As Kate pointed out elsewhere, we can probably now get rid of the
> ServiceWorker entitlement (in a separate patch!)
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:525
> > +    bool isFullBrowser = WTF::processHasEntitlement("com.apple.developer.web-browser");
> 
> Why isn't this just:
> 
> bool hasServiceWorkerEntitlement =
> (WTF::processHasEntitlement("com.apple.developer.WebKit.ServiceWorkers") ||
> WTF::processHasEntitlement("com.apple.developer.web-browser")) &&
> ![_configuration preferences]._serviceWorkerEntitlementDisabledForTesting;
> 
> ... And then get rid of the 'isFullBrowser' variable?
> 
> This seems to say that we would not run full web-browser tests where the SW
> stuff is disabled for testing.

That makes sense, will fix and re-upload.
Comment 9 Kate Cheney 2020-06-25 11:19:15 PDT
Created attachment 402750 [details]
Patch
Comment 10 Brent Fulgham 2020-06-25 12:03:46 PDT
Comment on attachment 402750 [details]
Patch

r=me
Comment 11 EWS 2020-06-25 13:04:56 PDT
Committed r263525: <https://trac.webkit.org/changeset/263525>

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