Bug 33418 - [Qt] Custom select popups.
Summary: [Qt] Custom select popups.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 31552 33708 33887
  Show dependency treegraph
 
Reported: 2010-01-08 21:17 PST by Luiz Agostini
Modified: 2010-01-29 06:30 PST (History)
10 users (show)

See Also:


Attachments
Custom popups, popup factories and usage example (27.75 KB, patch)
2010-01-09 07:43 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Custom popups, popup factories and usage example. (29.51 KB, patch)
2010-01-09 08:53 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Custom popups, popup factories and usage example. (29.86 KB, patch)
2010-01-09 09:09 PST, Luiz Agostini
hausmann: review-
Details | Formatted Diff | Diff
Custom popups and popup factories. (19.33 KB, patch)
2010-01-15 01:30 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Custom popups and popup factories. (30.36 KB, patch)
2010-01-18 14:03 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Custom popups and popup factories. (30.93 KB, patch)
2010-01-18 14:52 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Custom popups and popup factories. (31.00 KB, patch)
2010-01-19 00:15 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
WebCore patch. (15.93 KB, patch)
2010-01-20 06:43 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Public API (17.74 KB, patch)
2010-01-20 07:01 PST, 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-01-08 21:17:41 PST
Custom select popup factories.
Usage example in QGVLauncher.
Comment 1 Kenneth Rohde Christiansen 2010-01-09 04:49:52 PST
Where are the attachments, Luiz?
Comment 2 Luiz Agostini 2010-01-09 07:43:03 PST
Created attachment 46206 [details]
Custom popups, popup factories and usage example
Comment 3 WebKit Review Bot 2010-01-09 07:47:43 PST
Attachment 46206 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/176377
Comment 4 WebKit Review Bot 2010-01-09 07:48:02 PST
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
Comment 5 Luiz Agostini 2010-01-09 08:07:19 PST
Build bot probably is not using Qt 4.6. I used animations in QGVLauncher. This seems to be the reason for the compilation error.
Comment 6 Kenneth Rohde Christiansen 2010-01-09 08:13:10 PST
I guess you should leave the usage example as a separate commit, and guard it with version checks for Qt 4.6
Comment 7 Kenneth Rohde Christiansen 2010-01-09 08:17:11 PST
+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.
Comment 8 Kenneth Rohde Christiansen 2010-01-09 08:18:59 PST
What about merging your QWebPopupObserver with the QWebPopupPrivate?
Comment 9 Luiz Agostini 2010-01-09 08:53:44 PST
Created attachment 46208 [details]
Custom popups, popup factories and usage example.
Comment 10 Luiz Agostini 2010-01-09 08:58:05 PST
(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.
Comment 11 WebKit Review Bot 2010-01-09 08:59:18 PST
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
Comment 12 Kenneth Rohde Christiansen 2010-01-09 09:08:52 PST
(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
Comment 13 Luiz Agostini 2010-01-09 09:09:27 PST
Created attachment 46209 [details]
Custom popups, popup factories and usage example.
Comment 14 WebKit Review Bot 2010-01-09 09:10:12 PST
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
Comment 15 Kenneth Rohde Christiansen 2010-01-09 09:11:20 PST
+                     $$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.
Comment 16 Luiz Agostini 2010-01-09 09:13:27 PST
(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?
Comment 17 Luiz Agostini 2010-01-09 09:15:17 PST
(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
Comment 18 Kenneth Rohde Christiansen 2010-01-09 09:17:55 PST
(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.
Comment 19 Adam Barth 2010-01-09 16:34:32 PST
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.
Comment 20 Kenneth Rohde Christiansen 2010-01-11 02:28:28 PST
(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.
Comment 21 Simon Hausmann 2010-01-11 06:32:40 PST
(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.
Comment 22 Simon Hausmann 2010-01-11 06:33:50 PST
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.
Comment 23 Kenneth Rohde Christiansen 2010-01-11 06:41:51 PST
> * 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.
Comment 24 Luiz Agostini 2010-01-11 07:11:06 PST
(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.
Comment 25 Luiz Agostini 2010-01-15 01:30:10 PST
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.
Comment 26 Girish Ramakrishnan 2010-01-18 05:03:12 PST
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?
Comment 27 Kenneth Rohde Christiansen 2010-01-18 05:39:07 PST
(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() ?
Comment 28 Luiz Agostini 2010-01-18 14:03:16 PST
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.
Comment 29 Luiz Agostini 2010-01-18 14:52:41 PST
Created attachment 46855 [details]
Custom popups and popup factories.

Small corrections to previous patch. See comments there.
Comment 30 WebKit Review Bot 2010-01-18 15:54:44 PST
Attachment 46845 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/198224
Comment 31 WebKit Review Bot 2010-01-18 18:58:51 PST
Attachment 46855 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/200055
Comment 32 Luiz Agostini 2010-01-19 00:15:41 PST
Created attachment 46894 [details]
Custom popups and popup factories.

correcting previous broken patch.
Comment 33 Simon Hausmann 2010-01-19 07:51:52 PST
(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).
Comment 34 Luiz Agostini 2010-01-20 06:43:18 PST
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.
Comment 35 Luiz Agostini 2010-01-20 07:01:57 PST
Created attachment 47022 [details]
Public API

see comments in previous patch.
Comment 36 WebKit Commit Bot 2010-01-21 00:10:54 PST
Comment on attachment 47020 [details]
WebCore patch.

Clearing flags on attachment: 47020

Committed r53610: <http://trac.webkit.org/changeset/53610>
Comment 37 WebKit Commit Bot 2010-01-21 00:11:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Luiz Agostini 2010-01-21 03:10:19 PST
Proceeding with the api.
Comment 39 WebKit Review Bot 2010-01-21 03:15:56 PST
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.
Comment 40 Chang Shu 2010-01-27 14:36:08 PST
I see the following line in headers.pri, which does not seem to be right.
$$PWD/qwebdelegateselectpopup_p.h
Comment 41 Simon Hausmann 2010-01-27 15:01:24 PST
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.
Comment 42 Luiz Agostini 2010-01-29 06:30:38 PST
Will be worked in future as part of the full ui delegate solution.