Bug 114966 – [Qt] QWebKitPlatformPlugin should export available Plugin interfaces

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Arunprasad Rajkumar]

Currently QWebKitPlatformPlugin is not exporting its plugin interfaces, implementer has redefine once again by running _moc_ on "qplatformplugin.h". This unnecessary step can be avoided by exporting interfaces like QWebSelectMethod, QWebNotificationPresenter,..

[Arunprasad Rajkumar]

Created attachment 199047 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 199047 [details]
Patch

Platform plugins are, like their name saying, part of the platform implementation under QtWebKit.
Those APIs are not public and are made to be implemented by those deploying QtWebKit on a platform at compile time. They were not designed to allow per-application implementation.

If you need to add an implementation for your embedded device, I suggest doing so directly in your QtWebKit build to make sure that applications on your platform don't have to worry about it.

[Jocelyn Turcotte]

Comment on attachment 199047 [details]
Patch

No I'm wrong on this, the header is already shipped, let me think about it.
However you shouldn't remove the example file, what is the reason behind this?

[Arunprasad Rajkumar]

(In reply to comment #2)
> (From update of attachment 199047 [details])
> Platform plugins are, like their name saying, part of the platform implementation under QtWebKit.
> Those APIs are not public and are made to be implemented by those deploying QtWebKit on a platform at compile time. They were not designed to allow per-application implementation.
> 
> If you need to add an implementation for your embedded device, I suggest doing so directly in your QtWebKit build to make sure that applications on your platform don't have to worry about it.

Agreed :) But already QtWebkit library has the definition of all the _signals_ of Plugin Interfaces. It will be good if we avoid the redundancy, (yes I agree it is trivial though!)

[Arunprasad Rajkumar]

(In reply to comment #3)
> (From update of attachment 199047 [details])
> No I'm wrong on this, the header is already shipped, let me think about it.
> However you shouldn't remove the example file, what is the reason behind this?

Actually the example has a duplicate file. It should use Api/qwebkitplatformplugin.h instead of having a one more copy :)

[Jocelyn Turcotte]

(In reply to comment #3)
> However you shouldn't remove the example file, what is the reason behind this?
I see that we had a copy of the header there, just noticed so that would be fine.

It seems like this was settled in the past to avoid the dependency of plugins to qwebkitglobal.h in bug #71759.

I don't see why plugins should avoid it while application can, but Simon might have an idea.

[Arunprasad Rajkumar]

Created attachment 199052 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #7)
> Created an attachment (id=199052) [details]
> Patch

Actually the patch had a issue, platformplugin.pro should add webkit into dependency module list, tats why I uploaded again, sorry.!!

I will remove review request until it is finalized,:)

[Arunprasad Rajkumar]

Created attachment 199905 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #9)
> Created an attachment (id=199905) [details]
> Patch

Added `parent` parameter to QWebKitPlatformPlugin::createExtension().

[Simon Hausmann]

I admit I don't understand this change. Can you help me understand? :)

I'll explain the intentions of the current design first:

qwebkitplatformplugin.h declares a set of pure interfaces. There are no symbols meant to be defined and exported by QtWebKit.dll that correspond to this interface. Instead the Qt interface cast mechanism is mean to provide a type-safe cast to the initial instance (QWebKitPlatformPlugin) and its versioning mechanism (that works on the basis of string comparision) makes it safe.

The header file is designed to be _copied_ into a use-case (or platform) specific plugin project that is deployed together with QtWebKit on the target platform.

[Jocelyn Turcotte]

