Bug 213573 – Allow service workers for web browsers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We should allow service workers for full web browsing applications

[Kate Cheney]

<rdar://problem/64712630>

[Kate Cheney]

Created attachment 402685 [details]
Patch

[youenn fablet]

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.

[Kate Cheney]

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

[youenn fablet]

Let's see how the simple combining patch will be.
If it is very small, then manual testing might be good enough.

[Kate Cheney]

Created attachment 402745 [details]
Patch

[Brent Fulgham]

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.

[Kate Cheney]

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

[Kate Cheney]

Created attachment 402750 [details]
Patch

[Brent Fulgham]

Comment on attachment 402750 [details]
Patch

r=me

[EWS]

Committed r263525: <https://trac.webkit.org/changeset/263525>

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