Bug 177041

Summary: [WPE][GTK] Merge ProcessLauncher[WPE,GTK]
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WPE WebKitAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, mcatanzaro, webkit-bug-importer
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch cgarcia: review+, cgarcia: commit-queue-

Description Michael Catanzaro 2017-09-16 14:54:29 PDT
ProcessLauncherWPE.cpp and ProcessLauncherGTK.cpp contain largely duplicated code. Let's merge them together as ProcessLauncherGLib.cpp.
Comment 1 Michael Catanzaro 2017-09-16 14:58:48 PDT
Created attachment 321014 [details]
Patch
Comment 2 Carlos Garcia Campos 2017-09-17 01:22:29 PDT
Comment on attachment 321014 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321014&action=review

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:104
> +        nargs = 5;

nargs++ here would be safer in case we add a new one

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:113
> +        for (auto it = splitArgs.begin(); it != splitArgs.end(); it++)

I know this was existing code, but we could take advantage of this patch to modernize it a bit.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:123
> +    for (auto it = prefixArgs.begin(); it != prefixArgs.end(); it++)

Ditto.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:132
> +    argv[i++] = const_cast<char*>(realPluginPath.data());

I think this need to be protected by a ENABLE(NETSCAPE_PLUGIN_API) ifdef.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:136
> +    if (!g_spawn_async(0, argv, 0, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, childSetupFunction, GINT_TO_POINTER(socketPair.server), &pid, &error.outPtr())) {

0 -> nullptr

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> +        g_printerr("Unable to fork a new WebProcess: %s.\n", error->message);

WTFLogAlways?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:151
> +    RefPtr<ProcessLauncher> protectedThis(this);
> +    IPC::Connection::Identifier serverSocket = socketPair.server;
> +    RunLoop::main().dispatch([protectedThis, pid, serverSocket] {

RunLoop::main().dispatch([protectedThis = makeRef(*this), pid, serverSocket = socketPair.server] {

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:152
> +        protectedThis->didFinishLaunchingProcess(pid, serverSocket);

You can also capture this for convenience and then you don't need to capture pid

didFinishLaunchingProcess(m_processIdentifier, serverSocket);

I know all this is just copied code, not yours, I don't like making refactorings or other changes in behavior in this kind of patches, but coding style and modernization is always welcome, I think :-)
Comment 3 Michael Catanzaro 2017-09-17 07:29:46 PDT
(In reply to Carlos Garcia Campos from comment #2)
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:132
> > +    argv[i++] = const_cast<char*>(realPluginPath.data());
> 
> I think this need to be protected by a ENABLE(NETSCAPE_PLUGIN_API) ifdef.

Yes, I broke this by moving realPluginPath inside the ifdef. I'll do this:

#if ENABLE(NETSCAPE_PLUGIN_API)
    argv[i++] = const_cast<char*>(realPluginPath.data());
#else
    argv[i++] = nullptr;
#endif
    argv[i++] = nullptr;

This will avoid leaving uninitialized stack memory in that index. It's awkward. This messiness should all be replaced with a Vector, but that will be a follow-up patch.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> > +        g_printerr("Unable to fork a new WebProcess: %s.\n", error->message);
> 
> WTFLogAlways?

This should be a fatal error, so I'll change it to g_error(). I'll also change the error message to say "child process" rather than "WebProcess" since that has been on my TODO list for at least two years now. Also, "g_printerr() should not be used from within libraries" according to its documentation.
Comment 4 Michael Catanzaro 2017-09-17 07:34:06 PDT
Committed r222133: <http://trac.webkit.org/changeset/222133>
Comment 5 Michael Catanzaro 2017-09-17 07:37:35 PDT
Committed r222134: <http://trac.webkit.org/changeset/222134>