RESOLVED FIXED 29710
[Qt] QtWebKit does not support QGraphicsWidget-plugins
https://bugs.webkit.org/show_bug.cgi?id=29710
Summary [Qt] QtWebKit does not support QGraphicsWidget-plugins
J-P Nurmi
Reported 2009-09-24 03:56:58 PDT
Even though QWebGraphicsItem has been introduced, FrameLoaderClientQt does not support QGraphicsWidget-plugins.
Attachments
Patched FrameLoaderClientQt to support QGraphicsWidget-plugins. (3.21 KB, patch)
2009-09-24 04:04 PDT, J-P Nurmi
hausmann: review-
Revised patch (5.51 KB, patch)
2009-09-29 04:49 PDT, J-P Nurmi
hausmann: review-
Third attempt (6.85 KB, patch)
2009-10-05 06:04 PDT, J-P Nurmi
hausmann: review+
J-P Nurmi
Comment 1 2009-09-24 04:04:23 PDT
Created attachment 40057 [details] Patched FrameLoaderClientQt to support QGraphicsWidget-plugins.
J-P Nurmi
Comment 2 2009-09-24 04:14:46 PDT
This makes it possible reimplement QWebPage::createPlugin() in the following manner: QObject* MyWebPage::createPlugin(const QString& classId, const QUrl& url, const QStringList& paramNames, const QStringList& paramValues) { // ... return new MyGraphicsWidget(qobject_cast<QGraphicsWidget*>(parent())); } This just assumes that QWebGraphicsItem is the parent of the QWebPage instance. Perhaps providing a QWebGraphicsItem accessor in QWebPage API would be needed, since there is no way to get the associated QWebGraphicsItem. QWebPage::view() returns the graphics view...
Kenneth Rohde Christiansen
Comment 3 2009-09-25 07:17:09 PDT
It is about time moving these QtPlugin code out of the FrameLoaderClient and put it with the rest of the plugin code.
Simon Hausmann
Comment 4 2009-09-28 13:04:00 PDT
Comment on attachment 40057 [details] Patched FrameLoaderClientQt to support QGraphicsWidget-plugins. 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 :)
J-P Nurmi
Comment 5 2009-09-29 04:49:32 PDT
Created attachment 40295 [details] Revised patch Revised patch: - added QtPluginGraphicsWidget::create() - added QWebPagePrivate::graphicsWidget - made FrameLoaderClientQt::createPlugin() use QWebPagePrivate::graphicsWidget as the parent item I second Simon that moving that plugin code where it belongs could be a separate commit.
Simon Hausmann
Comment 6 2009-10-02 07:35:15 PDT
Comment on attachment 40295 [details] Revised patch Hi J-P, sorry for the long delay :-( I think your patch is good, I like the way that it's not neccessary for subclasses to provide a parent item. However I think the approach of storing a pointer to the QGraphicsWidget is a bit fragile. I would prefer of we could extend QWebPageClient with a virtual function that returns a QObject that is to be used as parent for plugins. In the implementation it would look like this: class QWebPageClient { ... QObject* pluginParent() const = 0; ... } and then QObject* QWebViewPrivate::pluginParent() const { return q_func(); } QObject* QGraphicsWebViewPrivate::pluginParent() const { return q_func(); } > + graphicsWidget->setParentItem(m_webFrame->page()->d->graphicsWidget); ... and then the above becomes: graphicsWidget->setParentItem(qobject_cast<QGraphicsObject*>(m_webFrame->page()->d->client->pluginParent() And the same for the QWidget based plugins.
Kenneth Rohde Christiansen
Comment 7 2009-10-04 07:55:08 PDT
> ... and then the above becomes: > > graphicsWidget->setParentItem(qobject_cast<QGraphicsObject*>(m_webFrame->page()->d->client->pluginParent() > > And the same for the QWidget based plugins. Maybe that should be used for the Netscape plugins as well.
J-P Nurmi
Comment 8 2009-10-05 06:04:43 PDT
Created attachment 40629 [details] Third attempt
Simon Hausmann
Comment 9 2009-10-05 06:58:06 PDT
Comment on attachment 40629 [details] Third attempt Thanks J-P :)
Simon Hausmann
Comment 10 2009-10-05 07:03:33 PDT
When landing I'll have to replace the Qt version check with one that requires Qt 4.6, as this patch breaks the build against Qt 4.6 due to the use of QGraphicsObject.
Simon Hausmann
Comment 11 2009-10-05 07:03:50 PDT
Note You need to log in before you can comment on or make changes to this bug.