Bug 148616 - [EFL] Fix the problem in which environment variable included in webprocess-cmd-prefix can't be parsed
Summary: [EFL] Fix the problem in which environment variable included in webprocess-cm...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-30 09:11 PDT by Joonghun Park
Modified: 2015-10-14 03:34 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2015-08-30 09:16 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2015-08-30 09:30 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2015-08-30 20:39 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Revise log typo: execute -> execve (3.45 KB, patch)
2015-09-04 05:38 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Rename the function as parseAndRemoveEnvironments (3.42 KB, patch)
2015-10-13 23:20 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff
Patch (2.77 KB, patch)
2015-10-14 00:33 PDT, Joonghun Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joonghun Park 2015-08-30 09:11:02 PDT
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"
Comment 1 Joonghun Park 2015-08-30 09:16:21 PDT
Created attachment 260242 [details]
Patch
Comment 2 Joonghun Park 2015-08-30 09:30:53 PDT
Created attachment 260243 [details]
Patch
Comment 3 Joonghun Park 2015-08-30 20:39:25 PDT
Created attachment 260271 [details]
Patch
Comment 4 Joonghun Park 2015-09-04 05:38:25 PDT
Created attachment 260588 [details]
Revise log typo: execute -> execve
Comment 5 Gyuyoung Kim 2015-10-13 22:16:46 PDT
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.
Comment 6 Joonghun Park 2015-10-13 23:20:23 PDT
Created attachment 263061 [details]
Rename the function as parseAndRemoveEnvironments
Comment 7 Joonghun Park 2015-10-13 23:25:54 PDT
(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.
Comment 8 Gyuyoung Kim 2015-10-13 23:35:13 PDT
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.
Comment 9 Joonghun Park 2015-10-14 00:33:31 PDT
(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.
Comment 10 Joonghun Park 2015-10-14 00:33:56 PDT
Created attachment 263065 [details]
Patch
Comment 11 WebKit Commit Bot 2015-10-14 03:34:45 PDT
Comment on attachment 263065 [details]
Patch

Clearing flags on attachment: 263065

Committed r191041: <http://trac.webkit.org/changeset/191041>
Comment 12 WebKit Commit Bot 2015-10-14 03:34:50 PDT
All reviewed patches have been landed.  Closing bug.