Bug 38438

Created attachment 54885 [details] patch 1

Created attachment 54886 [details] usage example

(In reply to comment #2) > Created an attachment (id=54886) [details] > usage example This example is essentially what we have done for maemo. Just build it and it will create a plugin and put it in Qt plugins directory. The plugin will than be automatically used by QtWebKit. This plugin will replace all select popups by QDialogs. To use the plugin for <select multiple> elements include the following line to WebCore.pro and make a clean build of QtWebKit. DEFINES += ENABLE_NO_LISTBOX_RENDERING=1

I'd like to avoid using the term input method for this work, as it is generally used for text input methods (across toolkits and platforms). I am on the other hand quite fond of the term platform plugin :)

Comment on attachment 54885 [details] patch 1 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!

I actually find it to be some kind of input handling, instead of purely UI, and you can implement a plugin without any UI whatsoever. What about SelectMethod?

> 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? How does that sit with layering?

Created attachment 54903 [details] patch 2

Created attachment 54904 [details] usage example

(In reply to comment #5) > (From update of attachment 54885 [details]) > > WebCore/ChangeLog:5 > + [Qt] Select input method plugin > Like I wrote in the earlier comment, I suggest to use platform plugin in the > title. done. > > 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" :) done. > > WebKit/qt/Api/qwebkitplatformplugininterface.h:33 > + public: > Please add a virtual inline destructor here, to avoid annoying gcc compiler > warnings. done. > > 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. done. is that what you mean? > > 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" ? :) sure :). done. > > WebKit/qt/WebCoreSupport/QtPlatformPlugin.cpp:35 > + SelectData(QtAbstractWebPopup* data) : d(data) {} > I guess QWebSelectData and QAbstractWebPopup could be merged in the future? I think that it could be a layering problem. > > 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()? no good reason. changed. > > 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 :) done. > > 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... done. > > 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? Again I think that it could be a layreing problem. I came to this solution to avoid to use in WebKit/qt/API files that are implemented in WebCore/platform/qt. > > > 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! Good comments. Thanks. :)

Created attachment 54977 [details] orbit usage example This is a very simple plugin example using orbit. It does not handle multiple selections.

(In reply to comment #11) > Created an attachment (id=54977) [details] > orbit usage example > > This is a very simple plugin example using orbit. It does not handle multiple > selections. I did not look at the code very closely, but wanted to give some quick feedback: 1. The Popup class should inherit HbDialog or HbListDialog but not QDialog. 2. Enabling the mutli-select list is a main use-case, I don't think it should be treated as an extension. IMO, The same API can be used for both single-select and multi-select. Just set a flag indicating if the dialog should allow multi-select or not. 3. Do we want to allow using control modifier for multi-select? The current proposal only allows shift modifier.

(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=54977) [details] [details] > > orbit usage example > > > > This is a very simple plugin example using orbit. It does not handle multiple > > selections. > > I did not look at the code very closely, but wanted to give some quick > feedback: > 1. The Popup class should inherit HbDialog or HbListDialog but not QDialog. > 2. Enabling the mutli-select list is a main use-case, I don't think it should > be treated as an extension. IMO, The same API can be used for both > single-select and multi-select. Just set a flag indicating if the dialog should > allow multi-select or not. > 3. Do we want to allow using control modifier for multi-select? The current > proposal only allows shift modifier. The intention here is just to provide simple examples of plugins. The focus is in the plugin support by QtWebKit and not in the plugins examples themselves. As I have never been using orbit it would be hard to me to provide more elaborated examples.

(In reply to comment #13) > The intention here is just to provide simple examples of plugins. The focus is > in the plugin support by QtWebKit and not in the plugins examples themselves. > As I have never been using orbit it would be hard to me to provide more > elaborated examples. Only the first comment is related to Orbit :-)

(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=54977) [details] [details] > > orbit usage example > > > > This is a very simple plugin example using orbit. It does not handle multiple > > selections. > > I did not look at the code very closely, but wanted to give some quick > feedback: > 1. The Popup class should inherit HbDialog or HbListDialog but not QDialog. I can't say much about orbit. :) > 2. Enabling the mutli-select list is a main use-case, I don't think it should > be treated as an extension. IMO, The same API can be used for both > single-select and multi-select. Just set a flag indicating if the dialog should > allow multi-select or not. It is possible to provide multi-select lists. Actually the first usage example provides multi-select lists. The flag that indicates if the dialog should allow multi-select is QWebSelectData::multiple(). > 3. Do we want to allow using control modifier for multi-select? The current > proposal only allows shift modifier. control modifier is there in QWebSelectMethod:: selectItem(). it is named allowMultiplySelections. We came to this name on the review proccess of previous patches. :)

(In reply to comment #14) > (In reply to comment #13) > > The intention here is just to provide simple examples of plugins. The focus is > > in the plugin support by QtWebKit and not in the plugins examples themselves. > > As I have never been using orbit it would be hard to me to provide more > > elaborated examples. > > Only the first comment is related to Orbit :-) now I see. :-) thanks

One comment about QWebKitPlatformPlugin::supportsExtension() : This method is not used by QtWebKit at the moment. It was included in plugin's interface by Kenneth's suggestion to avoid changing the plugin version when the support for it is enabled in future. Today, to use or not multi-select popus is a compile time decision. if ENABLE(NO_LISTBOX_RENDERING) is true at compile time all <select> elements, multiple or not, will be rendered as combo boxes and will rely on the plugins to provide the popups.

I assume that "selectItem" is used both for selection and de-selection of items in a multi-select list? One more Symbian specific comment is that we will need to rework the way plugins are loaded. Take a look at PluginDatabaseSymbian.cpp s an example.

(In reply to comment #18) > I assume that "selectItem" is used both for selection and de-selection of items > in a multi-select list? Yes, you are right. > > One more Symbian specific comment is that we will need to rework the way > plugins are loaded. Take a look at PluginDatabaseSymbian.cpp s an example. I am using QCoreApplication::libraryPaths() to look for the plugins. It should work in Symbian as well. I probably did not understand your point. :)

(In reply to comment #19) > I probably did not understand your point. :) The point was that QPluginLoader on Symbian needs some quirks in order to work properly :-)

Comment on attachment 54903 [details] patch 2 WebKit/qt/Api/qwebkitplatformplugin.h:34 + ~QWebSelectData() {} Please add the inline keyword before landing. WebKit/qt/Api/qwebkitplatformplugin.h:51 + ~QWebSelectMethod() {} Ditto :) I think the patch looks good otherwise. My only other concern would be that this slows down the startup time. I think it would be better if the plugin was loaded on-demand. That can be done in a follow-up patch.

(In reply to comment #21) > (From update of attachment 54903 [details]) > WebKit/qt/Api/qwebkitplatformplugin.h:34 > + ~QWebSelectData() {} > Please add the inline keyword before landing. > > WebKit/qt/Api/qwebkitplatformplugin.h:51 > + ~QWebSelectMethod() {} > Ditto :) Ok. > > I think the patch looks good otherwise. My only other concern would be that > this slows down the startup time. I think it would be better if the plugin was > loaded on-demand. That can be done in a follow-up patch. The plugin is loaded on-demand in this implementation.

Created attachment 55407 [details] patch 3

(In reply to comment #20) > (In reply to comment #19) > > I probably did not understand your point. :) > The point was that QPluginLoader on Symbian needs some quirks in order to work > properly :-) I see. Please let me know if you have problems loading plugins in Symbian.

(In reply to comment #22) > > I think the patch looks good otherwise. My only other concern would be that > > this slows down the startup time. I think it would be better if the plugin was > > loaded on-demand. That can be done in a follow-up patch. > > The plugin is loaded on-demand in this implementation. You're right, I overlooked that. Great stuff, Luiz :)

Comment on attachment 55407 [details] patch 3 Clearing flags on attachment: 55407 Committed r59033: <http://trac.webkit.org/changeset/59033>

All reviewed patches have been landed. Closing bug.

Is this really meant to go into the 2.0 release branch? It seems like a new feature to me, and I thought the resulting work is scheduled for >=2.1 ?

According to the latest pla on record, this is no longer needed for 2.0 - removing dependency for 2.0 bug.

(In reply to comment #28) > Is this really meant to go into the 2.0 release branch? > > It seems like a new feature to me, and I thought the resulting work is scheduled for >=2.1 ? 4baa866d2216e344e1134be1861902838609956b depends on patch 1. If this doesn't go into the 2.0 release branch, the commit should be updated.