[webkit-reviews] review denied: [Bug 29710] [Qt] QtWebKit does not support QGraphicsWidget-plugins : [Attachment 40057] Patched FrameLoaderClientQt to support QGraphicsWidget-plugins.

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied J-P Nurmi <jpnurmi at gmail.com>'s
request for review:
Bug 29710: [Qt] QtWebKit does not support QGraphicsWidget-plugins
https://bugs.webkit.org/show_bug.cgi?id=29710

Attachment 40057: Patched FrameLoaderClientQt to support
QGraphicsWidget-plugins.
https://bugs.webkit.org/attachment.cgi?id=40057&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
First of all: Thanks for the patch! :)

(feel free to use your company email address, btw *hint* *hint* :-)

I think your patch needs only a few minor adjustments. I agree
however with Kenneth that it would be best to move those plugin wrapper
classes to WebCore/plugins/qt.

But IMHO that'd be okay to do in a separate commit.

> +		   RefPtr<QtPluginGraphicsWidget> w = adoptRef(new
QtPluginGraphicsWidget(graphicsWidget));

I think QtPluginGraphicsWidget should implement the create() pattern instead of
having
a public constructor, like all RefCounted classes.

Another thing: Right now the person implementing Page::createPlugin() has to
provide
a parent to plugin graphics widget itself. This is not the case with the
QWidget based approach. I think it would be a better API if the caller in
FrameLoaderClientQt.cpp could take care of giving the returned graphicswidget
the correct parent instead of requiring the implementor to think about this
detail that I'm _sure_ many people will forget about :)