Summary: | Remove ServiceWorkerProcessProxy | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | Service Workers | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, beidson, cdumez, commit-queue, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
youenn fablet
2019-09-05 05:25:34 PDT
Created attachment 378077 [details]
Patch
Created attachment 378081 [details]
Patch
Created attachment 378182 [details]
Patch
Comment on attachment 378182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378182&action=review > Source/WebCore/workers/service/ServiceWorkerProvider.cpp:70 > + if (dummyDocumentIdentifiers.contains(document->identifier())) Why cannot we simply use if (!document->page() || document->page()->isUtilityPage()) continue; ? > Source/WebCore/workers/service/context/SWContextManager.cpp:180 > +HashSet<DocumentIdentifier> SWContextManager::dummyDocumentIdentifiers() const Not convinced this is needed. > Source/WebCore/workers/service/context/SWContextManager.cpp:193 > + m_pendingServiceWorkerTerminationRequests.add(serviceWorker->identifier(), makeUnique<ServiceWorkerTerminationRequest>(*this, serviceWorker->identifier(), 10_s)); We likely want a global constant for this 10_s. > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.h:79 > + DocumentIdentifier dummyDocumentIdentifier() const { return m_document->identifier(); } Not convinced this is needed. > Source/WebKit/UIProcess/WebProcessProxy.cpp:137 > +Ref<WebProcessProxy> WebProcessProxy::createForServiceWorkers(WebProcessPool& processPool, RegistrableDomain&& registrableDomain, WebsiteDataStore& websiteDataStore) Why do we want a new createForServiceWorkers() factory function? Why cannot we use the regular create() and then have a setter for the service worker information (e.g. WebProcessProxy::initializeServiceWorkerInformation())? I imagine that in the future, the WebProcessProxy may already be running (for a page) and we'll need to add the service worker information to it after the fact, so such setter would be useful anyway. > Source/WebKit/UIProcess/WebProcessProxy.cpp:292 > + launchOptions.extraInitializationData.add("service-worker-process"_s, "1"_s); Looks like this is going to need to change at some point if we want process sharing between service workers and pages. I'll reduce the patch to removing ServiceWorkerProcessProxy only, we can then progressively land the changes to make service workers coexist with real pages. Created attachment 378355 [details]
Patch
> Why do we want a new createForServiceWorkers() factory function? Why cannot
> we use the regular create() and then have a setter for the service worker
> information (e.g. WebProcessProxy::initializeServiceWorkerInformation())?
> I imagine that in the future, the WebProcessProxy may already be running
> (for a page) and we'll need to add the service worker information to it
> after the fact, so such setter would be useful anyway.
I kept it this way for now, m_registrableDomain is fully controlled by WebProcessProxy for now.
Comment on attachment 378355 [details]
Patch
r=me
Comment on attachment 378355 [details] Patch Clearing flags on attachment: 378355 Committed r249647: <https://trac.webkit.org/changeset/249647> All reviewed patches have been landed. Closing bug. |