Bug 49343

Summary: Use qobject_cast instead of static_cast when using the platform plugin extensions
Product: WebKit Reporter: Andre Pedralho <andre.pedralho>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: andre.pedralho, bfulgham, cmarcelo, kenneth, luiz, tonikitoo, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin.
none
Increasing platform plugin version and updating to TT.
none
Deleting extension when cast fails and warning user about it. none

Andre Pedralho
Reported 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; }
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
Increasing platform plugin version and updating to TT. (7.32 KB, patch)
2010-11-22 07:56 PST, Andre Pedralho
no flags
Deleting extension when cast fails and warning user about it. (9.73 KB, patch)
2010-12-01 05:16 PST, Andre Pedralho
no flags
Andre Pedralho
Comment 1 2010-11-10 14:24:56 PST
Created attachment 73540 [details] Adding INTERFACE macros in the plugin extensions and using qobject_cast in QtPlatformPlugin.
Andre Pedralho
Comment 2 2010-11-10 14:29:19 PST
Adding some possible reviewers.
Antonio Gomes
Comment 3 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.
Luiz Agostini
Comment 4 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?
Andre Pedralho
Comment 5 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.
Antonio Gomes
Comment 6 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 :)
Luiz Agostini
Comment 7 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.
Luiz Agostini
Comment 8 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.
Andre Pedralho
Comment 9 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!
Yael
Comment 10 2010-11-22 07:08:22 PST
This patch should bump the version of the plugin.
Andre Pedralho
Comment 11 2010-11-22 07:56:47 PST
Created attachment 74555 [details] Increasing platform plugin version and updating to TT.
Luiz Agostini
Comment 12 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.
Andre Pedralho
Comment 13 2010-12-01 05:16:39 PST
Created attachment 75266 [details] Deleting extension when cast fails and warning user about it.
Andre Pedralho
Comment 14 2010-12-22 15:46:46 PST
Kenneth, could you give your opinion here, please?
Brent Fulgham
Comment 15 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.
Note You need to log in before you can comment on or make changes to this bug.