Bug 148616 – [EFL] Fix the problem in which environment variable included in webprocess-cmd-prefix can't be parsed

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Joonghun Park]

This patch fix the problem in which environment variable in web process-cmd-prefix can't be parsed.
process-cmd-prefix option doesn't work in two cases.
1. When executing run-layout-tests, 
2. When executing MiniBrowser with web process-cmd-prefix environment variable.

After this patch, webprocesss-cmd-prefix should include full path of executable file because execute doesn't consider PATH,
e.g. Tools/Scripts/run-webkit-tests --efl --webprocess-cmd-prefix="DISPLAY=:0 /usr/bin/xterm -e gdb --args"

[Joonghun Park]

Created attachment 260242 [details]
Patch

[Joonghun Park]

Created attachment 260243 [details]
Patch

[Joonghun Park]

Created attachment 260271 [details]
Patch

[Joonghun Park]

Created attachment 260588 [details]
Revise log typo: execute -> execve

[Gyuyoung Kim]

Comment on attachment 260588 [details]
Revise log typo: execute -> execve

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

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68
> +static void parseAndRemoveEnvsFrom(Vector<std::unique_ptr<char[]>>&& args)

s/Envs/Environments/g. Beside I think *From* is redundant.

[Joonghun Park]

Created attachment 263061 [details]
Rename the function as parseAndRemoveEnvironments

[Joonghun Park]

(In reply to comment #5)
> Comment on attachment 260588 [details]
> Revise log typo: execute -> execve
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260588&action=review
> 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:68
> > +static void parseAndRemoveEnvsFrom(Vector<std::unique_ptr<char[]>>&& args)
> 
> s/Envs/Environments/g. Beside I think *From* is redundant.

I revised this patch as you commented, Gyuyoung.

In addition to it, I'm planning to revise the document(https://trac.webkit.org/wiki/WebKitEFLLayoutTest)'s corresponding part
to align with this change if this patch is applied.

[Gyuyoung Kim]

Comment on attachment 263061 [details]
Rename the function as parseAndRemoveEnvironments

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

> Source/WebKit2/ChangeLog:8
> +        This patch fix the problem in which environment variable in web process-cmd-prefix can't be parsed.

s/fix/fixes/g

> Source/WebKit2/ChangeLog:13
> +        After this patch, webprocesss-cmd-prefix should include full path of executable file

One question, should we add full patch whenever using *webprocess-cmd-prefix* ? Isn't there any solution not to use full path ?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:80
> +        if (strchr(arg, static_cast<int>('=')) == nullptr)

Should we use static_cast<int> here ? It looks like redundant casting.

[Joonghun Park]

(In reply to comment #8)
> Comment on attachment 263061 [details]
> Rename the function as parseAndRemoveEnvironments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263061&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        This patch fix the problem in which environment variable in web process-cmd-prefix can't be parsed.
> 
> s/fix/fixes/g
> 
Done.
> > Source/WebKit2/ChangeLog:13
> > +        After this patch, webprocesss-cmd-prefix should include full path of executable file
> 
> One question, should we add full patch whenever using
> *webprocess-cmd-prefix* ? Isn't there any solution not to use full path ?
> 
I revised not to use full path. So there is no need to change the document anymore.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:80
> > +        if (strchr(arg, static_cast<int>('=')) == nullptr)
> 
> Should we use static_cast<int> here ? It looks like redundant casting.
Done.

[Joonghun Park]

Created attachment 263065 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 263065 [details]
Patch

Clearing flags on attachment: 263065

Committed r191041: <http://trac.webkit.org/changeset/191041>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.