[WPE] Do not create a PlatformDisplay in the Service Worker process
Created attachment 362440 [details] Patch
Comment on attachment 362440 [details] Patch I think this will be easier after bug #194494
(In reply to Carlos Garcia Campos from comment #2) > Comment on attachment 362440 [details] > Patch > > I think this will be easier after bug #194494 We would need to add a parameter to WebProcessPool::platformInitializeWebProcess (in WebProcessPool::initializeNewWebProcess, the argument would be process.isServiceWorkerProcess()).
(In reply to Loïc Yhuel from comment #3) > (In reply to Carlos Garcia Campos from comment #2) > > Comment on attachment 362440 [details] > > Patch > > > > I think this will be easier after bug #194494 > > We would need to add a parameter to > WebProcessPool::platformInitializeWebProcess (in > WebProcessPool::initializeNewWebProcess, the argument would be > process.isServiceWorkerProcess()). I would add an implementation of WebProcess::platformInitializeProcess to initialize m_processType (we need to make this non-mac specific in WebProcess.h). Then you can simply check m_pocessType from platformInitializeWebProcess
(In reply to Carlos Garcia Campos from comment #4) > (In reply to Loïc Yhuel from comment #3) > > (In reply to Carlos Garcia Campos from comment #2) > > > Comment on attachment 362440 [details] > > > Patch > > > > > > I think this will be easier after bug #194494 > > > > We would need to add a parameter to > > WebProcessPool::platformInitializeWebProcess (in > > WebProcessPool::initializeNewWebProcess, the argument would be > > process.isServiceWorkerProcess()). > > I would add an implementation of WebProcess::platformInitializeProcess to > initialize m_processType (we need to make this non-mac specific in > WebProcess.h). Then you can simply check m_pocessType from > platformInitializeWebProcess WebProcess::platformInitializeProcess is on WebProcess side, we need a test on the UIProcess side to avoid creating the fd.
Created attachment 362919 [details] Patch
Created attachment 362973 [details] Patch
Comment on attachment 362973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362973&action=review > Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:95 > -void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters) > +void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters, bool isServiceWorkerProcess) > { > #if PLATFORM(WPE) > - parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); > + if (!isServiceWorkerProcess) > + parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); Instead of the extra bool parameter to this method, there should be an extra bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters for the WPE port. It would be initialized in WebProcessPool::initializeNewWebProcess() before the platformInitializeWebProcess() call.
Problem is it's not actually used in the web process.
(In reply to Zan Dobersek from comment #8) > Comment on attachment 362973 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362973&action=review > > > Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:95 > > -void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters) > > +void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters, bool isServiceWorkerProcess) > > { > > #if PLATFORM(WPE) > > - parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); > > + if (!isServiceWorkerProcess) > > + parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); > > Instead of the extra bool parameter to this method, there should be an extra > bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters > for the WPE port. It would be initialized in > WebProcessPool::initializeNewWebProcess() before the > platformInitializeWebProcess() call. So in WebProcessPool::initializeNewWebProcess : #if PLATFORM(WPE) && ENABLE(SERVICE_WORKER) parameters.isServiceWorkerProcess = process.isServiceWorkerProcess(); #endif
Created attachment 363167 [details] Patch
(In reply to Zan Dobersek from comment #8) > Instead of the extra bool parameter to this method, there should be an extra > bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters > for the WPE port. It would be initialized in > WebProcessPool::initializeNewWebProcess() before the > platformInitializeWebProcess() call. Done. I did put in under "#if ENABLE(SERVICE_WORKER)", but I don't know if it's the right choice, since it makes the patch more complicated since those flags must be replicated in several places. The code style guidelines also don't mention the case of flags mixed with blocks, and I didn't find examples when searching quickly. First solution, with the "if (1)" to avoid any possible -Wmisleading-indentation : #if ENABLE(SERVICE_WORKER) if (!parameters.isServiceWorkerProcess) #else if (1) #endif parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); Second solution, with a one-line block (would be against the rules without the #if) : #if ENABLE(SERVICE_WORKER) if (!parameters.isServiceWorkerProcess) #endif { parameters.hostClientFileDescriptor = wpe_renderer_host_create_client(); }
(In reply to Michael Catanzaro from comment #9) > Problem is it's not actually used in the web process. OK, let's then make this more WPE-specific.
Comment on attachment 363167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363167&action=review > Source/WebKit/Shared/WebProcessCreationParameters.h:207 > +#if ENABLE(SERVICE_WORKER) > + bool isServiceWorkerProcess { false }; > +#endif Let's rename this to `isWPEClient`, and place it above the other two member variables here. Decoding/encoding sequencing should follow this order. It should also default to true, and not be guarded with ENABLE(SERVICE_WORKER). > Source/WebKit/UIProcess/WebProcessPool.cpp:944 > +#if PLATFORM(WPE) && ENABLE(SERVICE_WORKER) > + parameters.isServiceWorkerProcess = process.isServiceWorkerProcess(); > +#endif Here, `isWPEClient` should be set to the negative value of `process.isServiceWorkerProcess()`. PLATFORM(WPE) build guards are fitting, SERVICE_WORKER maybe not since the `isServiceWorkerProcess()` getter isn't guarded with it. > Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:98 > +#if ENABLE(SERVICE_WORKER) > + if (!parameters.isServiceWorkerProcess) > +#else > + if (1) > +#endif This would now be a simple, unguarded check of `isWPEClient` value. > Source/WebKit/WebProcess/glib/WebProcessGLib.cpp:59 > +#if ENABLE(SERVICE_WORKER) > + if (!parameters.isServiceWorkerProcess) > +#endif Ditto.
(In reply to Zan Dobersek from comment #14) > Comment on attachment 363167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363167&action=review > > > Source/WebKit/Shared/WebProcessCreationParameters.h:207 > > +#if ENABLE(SERVICE_WORKER) > > + bool isServiceWorkerProcess { false }; > > +#endif > > Let's rename this to `isWPEClient`, and place it above the other two member > variables here. Decoding/encoding sequencing should follow this order. It > should also default to true, and not be guarded with ENABLE(SERVICE_WORKER). > Then, should it be tested for implementationLibraryName usage too (ie not calling wpe_loader_init in the Service Worker process) ? The specific `isWPEClient` name might not be appropriate if we later find other things we can skip for Service Workers : for example the `WebCore::initializeGStreamer` in `WebProcess::platformInitializeWebProcess` is probably not needed.
(In reply to Loïc Yhuel from comment #15) > (In reply to Zan Dobersek from comment #14) > > Comment on attachment 363167 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=363167&action=review > > > > > Source/WebKit/Shared/WebProcessCreationParameters.h:207 > > > +#if ENABLE(SERVICE_WORKER) > > > + bool isServiceWorkerProcess { false }; > > > +#endif > > > > Let's rename this to `isWPEClient`, and place it above the other two member > > variables here. Decoding/encoding sequencing should follow this order. It > > should also default to true, and not be guarded with ENABLE(SERVICE_WORKER). > > > Then, should it be tested for implementationLibraryName usage too (ie not > calling wpe_loader_init in the Service Worker process) ? > Yes, the wpe_loader_init() call should also be avoided. > The specific `isWPEClient` name might not be appropriate if we later find > other things we can skip for Service Workers : for example the > `WebCore::initializeGStreamer` in `WebProcess::platformInitializeWebProcess` > is probably not needed. Makes sense. We can use the current name and expand its use over GStreamer in a future patch.
(In reply to Zan Dobersek from comment #16) > Makes sense. We can use the current name and expand its use over GStreamer > in a future patch. What do you mean by current ? `isWPEClient` or `isServiceWorkerProcess` ?
(In reply to Loïc Yhuel from comment #17) > (In reply to Zan Dobersek from comment #16) > > Makes sense. We can use the current name and expand its use over GStreamer > > in a future patch. > What do you mean by current ? `isWPEClient` or `isServiceWorkerProcess` ? `isServiceWorkerProcess` is fine. It can always be amended afterwards, anyway.
Created attachment 363524 [details] Patch
Comment on attachment 363524 [details] Patch Clearing flags on attachment: 363524 Committed r242437: <https://trac.webkit.org/changeset/242437>
All reviewed patches have been landed. Closing bug.