[webkit-reviews] review granted: [Bug 87672] [Qt] Add quirks for running the web process in a profiler shell, like valgrind : [Attachment 144525] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has granted Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 87672: [Qt] Add quirks for running the web process in a profiler shell,
like valgrind
https://bugs.webkit.org/show_bug.cgi?id=87672

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

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144525&action=review


I think this is a very good idea (r+). But please fix the string handling
before landing. Just call qgetenv() every time and remember to use constData()
if you can.

In this case I would even argue that the use of arg() is overkill, this is
merely about concatenating a bunch of strings, so perhaps it would make sense
to use the string builder in Qt with the % operator:

http://doc-snapshot.qt-project.org/5.0/qstring.html#more-efficient-string-const
ruction

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:105
> +    static QByteArray webProcessPrefix =
qgetenv("QT_WEBKIT_WEB_PROCESS_COMMAND_PREFIX");
> +    static QByteArray pluginProcessPrefix =
qgetenv("QT_WEBKIT_PLUGIN_PROCESS_COMMAND_PREFIX");

I wouldn't simply bother with making those global variables. The strings are
already global in the environment and we don't launch processes often enough to
IMHO justify the time we "save" by not extracting the strings again out of the
environment. At the same time the _copy_ of the string uses memory for no
reason :)

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:118
> +    QString commandLine = prefix.isEmpty() ? QStringLiteral("%1 %2") :
QString(QStringLiteral("%1 %2 %3")).arg(QLatin1String(prefix.data()));

By calling data() instead of constData() you'll end up creating a copy of the
QByteArray for no reason. The non-const data() calls detach() and will detach
because the refcount will be 2 (the original prefix variable holds one ref, the
local prefix variable another one).