Bug 38438 - [Qt] Platform plugin
Summary: [Qt] Platform plugin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Luiz Agostini
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-05-02 09:12 PDT by Luiz Agostini
Modified: 2010-06-14 08:40 PDT (History)
6 users (show)

See Also:


Attachments
patch 1 (15.86 KB, patch)
2010-05-02 10:06 PDT, Luiz Agostini
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
usage example (3.03 KB, application/octet-stream)
2010-05-02 10:10 PDT, Luiz Agostini
no flags Details
patch 2 (16.33 KB, patch)
2010-05-02 21:10 PDT, Luiz Agostini
hausmann: review+
hausmann: commit-queue-
Details | Formatted Diff | Diff
usage example (3.06 KB, application/octet-stream)
2010-05-02 21:12 PDT, Luiz Agostini
no flags Details
orbit usage example (2.49 KB, application/octet-stream)
2010-05-03 17:09 PDT, Luiz Agostini
no flags Details
patch 3 (16.36 KB, patch)
2010-05-07 12:34 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2010-05-02 09:12:30 PDT
Select input method private API.
Comment 1 Luiz Agostini 2010-05-02 10:06:41 PDT
Created attachment 54885 [details]
patch 1
Comment 2 Luiz Agostini 2010-05-02 10:10:25 PDT
Created attachment 54886 [details]
usage example
Comment 3 Luiz Agostini 2010-05-02 10:17:54 PDT
(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
Comment 4 Simon Hausmann 2010-05-02 13:34:35 PDT
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 5 Simon Hausmann 2010-05-02 13:48:37 PDT
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!
Comment 6 Kenneth Rohde Christiansen 2010-05-02 14:45:10 PDT
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?
Comment 7 Kenneth Rohde Christiansen 2010-05-02 14:47:08 PDT
> 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?
Comment 8 Luiz Agostini 2010-05-02 21:10:35 PDT
Created attachment 54903 [details]
patch 2
Comment 9 Luiz Agostini 2010-05-02 21:12:36 PDT
Created attachment 54904 [details]
usage example
Comment 10 Luiz Agostini 2010-05-02 21:48:18 PDT
(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. :)
Comment 11 Luiz Agostini 2010-05-03 17:09:49 PDT
Created attachment 54977 [details]
orbit usage example

This is a very simple plugin example using orbit. It does not handle multiple selections.
Comment 12 Yael 2010-05-04 10:57:17 PDT
(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.
Comment 13 Luiz Agostini 2010-05-05 10:11:05 PDT
(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.
Comment 14 Yael 2010-05-05 15:23:13 PDT
(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 :-)
Comment 15 Luiz Agostini 2010-05-05 15:42:36 PDT
(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. :)
Comment 16 Luiz Agostini 2010-05-05 15:43:51 PDT
(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
Comment 17 Luiz Agostini 2010-05-05 15:56:50 PDT
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.
Comment 18 Yael 2010-05-06 05:04:57 PDT
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.
Comment 19 Luiz Agostini 2010-05-07 08:20:28 PDT
(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. :)
Comment 20 Yael 2010-05-07 09:37:44 PDT
(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 21 Simon Hausmann 2010-05-07 10:02:07 PDT
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.
Comment 22 Luiz Agostini 2010-05-07 11:52:49 PDT
(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.
Comment 23 Luiz Agostini 2010-05-07 12:34:06 PDT
Created attachment 55407 [details]
patch 3
Comment 24 Luiz Agostini 2010-05-07 12:40:04 PDT
(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.
Comment 25 Simon Hausmann 2010-05-07 15:53:41 PDT
(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 26 WebKit Commit Bot 2010-05-08 11:51:59 PDT
Comment on attachment 55407 [details]
patch 3

Clearing flags on attachment: 55407

Committed r59033: <http://trac.webkit.org/changeset/59033>
Comment 27 WebKit Commit Bot 2010-05-08 11:52:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Simon Hausmann 2010-05-21 01:16:40 PDT
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 ?
Comment 29 Laszlo Gombos 2010-05-21 05:00:09 PDT
According to the latest pla on record, this is no longer needed for 2.0 - removing dependency for 2.0 bug.
Comment 30 Tasuku Suzuki 2010-06-14 08:40:08 PDT
(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.