[Webkit-unassigned] [Bug 105156] [EFL] Process launcher uses system() for wrapping the WebProcess when using WEB_PROCESS_CMD_PREFIX

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=105156





--- Comment #7 from Sergio Correia <sergio.correia at openbossa.org>  2013-05-03 05:40:04 PST ---
Thanks for the review, Mikhail.

(In reply to comment #6)
> (From update of attachment 200383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200383&action=review
> 
> > Source/WebKit2/ChangeLog:3
> > +        [EFL][WK2] ProcessLauncher: do not handle WEB_PROCESS_CMD_PREFIX with system()
> 
> Seems that the actual bug has another title.
> 

Yeah, I thought I could change it and ask someone to rename the actual bug, but I will just make the title in the patch to be the same as the bug, instead.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:40
> > +    ProcessExecArgs(String cmd)
> 
> const String&
> 
> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:46
> > +        cmd.split(' ', args);
> 
> so you've composed this string before in ProcessLauncher::launchProcess(), and now you're decomposing it again..
> 

Indeed. I will pass the possible prefix as a separate parameter and break only it. 

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:67
> > +    char* const* args() { return const_cast<char* const*>(m_argv); }
> 
> why not const char* ?
> 

I was matching the prototype of execvp():  int execvp(const char *file, char *const argv[]); 

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:71
> > +    Vector<CString> m_splitArgs;
> 
> why is that needed?
> 

Because I was making the items in argv point to them, rather than copying them with something like strdup() - which would require me to free() them in the destructor -, and hence they should exist while argv itself exist.  

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:98
> > +    String fullCmd = String::fromUTF8(executablePath.data()) + ' ' + String::number(sockets[0]) + ' ' + String::fromUTF8(pluginPath.data());
> 
> I'd consider using of StringBuilder
>

I am going to take a look at StringBuilder, but probably if I send them as separate arguments, I will be able to avoid composing it here to decompose it in the class.

> > Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:103
> > +    ProcessExecArgs execArgs(fullCmd);
> 
> why do you need a class here? seems a function would be enough
>

It seemed simpler to handle the deallocation logic in a destructor, rather than having to do it manually.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.