(In reply to comment #11)
> The header file is designed to be _copied_ into a use-case (or platform) specific plugin project that is deployed together with QtWebKit on the target platform.

Maybe those headers shouldn't be installed then, is it necessary that they are? They shouldn't be in Source/WebKit/qt/Api.

[Arunprasad Rajkumar]

(In reply to comment #11)
> I admit I don't understand this change. Can you help me understand? :)
> 
> I'll explain the intentions of the current design first:
> 
> qwebkitplatformplugin.h declares a set of pure interfaces. There are no symbols meant to be defined and exported by QtWebKit.dll that correspond to this interface. Instead the Qt interface cast mechanism is mean to provide a type-safe cast to the initial instance (QWebKitPlatformPlugin) and its versioning mechanism (that works on the basis of string comparision) makes it safe.

Yes, I accept some of them are pure interfaces, but other interfaces like QWebSelectMethod has signals like selectItem, didHide; Its definition will be available in libQtWebKit.so, but it is not exported. In order to get the definition of those signals I have to _moc_ again & link the moc with my application.

> 
> The header file is designed to be _copied_ into a use-case (or platform) specific plugin project that is deployed together with QtWebKit on the target platform.

Why it has to be copied? It is already part of delivery(available as QtWebKit/QWebKitPlatformPlugin). So it should be usable as like other Qt plugin interfaces.

Intention of exporting interfaces is to use the signal definition which is already present in libQtWebKit.so and to pass _parent_ to plug-in.

[Simon Hausmann]

(In reply to comment #13)
> (In reply to comment #11)
> > I admit I don't understand this change. Can you help me understand? :)
> > 
> > I'll explain the intentions of the current design first:
> > 
> > qwebkitplatformplugin.h declares a set of pure interfaces. There are no symbols meant to be defined and exported by QtWebKit.dll that correspond to this interface. Instead the Qt interface cast mechanism is mean to provide a type-safe cast to the initial instance (QWebKitPlatformPlugin) and its versioning mechanism (that works on the basis of string comparision) makes it safe.
> 
> Yes, I accept some of them are pure interfaces, but other interfaces like QWebSelectMethod has signals like selectItem, didHide; Its definition will be available in libQtWebKit.so, but it is not exported. In order to get the definition of those signals I have to _moc_ again & link the moc with my application.
> 
> > 
> > The header file is designed to be _copied_ into a use-case (or platform) specific plugin project that is deployed together with QtWebKit on the target platform.
> 
> Why it has to be copied? It is already part of delivery(available as QtWebKit/QWebKitPlatformPlugin). So it should be usable as like other Qt plugin interfaces.
> 
> Intention of exporting interfaces is to use the signal definition which is already present in libQtWebKit.so and to pass _parent_ to plug-in.

The problem with exporting symbols is that then they are subject to binary compatibility constraints. The pure interface approach doesn't require that. (Is adding the copied header file to HEADERS so that moc runs again very difficult?)

[Arunprasad Rajkumar]

(In reply to comment #14)
> The problem with exporting symbols is that then they are subject to binary compatibility constraints. The pure interface approach doesn't require that. 

If I understood correctly, as like other Qt plugins(QPA,imageformats,..) qtwebkitplatformplugin is not binary compatible between Qt deliveries. Suppose if I have a qtwebkitplatformplugin plugin which is compiled against 1.0, the same plugin can't be used as is for 2.0. Am I right? That is the case, I agree with u :), but qwebkitplatformplugin.h shouldn't be a part of Qt delivery.


>(Is adding the copied header file to HEADERS so that moc runs again very difficult?)

Running _moc_ again is not a problem :)



One more query, what about passing the parent to QtWebKitPlatformPlugin::createExtension? because the plugin may need the QObject who asks to create extensions.

[Arunprasad Rajkumar]

(In reply to comment #15)
> (In reply to comment #14)
> > The problem with exporting symbols is that then they are subject to binary compatibility constraints. The pure interface approach doesn't require that. 
> 
> If I understood correctly, as like other Qt plugins(QPA,imageformats,..) qtwebkitplatformplugin is not binary compatible between Qt deliveries. Suppose if I have a qtwebkitplatformplugin plugin which is compiled against 1.0, the same plugin can't be used as is for 2.0. Am I right? That is the case, I agree with u :), but qwebkitplatformplugin.h shouldn't be a part of Qt delivery.
> 
> 

What about exporting & declaring a clear note like "It is not Binary compatible" as like other Qt plugins?(QPA,imageformats,..)

[Simon Hausmann]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > The problem with exporting symbols is that then they are subject to binary compatibility constraints. The pure interface approach doesn't require that. 
> > 
> > If I understood correctly, as like other Qt plugins(QPA,imageformats,..) qtwebkitplatformplugin is not binary compatible between Qt deliveries. Suppose if I have a qtwebkitplatformplugin plugin which is compiled against 1.0, the same plugin can't be used as is for 2.0. Am I right? That is the case, I agree with u :), but qwebkitplatformplugin.h shouldn't be a part of Qt delivery.
> > 
> > 
> 
> What about exporting & declaring a clear note like "It is not Binary compatible" as like other Qt plugins?(QPA,imageformats,..)

Then you still get crashes when QtWebKit tries to load a platform plugin that was compiled against the wrong version of the interface.

The crash is what the interface cast and versioning is supposed to protect us against.

If we're willing to give that up, then we could export symbols and actual base classes.

[Arunprasad Rajkumar]

(In reply to comment #17)
> Then you still get crashes when QtWebKit tries to load a platform plugin that was compiled against the wrong version of the interface.
> 
> The crash is what the interface cast and versioning is supposed to protect us against.

m_plugin = qobject_cast<QWebKitPlatformPlugin*>(obj);

qobject_cast will do it right?(specialized version for Q_DECLARE_INTERFACE)

> 
> If we're willing to give that up, then we could export symbols and actual base classes.

You guys have to decide :), I'm always +1 for exporting symbols.

I also added parent parameter to createExtension like below,

virtual QObject* createExtension(Extension, QObject* parent = 0) const = 0;. 

What do you think?

[Simon Hausmann]

(In reply to comment #18)
> (In reply to comment #17)
> > Then you still get crashes when QtWebKit tries to load a platform plugin that was compiled against the wrong version of the interface.
> > 
> > The crash is what the interface cast and versioning is supposed to protect us against.
> 
> m_plugin = qobject_cast<QWebKitPlatformPlugin*>(obj);
> 
> qobject_cast will do it right?(specialized version for Q_DECLARE_INTERFACE)

Yes, exactly.
 
> > 
> > If we're willing to give that up, then we could export symbols and actual base classes.
> 
> You guys have to decide :), I'm always +1 for exporting symbols.
> 
> I also added parent parameter to createExtension like below,
> 
> virtual QObject* createExtension(Extension, QObject* parent = 0) const = 0;. 
> 
> What do you think?

Can you explain why you would like to add the parent? (the ChangeLog does not mention why)

[Arunprasad Rajkumar]

(In reply to comment #19)
> > I also added parent parameter to createExtension like below,
> > 
> > virtual QObject* createExtension(Extension, QObject* parent = 0) const = 0;. 
> > 
> > What do you think?
> 
> Can you explain why you would like to add the parent? (the ChangeLog does not mention why)

Sorry :(, I will describe it in ChangeLog. The need for parent may have more usecases.But from my view,
we can use _parent_ while creating any new widgets from plugin(if we didn't specify the widget will be created in new window), not only for this, objects lifetime can be associated with _parent_ so that destruction can be automated.

(Sample usecase, while creating a MultipleSelections widget using via plugin, we can assign the Popup's parent as _parent_ so that it won't need a separate window to show it)

_parent_ - denotes the parent passed as a parameter to QWebKitPlatformPlugin::createExtension()

[Arunprasad Rajkumar]

Created attachment 200953 [details]
Patch

[Arunprasad Rajkumar]

Hi Simon, 

I updated patch with proper comments, please review it.

-Arun

[Arunprasad Rajkumar]

Simon, Can you please review it?

[Anders Carlsson]

Comment on attachment 200953 [details]
Patch

Qt has been removed, clearing review flags.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.