[webkit-reviews] review denied: [Bug 105156] [EFL][WK2] Process launcher uses system() for wrapping the WebProcess when using WEB_PROCESS_CMD_PREFIX : [Attachment 201709] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Sergio Correia
<sergio.correia at openbossa.org>'s request for review:
Bug 105156: [EFL][WK2] Process launcher uses system() for wrapping the
WebProcess when using WEB_PROCESS_CMD_PREFIX
https://bugs.webkit.org/show_bug.cgi?id=105156

Attachment 201709: Patch
https://bugs.webkit.org/attachment.cgi?id=201709&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=201709&action=review


> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:49
> +namespace WTF {
> +template <> void deleteOwnedArrayPtr<char* const>(char* const* args)
> +{
> +    if (!args)
> +	   return;
> +
> +    for (size_t i = 0; args[i]; ++i)
> +	   delete[] args[i];
> +    delete[] args;
> +}
> +} // namespace WTF
> +

This should not be moved to WTF, this should not exist at all.

You can change behaviors of WebKit ADTs with traits and template arguments. But
certainly _NOT_ by doing template specialization of internal structure.

By doing this, you are breaking the semantic of OwnArrayPtr. Anyone who modify
this file later (and any reviewer checking patch) would assume the common well
defined behavior. This would introduce subtle bugs and make reviews impossible.


> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:52
> +static PassOwnArrayPtr<char* const> createArgsArray(const String& prefix,
const String& executablePath, const String& socket, const String& pluginPath)

Why not just return a Vector<OwnArrayPtr<char* const>> here?

> Source/WebKit2/UIProcess/Launcher/efl/ProcessLauncherEfl.cpp:106
> +    OwnArrayPtr<char* const> args = createArgsArray(processCmdPrefix,
executablePath, String::number(sockets[0]), pluginPath);

It looks like this should be in the if (!pid) branch.