Bug 49343 - Use qobject_cast instead of static_cast when using the platform plugin extensions
Summary: Use qobject_cast instead of static_cast when using the platform plugin extens...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 14:19 PST by Andre Pedralho
Modified: 2022-07-01 10:43 PDT (History)
7 users (show)

See Also:


Attachments
Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin. (6.70 KB, patch)
2010-11-10 14:24 PST, Andre Pedralho
no flags Details | Formatted Diff | Diff
Increasing platform plugin version and updating to TT. (7.32 KB, patch)
2010-11-22 07:56 PST, Andre Pedralho
no flags Details | Formatted Diff | Diff
Deleting extension when cast fails and warning user about it. (9.73 KB, patch)
2010-12-01 05:16 PST, Andre Pedralho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Pedralho 2010-11-10 14:19:21 PST
In WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp we are casting the plugin extensions using static_cast and the docs recommends to use qobject_cast.

For example:

QWebTouchModifier* QtPlatformPlugin::createTouchModifier()
{
    QWebKitPlatformPlugin* p = plugin();
-    return p ? static_cast<QWebTouchModifier*>(p->createExtension(QWebKitPlatformPlugin::TouchInteraction)) : 0;
+    return p ? qobject_cast<QWebTouchModifier*>(p->createExtension(QWebKitPlatformPlugin::TouchInteraction)) : 0;
}
Comment 1 Andre Pedralho 2010-11-10 14:24:56 PST
Created attachment 73540 [details]
Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin.
Comment 2 Andre Pedralho 2010-11-10 14:29:19 PST
Adding some possible reviewers.
Comment 3 Antonio Gomes 2010-11-10 20:56:02 PST
It looks ok to me.

Luiz told me it was the right thing to do, but I would like him to confirm before reviewing.

