Bug 29710 – [Qt] QtWebKit does not support QGraphicsWidget-plugins

• Component: WebKit Qt
• Status: RESOLVED FIXED

[J-P Nurmi]

Even though QWebGraphicsItem has been introduced, FrameLoaderClientQt does not support QGraphicsWidget-plugins.

[J-P Nurmi]

Created attachment 40057 [details]
Patched FrameLoaderClientQt to support QGraphicsWidget-plugins.

[J-P Nurmi]

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]

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

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

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

Created attachment 40629 [details]
Third attempt

[Simon Hausmann]

Comment on attachment 40629 [details]
Third attempt

Thanks J-P :)

[Simon Hausmann]

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]

Committed r49092: <http://trac.webkit.org/changeset/49092>