Bug 29710 - [Qt] QtWebKit does not support QGraphicsWidget-plugins
: [Qt] QtWebKit does not support QGraphicsWidget-plugins
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
: Qt
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-24 03:56 PDT by J-P Nurmi
Modified: 2009-10-05 07:03 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description J-P Nurmi 2009-09-24 03:56:58 PDT
Even though QWebGraphicsItem has been introduced, FrameLoaderClientQt does not support QGraphicsWidget-plugins.
Comment 1 J-P Nurmi 2009-09-24 04:04:23 PDT
Created attachment 40057 [details]
Patched FrameLoaderClientQt to support QGraphicsWidget-plugins.
Comment 2 J-P Nurmi 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...
Comment 3 Kenneth Rohde Christiansen 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.
Comment 4 Simon Hausmann 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 :)
Comment 5 J-P Nurmi 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.
Comment 6 Simon Hausmann 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.
Comment 7 Kenneth Rohde Christiansen 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.
Comment 8 J-P Nurmi 2009-10-05 06:04:43 PDT
Created attachment 40629 [details]
Third attempt
Comment 9 Simon Hausmann 2009-10-05 06:58:06 PDT
Comment on attachment 40629 [details]
Third attempt

Thanks J-P :)
Comment 10 Simon Hausmann 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.
Comment 11 Simon Hausmann 2009-10-05 07:03:50 PDT
Committed r49092: <http://trac.webkit.org/changeset/49092>