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
Kate Cheney
2020-06-24 13:45:46 PDT
Created attachment 402685 [details]
Patch
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. (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. Let's see how the simple combining patch will be. If it is very small, then manual testing might be good enough. Created attachment 402745 [details]
Patch
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. (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. Created attachment 402750 [details]
Patch
Comment on attachment 402750 [details]
Patch
r=me
Committed r263525: <https://trac.webkit.org/changeset/263525> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402750 [details]. |