[Webkit-unassigned] [Bug 57617] [Qt][WK2] Implement PLUGIN_PROCESS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=57617





--- Comment #14 from Balazs Kelemen <kbalazs at webkit.org>  2011-04-27 04:20:57 PST ---
(In reply to comment #13)
> (From update of attachment 89585 [details])
> 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.

Accept.

> 
> > 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.

As I wrote in the changelog this change was made because in PluginProxy::geometryDidChange we pass a null Handle if we do not need to allocate a new backing store. It has nothing to do with the Attachment changes.

> 
> > 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.

This is in the core part (i.e. the webkit library), not in the binary of the plugin process. Naturally this depends on 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.

Accept.

> 
> > 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.

Do you mean keep the ASSERT_NOT_REACHED()? Actually the notImplemented() is also a debug only thing. I can keep the assert but in this case an ASSERT_WITH_MESSAGE makes more sense.

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

This is a build fix.

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

It is used. What is the question? :)

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

For Q_DECL_EXPORT.

> 
> > 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?)?

Accept.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.