[webkit-reviews] review granted: [Bug 72121] [Qt][WK2] Set up plugin process on Unix : [Attachment 130819] 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 72121: [Qt][WK2] Set up plugin process on Unix
https://bugs.webkit.org/show_bug.cgi?id=72121

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

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


r=me with comments. A few things that should be fixed before landing. The
QProcess one could be done in a separate patch or you could fix up this one and
upload a new version of this patch. Whichever you prefer :)

> Source/WebKit2/PluginProcess.pro:17
> +CONFIG += qtwebkit

Note that when the fix for bug #80590 lands, this has to be written as QT +=
webkit instead.

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:75
> +    QApplication app(argc, argv);

In the future we should change this to be QGuiApplication. At the latest as
part of the fix for bug #79458.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:146
> +    unsigned newLenght = 0;

newLenght -> newLength :)

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:163
> +static inline void putCharToStdOut(char c)
> +{
> +    int result;
> +    while ((result = fputc(c, stdout)) == EOF && errno == EINTR) { }
> +    ASSERT(result != EOF);
> +}

This function seems unused. Looks like an earlier left-over that could be
removed before landing.

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:64
> +    QProcess* process = new QProcess;
> +    QObject::connect(process, SIGNAL(finished(int)), process,
SLOT(deleteLater()), Qt::QueuedConnection);
> +    process->setReadChannel(QProcess::StandardOutput);
> +    process->start(commandLine);
> +
> +    if (!process->waitForFinished()
> +	   || process->exitStatus() != QProcess::NormalExit
> +	   || process->exitCode() != EXIT_SUCCESS) {
> +	   process->kill();
> +	   return false;
> +    }
> +

Since you now deal with the process synchronously, I think you can safely
allocate the process object on the stack and get rid of the finished ->
deleteLater connection.