Bug 33418

Where are the attachments, Luiz?

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

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

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

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

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

+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.

What about merging your QWebPopupObserver with the QWebPopupPrivate?

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

(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.

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

(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

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

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

+ $$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.

(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?

(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

(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.

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.

(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.

(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 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.

> * 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.

(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.

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.

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?

(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() ?

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.

Created attachment 46855 [details] Custom popups and popup factories. Small corrections to previous patch. See comments there.

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

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

Created attachment 46894 [details] Custom popups and popup factories. correcting previous broken patch.

(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).

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.

Created attachment 47022 [details] Public API see comments in previous patch.

Comment on attachment 47020 [details] WebCore patch. Clearing flags on attachment: 47020 Committed r53610: <http://trac.webkit.org/changeset/53610>

All reviewed patches have been landed. Closing bug.

Proceeding with the api.

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.

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

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.

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