Bug 33418 – [Qt] Custom select popups.

• Component: Platform
• Status: RESOLVED WONTFIX

[Luiz Agostini]

Custom select popup factories.
Usage example in QGVLauncher.

[Kenneth Rohde Christiansen]

Where are the attachments, Luiz?

[Luiz Agostini]

Created attachment 46206 [details]
Custom popups, popup factories and usage example

[WebKit Review Bot]

Attachment 46206 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/176377

[WebKit Review Bot]

Attachment 46206 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/QGVLauncher/main.cpp:35:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/QGVLauncher/custompopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/QGVLauncher/custompopup.cpp:115:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/qt/Api/qwebpopup.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.h:33:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/qt/Api/qwebpage.cpp:1753:  qt_qwebpage_setPopupFactory is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 9

[Luiz Agostini]

Build bot probably is not using Qt 4.6. I used animations in QGVLauncher. This seems to be the reason for the compilation error.

[Kenneth Rohde Christiansen]

I guess you should leave the usage example as a separate commit, and guard it with version checks for Qt 4.6

[Kenneth Rohde Christiansen]

+void QWebPage::setPopupFactory(QWebPopupFactory* factory)
+{
+    d->chromeClient->m_popupFactory = factory;
+}
+
+void qt_qwebpage_setPopupFactory(QWebPage* page, QWebPopupFactory* factory)
+{
+    page->setPopupFactory(factory);
+}

If  you wanted to add this as  private API for now, this is not correct. the  QWebPage::setPopupFactory() cannot exist.

why not 

void qt_qwebpage_setPopupFactory(QWebPage* page, QWebPopupFactory* factory)
{
    d->chromeClient->m_popupFactory = factory;
}
??

Also the chromeClient->m_popupFactory is not acceptable. Please add a setter instead.

[Kenneth Rohde Christiansen]

What about merging your QWebPopupObserver with the QWebPopupPrivate?

[Luiz Agostini]

Created attachment 46208 [details]
Custom popups, popup factories and usage example.

[Luiz Agostini]

