Bug 49343 – Use qobject_cast instead of static_cast when using the platform plugin extensions

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Andre Pedralho]

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;
}

[Andre Pedralho]

Created attachment 73540 [details]
Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin.

[Andre Pedralho]

Adding some possible reviewers.

[Antonio Gomes]

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.

[Luiz Agostini]

(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?

[Andre Pedralho]

(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.

[Antonio Gomes]

(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 :)

[Luiz Agostini]

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.

[Luiz Agostini]

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.

[Andre Pedralho]

(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!

[Yael]

This patch should bump the version of the plugin.

[Andre Pedralho]

Created attachment 74555 [details]
Increasing platform plugin version and updating to TT.

[Luiz Agostini]

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.

[Andre Pedralho]

Created attachment 75266 [details]
Deleting extension when cast fails and warning user about it.

[Andre Pedralho]

Kenneth, could you give your opinion here, please?

[Brent Fulgham]

QT WebKit is no longer part of this project. We also no longer support Plugins in the engine.