Bug 114966 - [Qt] QWebKitPlatformPlugin should export available Plugin interfaces
Summary: [Qt] QWebKitPlatformPlugin should export available Plugin interfaces
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-22 10:38 PDT by Arunprasad Rajkumar
Modified: 2014-02-03 03:25 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.74 KB, patch)
2013-04-22 10:44 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (9.92 KB, patch)
2013-04-22 11:26 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (18.48 KB, patch)
2013-04-27 07:04 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2013-05-07 11:49 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arunprasad Rajkumar 2013-04-22 10:38:07 PDT
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,..
Comment 1 Arunprasad Rajkumar 2013-04-22 10:44:15 PDT
Created attachment 199047 [details]
Patch
Comment 2 Jocelyn Turcotte 2013-04-22 11:06:24 PDT
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.
Comment 3 Jocelyn Turcotte 2013-04-22 11:15:34 PDT
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?
Comment 4 Arunprasad Rajkumar 2013-04-22 11:19:52 PDT
(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!)
Comment 5 Arunprasad Rajkumar 2013-04-22 11:22:43 PDT
(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 :)
Comment 6 Jocelyn Turcotte 2013-04-22 11:26:03 PDT
(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.
Comment 7 Arunprasad Rajkumar 2013-04-22 11:26:38 PDT
Created attachment 199052 [details]
Patch
Comment 8 Arunprasad Rajkumar 2013-04-22 11:29:08 PDT
(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,:)
Comment 9 Arunprasad Rajkumar 2013-04-27 07:04:27 PDT
Created attachment 199905 [details]
Patch
Comment 10 Arunprasad Rajkumar 2013-04-27 07:06:42 PDT
(In reply to comment #9)
> Created an attachment (id=199905) [details]
> Patch

Added `parent` parameter to QWebKitPlatformPlugin::createExtension().
Comment 11 Simon Hausmann 2013-05-02 01:51:57 PDT
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.
Comment 12 Jocelyn Turcotte 2013-05-02 02:04:37 PDT
(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.
Comment 13 Arunprasad Rajkumar 2013-05-02 02:13:47 PDT
(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.
Comment 14 Simon Hausmann 2013-05-02 03:49:50 PDT
(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?)
Comment 15 Arunprasad Rajkumar 2013-05-02 04:57:15 PDT
(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.
Comment 16 Arunprasad Rajkumar 2013-05-02 05:21:24 PDT
(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,..)
Comment 17 Simon Hausmann 2013-05-03 01:18:33 PDT
(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.
Comment 18 Arunprasad Rajkumar 2013-05-03 01:36:22 PDT
(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?
Comment 19 Simon Hausmann 2013-05-07 00:34:09 PDT
(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)
Comment 20 Arunprasad Rajkumar 2013-05-07 02:16:31 PDT
(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()
Comment 21 Arunprasad Rajkumar 2013-05-07 11:49:27 PDT
Created attachment 200953 [details]
Patch
Comment 22 Arunprasad Rajkumar 2013-05-18 13:40:51 PDT
Hi Simon, 

I updated patch with proper comments, please review it.

-Arun
Comment 23 Arunprasad Rajkumar 2013-07-12 00:17:11 PDT
Simon, Can you please review it?
Comment 24 Anders Carlsson 2013-10-02 21:31:34 PDT
Comment on attachment 200953 [details]
Patch

Qt has been removed, clearing review flags.
Comment 25 Jocelyn Turcotte 2014-02-03 03:25:43 PST
=== 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.