(In reply to comment #8)
> What about merging your QWebPopupObserver with the QWebPopupPrivate?

QWebPopupObserver is needed as a separated class.
Look at ChromeClientQt.cpp line 87.

[WebKit Review Bot]

Attachment 46208 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/QGVLauncher/main.cpp:35:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/QGVLauncher/custompopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/QGVLauncher/custompopup.cpp:127:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/qt/Api/qwebpopup.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/Api/qwebpage.cpp:1753:  qt_qwebpage_setPopupFactory is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 6

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> Attachment 46208 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/qt/QGVLauncher/main.cpp:35:  Found other header before WebCore config.h.
> Should be: config.h, primary header, blank line, and then alphabetically
> sorted.  [build/include_order] [4]

These errors in QGVLauncher can be ignored

> WebKit/qt/Api/qwebpopup.cpp:21:  Found header this file implements before
> WebCore config.h. Should be: config.h, primary header, blank line, and then
> alphabetically sorted.  [build/include_order] [4]

Pls fix

> WebKit/qt/QGVLauncher/custompopup.cpp:21:  Found header this file implements
> before WebCore config.h. Should be: config.h, primary header, blank line, and
> then alphabetically sorted.  [build/include_order] [4]

Ignore

> WebKit/qt/QGVLauncher/custompopup.cpp:127:  One line control clauses should not
> use braces.  [whitespace/braces] [4]

Please fix

> WebKit/qt/Api/qwebpopup.h:27:  Alphabetical sorting problem. 
> [build/include_order] [4]

fix

> WebKit/qt/Api/qwebpage.cpp:1753:  qt_qwebpage_setPopupFactory is incorrectly
> named. Don't use underscores in your identifier names.  [readability/naming]
> [4]

Ignore as we use this for private Qt API

[Luiz Agostini]

Created attachment 46209 [details]
Custom popups, popup factories and usage example.

[WebKit Review Bot]

Attachment 46209 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/QGVLauncher/main.cpp:35:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/Api/qwebpopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/QGVLauncher/custompopup.cpp:21:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit/qt/Api/qwebpage.cpp:1746:  qt_qwebpage_setPopupFactory is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 4

[Kenneth Rohde Christiansen]

+                     $$PWD/qwebpopup.h \
+                     $$PWD/qwebpopup_p.h

You need to make sure that neither of these are installed, as the API should be private for now.

[Luiz Agostini]

(In reply to comment #15)
> +                     $$PWD/qwebpopup.h \
> +                     $$PWD/qwebpopup_p.h
> 
> You need to make sure that neither of these are installed, as the API should be
> private for now.

How can I do that?

[Luiz Agostini]

(In reply to comment #12)
> (In reply to comment #11)
> > Attachment 46208 [details] [details] did not pass style-queue:
> > 
> > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> > WebKit/qt/QGVLauncher/main.cpp:35:  Found other header before WebCore config.h.
> > Should be: config.h, primary header, blank line, and then alphabetically
> > sorted.  [build/include_order] [4]
> 
> These errors in QGVLauncher can be ignored
> 
> > WebKit/qt/Api/qwebpopup.cpp:21:  Found header this file implements before
> > WebCore config.h. Should be: config.h, primary header, blank line, and then
> > alphabetically sorted.  [build/include_order] [4]
> 
> Pls fix
Should I include config.h in a file that is supposed to be used as public API in future?
> 
> > WebKit/qt/QGVLauncher/custompopup.cpp:21:  Found header this file implements
> > before WebCore config.h. Should be: config.h, primary header, blank line, and
> > then alphabetically sorted.  [build/include_order] [4]
> 
> Ignore
> 
> > WebKit/qt/QGVLauncher/custompopup.cpp:127:  One line control clauses should not
> > use braces.  [whitespace/braces] [4]
> 
> Please fix
> 
> > WebKit/qt/Api/qwebpopup.h:27:  Alphabetical sorting problem. 
> > [build/include_order] [4]
> 
> fix
> 
> > WebKit/qt/Api/qwebpage.cpp:1753:  qt_qwebpage_setPopupFactory is incorrectly
> > named. Don't use underscores in your identifier names.  [readability/naming]
> > [4]
> 
> Ignore as we use this for private Qt API

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> (In reply to comment #15)
> > +                     $$PWD/qwebpopup.h \
> > +                     $$PWD/qwebpopup_p.h
> > 
> > You need to make sure that neither of these are installed, as the API should be
> > private for now.
> 
> How can I do that?

I don't have the source at hand right now to have a look. I guess we can verify
that later.

Would it be possible to split the patch up into two? 

1) implementation of new API
2) example added to launcher

The patch is too big to review as it is right now.

[Adam Barth]

We should file bugs about the style false positives.  If one of you would like to do it, I suspect you'll be more accurate in describing what needs to change.  If not, I can take care of it.

[Kenneth Rohde Christiansen]

(In reply to comment #19)
> We should file bugs about the style false positives.  If one of you would like
> to do it, I suspect you'll be more accurate in describing what needs to change.
>  If not, I can take care of it.

Could you do that Luiz?

There are basically two issues:

1) Private Qt API starts with qt_ and uses underscores. So it would be nice to ignore the underscore rule for methods starting with qt_.

2) We do not need to include config.h in our applications (our two launchers, QtLauncher and QGVLauncher, which btw probably should be merged), not in our autotests. It would be nice to ignore this check for some specific subdirs.

[Simon Hausmann]

(In reply to comment #13)
> Created an attachment (id=46209) [details]
> Custom popups, popup factories and usage example.

Some comments:

* I don't understand why we're adding a public exported class but a private API to actually use it.

* I'm not particularly fond of the term QWebPopup. It suggests that this relates to popup menus in web sites (sometimes also called dhtml menus). Just brainstorming: How about QWebListSelectionDelegate (too long)?

* Public Qt classes usually don't have non-virtual protected methods, they belong into the d-pointer.

* It appears the parent of the popup should be passed to the constructor and at the time the factory is called. That would also reduce the number of virtual functions one has to re-implement.

* Why is there a difference between show() and hide()? Why not capture the delegation in one single function call? Something along the lines of

int selectedIndex = select(geometry, items, initialSelectedIndex);


I think a goal of this API should be that it requires minimal effort to implement. If all it takes is one method call, then perhaps a better frame for this would be a QWebUIDelegate of some sort, provided we can keep it easily and conviently extensible.

[Simon Hausmann]

Comment on attachment 46209 [details]
Custom popups, popup factories and usage example.

I'm going to say r- as we can't land it as-is. But that should not prevent us from having a thorough review of the API.

[Kenneth Rohde Christiansen]

> * I'm not particularly fond of the term QWebPopup. It suggests that this
> relates to popup menus in web sites (sometimes also called dhtml menus). Just
> brainstorming: How about QWebListSelectionDelegate (too long)?

Me neither, and that was not the name me and Luiz had discussed. On the other hand Selection is confusing as it sounds as we are selecting something. Select might be better. In WebCore sense it is called Popup as it is not inline in the page, which a select can be (for instance if I add the size property).

> I think a goal of this API should be that it requires minimal effort to
> implement. If all it takes is one method call, then perhaps a better frame for
> this would be a QWebUIDelegate of some sort, provided we can keep it easily and
> conviently extensible.

In the future we might need to pass some style info as font, indentation, etc.

[Luiz Agostini]

(In reply to comment #21)
> (In reply to comment #13)
> > Created an attachment (id=46209) [details] [details]
> > Custom popups, popup factories and usage example.
> 
> Some comments:
> 
> * I don't understand why we're adding a public exported class but a private API
> to actually use it.
If this is supposed to get in to 4.6.x release we need to make it private. Should I move the class to the *_p.h file until it can be made public?
> 
> * I'm not particularly fond of the term QWebPopup. It suggests that this
> relates to popup menus in web sites (sometimes also called dhtml menus). Just
> brainstorming: How about QWebListSelectionDelegate (too long)?
What about QWebSelectPopupDelegate. It says it is a popup related to a <select> html element.
> 
> * Public Qt classes usually don't have non-virtual protected methods, they
> belong into the d-pointer.
Those methods are there to be used by the derived classes, not to be implemented by them. Should the d-pointer be protected?
> 
> * It appears the parent of the popup should be passed to the constructor and at
> the time the factory is called. That would also reduce the number of virtual
> functions one has to re-implement.
The popup object is created for each combobox just when needed but it is kept alive until the page is unloaded. There could for example have many views showing the same scene. The parent must then be adjusted before showing the popup.
> 
> * Why is there a difference between show() and hide()? Why not capture the
> delegation in one single function call? Something along the lines of
> 
> int selectedIndex = select(geometry, items, initialSelectedIndex);
I think that this call must not be synchronous. I think it is not possible to block that thread while the user chooses an item in a combobox.
> 
> 
> I think a goal of this API should be that it requires minimal effort to
> implement. If all it takes is one method call, then perhaps a better frame for
> this would be a QWebUIDelegate of some sort, provided we can keep it easily and
> conviently extensible.

[Luiz Agostini]

Created attachment 46653 [details]
Custom popups and popup factories.


main changes in this patch:
   * make the API public.
   * change the name from QWebPopup to QWebSelectPopupDelegate.
   * remove the abstract method setParent and create a new non virtual method QWidget* parentWidget() in QWebSelectPopupDelegate. It can be useful for those who use QWidgets in their popup customizations but can be ignored by those that do not.
   * remove the usage example from the patch.

[Girish Ramakrishnan]

Some suggestions that I mentioned this on irc, pasting here for the record.

1. Rename populate to setItems
2. Rename setIndex to setSelectedIndex or setCurrentIndex?
3. show() should not ideally have a selectedIndex argument. Have a separate select() maybe?
4. didHide to something more Qtish like close() or accept() or setAccepted(). (I hope I understood it's intention correctly).
5. parentWidget() needs a better name, but I don't have a better suggestion.

Also, how does the patch also deal with multiple selections? (luiz said it doesn't). So, we just leave that for later?

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> Some suggestions that I mentioned this on irc, pasting here for the record.
> 
> 1. Rename populate to setItems
> 2. Rename setIndex to setSelectedIndex or setCurrentIndex?

int	currentIndex () const
void	setCurrentIndex ( int index )

Seems to be the Qt way.

> 3. show() should not ideally have a selectedIndex argument. Have a separate
> select() maybe?

maybe add a geometry() method as well as the currentIndex()! so on show you can call these yourself.

> 4. didHide to something more Qtish like close() or accept() or setAccepted().
> (I hope I understood it's intention correctly).
 
It is a method called by webcore. Maybe we should emit something like:

void QGraphicsObject::visibleChanged ()   [signal]
or
void QDockWidget::visibilityChanged ( bool visible )   [signal]

hmm, Qt has a problem with naming here :-)

> 5. parentWidget() needs a better name, but I don't have a better suggestion.

What about view() ?

[Luiz Agostini]

Created attachment 46845 [details]
Custom popups and popup factories.

Completely new API.
Changes made based on the suggestions made by Simon, Girish and Kenneth.

All the feedbacks and suggestions were of great value. I hope this change
has been in the right direction.

I am trying to learn. Thank you all.

[Luiz Agostini]

Created attachment 46855 [details]
Custom popups and popup factories.

Small corrections to previous patch. See comments there.

[WebKit Review Bot]

Attachment 46845 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/198224

[WebKit Review Bot]

Attachment 46855 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/200055

[Luiz Agostini]

Created attachment 46894 [details]
Custom popups and popup factories.

correcting previous broken patch.

[Simon Hausmann]

(In reply to comment #32)
> Created an attachment (id=46894) [details]
> Custom popups and popup factories.
> 
> correcting previous broken patch.

We're going to take a close look at the API here in Oslo this coming Friday (22nd).

[Luiz Agostini]

Created attachment 47020 [details]
WebCore patch.

Splitting previous patch in two.
This first has the code that is not related to the public API.
The next one will have just the code related to the public API proposal.

[Luiz Agostini]

Created attachment 47022 [details]
Public API

see comments in previous patch.

[WebKit Commit Bot]

Comment on attachment 47020 [details]
WebCore patch.

Clearing flags on attachment: 47020

Committed r53610: <http://trac.webkit.org/changeset/53610>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Luiz Agostini]

Proceeding with the api.

[WebKit Review Bot]

Attachment 47022 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:51:  Alphabetical sorting problem.  [build/include_order] [4]
Ignoring WebKit/qt/Api/qwebpage.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebdelegateselectpopup.cpp; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebdelegateselectpopup_p.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebpage.h; This file is exempt from the style guide.
Ignoring WebKit/qt/Api/qwebdelegateselectpopup.h; This file is exempt from the style guide.
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chang Shu]

I see the following line in headers.pri, which does not seem to be right.
$$PWD/qwebdelegateselectpopup_p.h

[Simon Hausmann]

Comment on attachment 47022 [details]
Public API

Taking this patch out of the review queue. In the API review over the phone last Friday we discussed that this needs more research as part of a full ui delegate solution.

[Luiz Agostini]

Will be worked in future as part of the full ui delegate solution.