[webkit-reviews] review denied: [Bug 57617] [Qt][WK2] Implement PLUGIN_PROCESS : [Attachment 89585] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Balazs Kelemen
<kbalazs at webkit.org>'s request for review:
Bug 57617: [Qt][WK2] Implement PLUGIN_PROCESS
https://bugs.webkit.org/show_bug.cgi?id=57617

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89585&action=review

> Source/WebKit2/Platform/CoreIPC/Attachment.h:53
> -    Attachment(int fileDescriptor, size_t);
> +    Attachment(int fileDescriptor, size_t = 0);

This API look rather strange, it should be split into two constructors.

> Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp:-72
> -    ASSERT(!isNull());
> -

Benjamin: we are in SharedMemoryUnix, so the assert totally make sense.
So Either you should have a new encoder for Arguments (with new asserts
depending on the type), or you need to fix the broken assertions.

> Source/WebKit2/PluginProcess/PluginProcess.cpp:48
> +// FIXME: the LOG macro don't work without that. Should be fixed there.
> +using namespace WebCore;

Benjamin: Should you rely add a dependency to WebCore here? You don't want to
link to WebCore.

> Source/WebKit2/PluginProcess/PluginProcess.cpp:190
> +	       ASSERT_NOT_REACHED();
> +	       return;

Benjamin: you have cleaning code to close the socket on the previous call to
fcntl, but not on this one.

> Source/WebKit2/PluginProcess/PluginProcess.cpp:200
> -    ASSERT_NOT_REACHED();
> +    notImplemented();

With your change it does not assert in debug. The notImplemented() instead of
the comment is a good idea.

> Source/WebKit2/PluginProcess/qt/PluginControllerProxyQt.cpp:48
> -void PluginControllerProxy::platformGeometryDidChange(const IntRect&
frameRect, const IntRect&)
> +void PluginControllerProxy::platformGeometryDidChange()

Benjamin: ???

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:36
> +#include <QDebug>

Benjamin: ???

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:38
> +#include <QtGlobal>

Benjamin: ???

> Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:50
> +    QApplication* app = new QApplication(argc, argv);

Benjamin: why are you creating that on the heap (and leaking it?)?