Bug 194830 – [WPE] Do not create a PlatformDisplay in the Service Worker process

• Status: RESOLVED
• Resolution: FIXED
• Component: WPE WebKit

[Loïc Yhuel]

[WPE] Do not create a PlatformDisplay in the Service Worker process

[Loïc Yhuel]

Created attachment 362440 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 362440 [details]
Patch

I think this will be easier after bug #194494

[Loïc Yhuel]

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

[Carlos Garcia Campos]

(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

[Loïc Yhuel]

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

[Loïc Yhuel]

Created attachment 362919 [details]
Patch

[Loïc Yhuel]

Created attachment 362973 [details]
Patch

[Zan Dobersek]

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.

[Michael Catanzaro]

Problem is it's not actually used in the web process.

[Loïc Yhuel]

(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

[Loïc Yhuel]

Created attachment 363167 [details]
Patch

[Loïc Yhuel]

(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();
    }

[Zan Dobersek]

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

[Zan Dobersek]

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.

[Loïc Yhuel]

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

[Zan Dobersek]

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

[Loïc Yhuel]

(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` ?

[Zan Dobersek]

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

[Loïc Yhuel]

Created attachment 363524 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363524 [details]
Patch

Clearing flags on attachment: 363524

Committed r242437: <https://trac.webkit.org/changeset/242437>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.