[webkit-reviews] review denied: [Bug 72121] [Qt][WK2] Set up plugin process on Unix : [Attachment 128332] Patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied 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 128332: Patch
https://bugs.webkit.org/attachment.cgi?id=128332&action=review

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


A couple of nitpicks, but otherwise this looks good to me. Let's do one last
round :)

> Source/WebKit2/PluginProcess.pro:7
> +load(features)

Is this needed?

> Source/WebKit2/PluginProcess/gtk/PluginProcessMainGtk.cpp:61
> +    fprintf(stderr, "PP running\nargs=%s,%s,%s\n", argv[0], argv[1],
argv[2]);

If arc2 == 2, which seems like a valid condition, the accessing argv[2] here
seems like an out-of-bounds array access.

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:97
> +	   // QProcess does not allow to read the output of the process after
> +	   // it has been terminated. Let's go to sleep and rely on the UI
> +	   // process to kill us.
> +	   sleep(10);

I don't understand how this is needed. The documentation of the QProcess
finished signal explains that the buffer should still be intact. Is there
perhaps an fflush(stdout); or so missing here?

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:153
> +    return adoptPtr(truncated);

I think this should be adoptArrayPtr.

> Source/WebKit2/Shared/Plugins/Netscape/x11/NetscapePluginModuleX11.cpp:172
> +    OwnPtr<char> name(truncateNewLines(metaData.name.ascii().data()));

Wait, truncateNewLines takes a String() as argument, but we're passing a const
char * as parameter, so this is implicitly constructing a String. Shouldn't
truncateNewLines take a const char* instead then?

> Source/WebKit2/Shared/qt/ShareableBitmapQt.cpp:80
> +    if (qFuzzyCompare(scaleFactor, 1)) {
> +	   paint(context, dstPoint, srcRect);
> +	   return;
> +    }

Hm, my memory is weak here, but doesn't this belong into a separate patch? :)

> Source/WebKit2/UIProcess/Plugins/qt/PluginProcessProxyQt.cpp:63
> +    QByteArray output = process->readAll();

Perhaps this could be simplified by using the patter that you can also find in
the QProcess auto-tests:

process.start();
process.waitForStarted();
if (process.waitForFinished()) {
    // success, read all stdout
} else
    // Fail, probably crashed.