NOTE that qobject_cast is slower than static_cast, and if we now the casting wont fail, the later is recommended.
Comment 4 Luiz Agostini 2010-11-11 14:10:19 PST
(In reply to comment #3)
> It looks ok to me.
> 
> Luiz told me it was the right thing to do, but I would like him to confirm before reviewing.
> 
> NOTE that qobject_cast is slower than static_cast, and if we now the casting wont fail, the later is recommended.

The interface implemented by the root object of the plugin is declared in qwebkitplatformplugin.h and qobject_cast is used to be sure that the plugin is producing an object of the expected type. This way the version is checked for the plugin as a whole but not for each object returned by the factory QWebKitPlatformPlugin::createExtension().

I think taht we could consider that those static_casts will not fail. And I think that we should change the plugin version anyway if any of the interfaces changes.

At the end I do not see any advantage and the casts will be slower. Is this change really needed? What is the motivation for this?
Comment 5 Andre Pedralho 2010-11-12 07:27:39 PST
(In reply to comment #4)
> (In reply to comment #3)

Ok! That's true! I see that the static_cast will not fail and it is faster... My motivation to do those changes were in the docs that say:

Making an application extensible through plugins involves the following steps:

   1. Define a set of interfaces (classes with only pure virtual functions) used to talk to the plugins.
   2. Use the Q_DECLARE_INTERFACE() macro to tell Qt's meta-object system about the interface.
   3. Use QPluginLoader in the application to load the plugins.
   4. Use qobject_cast() to test whether a plugin implements a given interface.

But that's ok for me to mark this bug as invalid as we are considering one single plugin (that follows this model) and is extended in other small pieces.
Comment 6 Antonio Gomes 2010-11-12 08:07:55 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> 
> Ok! That's true! I see that the static_cast will not fail and it is faster... My motivation to do those changes were in the docs that say:
> 
> Making an application extensible through plugins involves the following steps:
> 
>    1. Define a set of interfaces (classes with only pure virtual functions) used to talk to the plugins.
>    2. Use the Q_DECLARE_INTERFACE() macro to tell Qt's meta-object system about the interface.
>    3. Use QPluginLoader in the application to load the plugins.
>    4. Use qobject_cast() to test whether a plugin implements a given interface.
> 
> But that's ok for me to mark this bug as invalid as we are considering one single plugin (that follows this model) and is extended in other small pieces.

At least we have to fix the docs :)
Comment 7 Luiz Agostini 2010-11-21 20:38:05 PST
Comment on attachment 73540 [details]
Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin.

View in context: https://bugs.webkit.org/attachment.cgi?id=73540&action=review

I think that it may make sense. Platform plugin provides many features and the number of features will probably increase in near future. Then it would probably be nice if, after changing one of the interfaces, QtWebKit could still use the unchanged features of the older plugins. For it we need to have version information of each interface.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:101
> +    return p ? qobject_cast<QWebNotificationPresenter*>(p->createExtension(QWebKitPlatformPlugin::Notifications)) : 0;

This cast may fail if the plugin implements an older version of the interface. In this case the object returned by createExtension(), if any, should be destroyed.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:107
> +    return p ? qobject_cast<QWebHapticFeedbackPlayer*>(p->createExtension(QWebKitPlatformPlugin::Haptics)) : 0;

The same here.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:113
> +    return p ? qobject_cast<QWebTouchModifier*>(p->createExtension(QWebKitPlatformPlugin::TouchInteraction)) : 0;

The same here.
Comment 8 Luiz Agostini 2010-11-21 22:34:38 PST
And all the currently implemented plugins will stop working once this patch gets in. They all will need to be changed to add Q_INTERFACES to the classes.
Comment 9 Andre Pedralho 2010-11-22 06:10:31 PST
(In reply to comment #8)
> And all the currently implemented plugins will stop working once this patch gets in. They all will need to be changed to add Q_INTERFACES to the classes.

Sooner we land it fewer implementations have to change. Is there a procedure to warn all the implementers about those changes?

BTW, nice arguments in favor of it Agostini!
Comment 10 Yael 2010-11-22 07:08:22 PST
This patch should bump the version of the plugin.
Comment 11 Andre Pedralho 2010-11-22 07:56:47 PST
Created attachment 74555 [details]
Increasing platform plugin version and updating to TT.
Comment 12 Luiz Agostini 2010-11-22 09:24:34 PST
Comment on attachment 74555 [details]
Increasing platform plugin version and updating to TT.

View in context: https://bugs.webkit.org/attachment.cgi?id=74555&action=review

I still not 100% sure that we should do this nor that we are allowed to create such incompatibility with previous implemented plugins. It would be nice to have some other opinions.
Anyway, please look at these issues.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:108
> +    return p ? qobject_cast<QWebSelectMethod*>(p->createExtension(QWebKitPlatformPlugin::MultipleSelections)) : 0;

If the cast fail then the object returned by createExtension() must be destroyed. As it is now the return of createExtension() is leaking when the cast fail.
It would be nice to give to user or programmer a hint of why a certain feature is not working. Maybe we should use qWarning for that.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:115
> +    return p ? qobject_cast<QWebNotificationPresenter*>(p->createExtension(QWebKitPlatformPlugin::Notifications)) : 0;

The same here.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:121
> +    return p ? qobject_cast<QWebHapticFeedbackPlayer*>(p->createExtension(QWebKitPlatformPlugin::Haptics)) : 0;

The same here.

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:127
> +    return p ? qobject_cast<QWebTouchModifier*>(p->createExtension(QWebKitPlatformPlugin::TouchInteraction)) : 0;

The same here.
Comment 13 Andre Pedralho 2010-12-01 05:16:39 PST
Created attachment 75266 [details]
Deleting extension when cast fails and warning user about it.
Comment 14 Andre Pedralho 2010-12-22 15:46:46 PST
Kenneth, could you give your opinion here, please?
Comment 15 Brent Fulgham 2022-07-01 10:43:59 PDT
QT WebKit is no longer part of this project. We also no longer support Plugins in the engine.