[webkit-reviews] review denied: [Bug 38438] [Qt] Select input method plugin : [Attachment 54885] patch 1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 2 13:48:37 PDT 2010


Simon Hausmann <hausmann at webkit.org> has denied Luiz Agostini
<luiz.agostini at openbossa.org>'s request for review:
Bug 38438: [Qt] Select input method plugin
https://bugs.webkit.org/show_bug.cgi?id=38438

Attachment 54885: patch 1
https://bugs.webkit.org/attachment.cgi?id=54885&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>

WebCore/ChangeLog:5
 +	    [Qt] Select input method plugin
Like I wrote in the earlier comment, I suggest to use platform plugin in the
title.

WebKit/qt/Api/qwebkitplatformplugininterface.h:25
 +   *	Warning: The contents of this file is not  part of the public Qt API
"part of the public Qt API" -> "QtWebKit API" :)

WebKit/qt/Api/qwebkitplatformplugininterface.h:33
 +  public:
Please add a virtual inline destructor here, to avoid annoying gcc compiler
warnings.

WebKit/qt/Api/qwebkitplatformplugininterface.h:62
 +	virtual bool handlesMultipleSelections() = 0;
I don't mind this for now, but in the long run I suggest to replace one boolean
method per "feature" in the platform plugin with a "supportsExtension" pattern
with an enum. See qwebpage.h.

WebKit/qt/Api/qwebkitplatformplugininterface.h:65
 +  Q_DECLARE_INTERFACE(QWebKitPlatformPluginInterface,
"com.nokia.Qt.WebKit.QWebKitPlatformPluginInterface/1.0");
Why duplicate WebKit in "Qt.WebKit.QWebKitPlatformPlugin" instead of simply
"Qt.WebKit.PlatformPlugin" ? :)

WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:35
 +	SelectData(QtAbstractWebPopup* data) : d(data) {}
I guess QWebSelectData and QAbstractWebPopup could be merged in the future?

WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:119
 +	    obj->deleteLater();
deleteLater() doesn't sound right to me. I think if the plugin doesn't fit we
should delete it immediately. Any reason for the deleteLater()?

WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:124
 +  static QWebKitPlatformPluginInterface* plugin()
There's a lot of nesting of helper functions here. Why not combine them all
into one single method? It's just a few lines of code :)

WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:126
 +	static QWebKitPlatformPluginInterface* result = getPluginInterface();
I don't like that the plugin object is never deleted. WebKit currently isn't
really made for unloading and loading as a plugin, but on the other hand I
think we should give the plugin a chance for destruction...

WebKit/qt/WebCoreSupport/QtPlatformPlugin.h:31
 +  class SelectInputMethodWrapper : public QObject, public QtAbstractWebPopup
{
There's a lot of abstraction and wrapping here. Wouldn't it be simpler to have
one interface towards WebCore only, the one that we export and also implement
ourselves as fallback?


I'm going to say r- Luiz, but mostly because I think this needs a few more
cleanups. In general this looks like a very good start!


More information about the webkit-reviews mailing list