Bug 99297 – [EFL] should make plugin process debugging easier (PLUGIN_PROCESS_COMMAND_PREFIX)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jussi Kukkonen (jku)]

I'd like to add support for PLUGIN_PROCESS_CMD_PREFIX environment variable to make debugging easier (on a debug build). Then you could do 
   PLUGIN_PROCESS_CMD_PREFIX="xterm gdb --args" MiniBrowser url/to/something/with/plugins
and end up debugging the right process without tracking forks and all that.

This would be similar to WEB_PROCESS_CMD_PREFIX implemented by a couple of ports (which we should maybe move to non-port specific files as well, but that's another bug).

[Jussi Kukkonen (jku)]

Created attachment 168653 [details]
Patch

[Alexey Proskuryakov]

A nitpick: please don't abbreviate "command".

Looks reasonable to me, Anders or Sam should review.

[Sam Weinig]

Comment on attachment 168653 [details]
Patch

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

> Source/WebKit2/UIProcess/Plugins/PluginProcessProxy.cpp:82
> +    const char* cmdPrefix = getenv("PLUGIN_PROCESS_CMD_PREFIX");
> +    if (cmdPrefix && *cmdPrefix)
> +        launchOptions.processCmdPrefix = String::fromUTF8(cmdPrefix);
> +#endif

Won't this break non-EFL builds since they don't have the launchOptions.processCmdPrefix member.  I would prefer if you clean this up by adding platformInitializeLaunchOptions() that is implemented separately.

[Jussi Kukkonen (jku)]

(In reply to comment #2)
> A nitpick: please don't abbreviate "command".

I was trying to be in line with WEB_PROCESS_CMD_PREFIX, but the code variable name I can of course change.

(In reply to comment #3)
> Won't this break non-EFL builds since they don't have the launchOptions.processCmdPrefix member.  I would prefer if you clean this up by adding platformInitializeLaunchOptions() that is implemented separately.

Oops, you are correct, managed to misread ProcessLauncher.h. I'll add a platformInitializeLaunchOptions first, thanks.

[Alexey Proskuryakov]

> I was trying to be in line with WEB_PROCESS_CMD_PREFIX, but the code variable name I can of course change.

That one also looks like it should be renamed, of course.

> Oops, you are correct, managed to misread ProcessLauncher.h. I'll add a platformInitializeLaunchOptions first, thanks.

Why are EWS bots all green then?

[Jussi Kukkonen (jku)]

(In reply to comment #5)
> > Oops, you are correct, managed to misread ProcessLauncher.h. I'll add a platformInitializeLaunchOptions first, thanks.
> 
> Why are EWS bots all green then?

I assumed the EWS bots build Release, so the ifdef would be skipped. Admitedly I don't actually know if this is true...

[Jussi Kukkonen (jku)]

Created attachment 169170 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #3)
>  I would prefer if you clean this up by adding platformInitializeLaunchOptions() that is implemented separately.

I hope this patch on top of the on in bug 99583 is what you were thinking of.

This patch still includes the "#if PLATFORM(EFL)" inside PluginProcessProxyUnix.cpp -- I have a feeling GTK would also like this feature but that should probably happen in another bug because that would mean changing the ProcessLauncher header and presumably WebProcessProxyGtk as well.

[Alexey Proskuryakov]

> I assumed the EWS bots build Release, so the ifdef would be skipped.

Indeed.

[Jussi Kukkonen (jku)]

Created attachment 170097 [details]
Patch

[WebKit Review Bot]

Comment on attachment 170097 [details]
Patch

Clearing flags on attachment: 170097

Committed r132209: <http://trac.webkit.org/changeset/132209>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.