WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194830
[WPE] Do not create a PlatformDisplay in the Service Worker process
https://bugs.webkit.org/show_bug.cgi?id=194830
Summary
[WPE] Do not create a PlatformDisplay in the Service Worker process
Loïc Yhuel
Reported
2019-02-19 14:48:33 PST
[WPE] Do not create a PlatformDisplay in the Service Worker process
Attachments
Patch
(3.26 KB, patch)
2019-02-19 15:01 PST
,
Loïc Yhuel
no flags
Details
Formatted Diff
Diff
Patch
(6.82 KB, patch)
2019-02-25 13:12 PST
,
Loïc Yhuel
no flags
Details
Formatted Diff
Diff
Patch
(6.79 KB, patch)
2019-02-26 02:28 PST
,
Loïc Yhuel
no flags
Details
Formatted Diff
Diff
Patch
(5.93 KB, patch)
2019-02-27 17:39 PST
,
Loïc Yhuel
no flags
Details
Formatted Diff
Diff
Patch
(6.39 KB, patch)
2019-03-04 10:29 PST
,
Loïc Yhuel
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Loïc Yhuel
Comment 1
2019-02-19 15:01:54 PST
Created
attachment 362440
[details]
Patch
Carlos Garcia Campos
Comment 2
2019-02-20 00:25:36 PST
Comment on
attachment 362440
[details]
Patch I think this will be easier after
bug #194494
Loïc Yhuel
Comment 3
2019-02-20 02:39:01 PST
(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
Comment 4
2019-02-20 02:46:41 PST
(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
Comment 5
2019-02-20 04:11:30 PST
(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
Comment 6
2019-02-25 13:12:22 PST
Created
attachment 362919
[details]
Patch
Loïc Yhuel
Comment 7
2019-02-26 02:28:03 PST
Created
attachment 362973
[details]
Patch
Zan Dobersek
Comment 8
2019-02-27 05:29:31 PST
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
Comment 9
2019-02-27 07:26:54 PST
Problem is it's not actually used in the web process.
Loïc Yhuel
Comment 10
2019-02-27 09:49:33 PST
(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
Comment 11
2019-02-27 17:39:25 PST
Created
attachment 363167
[details]
Patch
Loïc Yhuel
Comment 12
2019-02-27 17:49:58 PST
(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
Comment 13
2019-03-01 00:29:50 PST
(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 14
2019-03-01 00:34:58 PST
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
Comment 15
2019-03-01 05:22:10 PST
(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
Comment 16
2019-03-04 05:59:32 PST
(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
Comment 17
2019-03-04 09:26:07 PST
(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
Comment 18
2019-03-04 10:19:45 PST
(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
Comment 19
2019-03-04 10:29:50 PST
Created
attachment 363524
[details]
Patch
WebKit Commit Bot
Comment 20
2019-03-05 03:59:33 PST
Comment on
attachment 363524
[details]
Patch Clearing flags on attachment: 363524 Committed
r242437
: <
https://trac.webkit.org/changeset/242437
>
WebKit Commit Bot
Comment 21
2019-03-05 03:59:35 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug