Bug 29710 - [Qt] QtWebKit does not support QGraphicsWidget-plugins
: [Qt] QtWebKit does not support QGraphicsWidget-plugins
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Qt
:
:
  Show dependency treegraph
 
Reported: 2009-09-24 03:56 PST by
Modified: 2009-10-05 07:03 PST (History)


Attachments
Patched FrameLoaderClientQt to support QGraphicsWidget-plugins. (3.21 KB, patch)
2009-09-24 04:04 PST, J-P Nurmi
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Revised patch (5.51 KB, patch)
2009-09-29 04:49 PST, J-P Nurmi
hausmann: review-
Review Patch | Details | Formatted Diff | Diff
Third attempt (6.85 KB, patch)
2009-10-05 06:04 PST, J-P Nurmi
hausmann: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-09-24 03:56:58 PST
Even though QWebGraphicsItem has been introduced, FrameLoaderClientQt does not support QGraphicsWidget-plugins.
------- Comment #1 From 2009-09-24 04:04:23 PST -------
Created an attachment (id=40057) [details]
Patched FrameLoaderClientQt to support QGraphicsWidget-plugins.
------- Comment #2 From 2009-09-24 04:14:46 PST -------
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...
------- Comment #3 From 2009-09-25 07:17:09 PST -------
It is about time moving these QtPlugin code out of the FrameLoaderClient and put it with the rest of the plugin code.
------- Comment #4 From 2009-09-28 13:04:00 PST -------
(From update of attachment 40057 [details])
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 :)
------- Comment #5 From 2009-09-29 04:49:32 PST -------
Created an attachment (id=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.
------- Comment #6 From 2009-10-02 07:35:15 PST -------
(From update of attachment 40295 [details])
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.
------- Comment #7 From 2009-10-04 07:55:08 PST -------
> ... 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.
------- Comment #8 From 2009-10-05 06:04:43 PST -------
Created an attachment (id=40629) [details]
Third attempt
------- Comment #9 From 2009-10-05 06:58:06 PST -------
(From update of attachment 40629 [details])
Thanks J-P :)
------- Comment #10 From 2009-10-05 07:03:33 PST -------
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.
------- Comment #11 From 2009-10-05 07:03:50 PST -------
Committed r49092: <http://trac.webkit.org/changeset/49092>