Bug 73560

Created attachment 117415 [details] Patch

MiniBrowser doesn't show anything, but will work once patch for bug 73338 lands.

(In reply to comment #2) > MiniBrowser doesn't show anything, but will work once patch for bug 73338 lands. anything = any popup menu yet :-)

Comment on attachment 117415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review > Source/WebKit2/ChangeLog:10 > + component used when a popup menu is shown. When loaded, this component will > + have available in its context the popupMenuModel object, that can be used not very English :/ When loaded the component will have the popupMenuModel available in its context. > Source/WebKit2/ChangeLog:17 > + The existing Desktop version is removed since after the Qt5 refactoring isn't > + working correctly. Once Qt have its own QML components for popup, we hope to use > + it as a default if no other popupMenu is specified. Sounds good > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465 > +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options. Maybe optionMenu or just OptionSelector? like FileSelector. > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53 > + enum Roles { > + EnabledRole = Qt::UserRole, > + SelectedRole = Qt::UserRole + 1, > + IsSeparatorRole = Qt::UserRole + 2 How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches? > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117 > + case IsSeparatorRole: Why not just SeparatorRole?

Comment on attachment 117415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review Thanks for the comments, Kenne. >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465 >> +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const > > I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options. > > Maybe optionMenu or just OptionSelector? like FileSelector. Maybe ItemSelector is better then? (like Grob branch) >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53 >> + IsSeparatorRole = Qt::UserRole + 2 > > How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches? I mentioned grouped options in a FIXME, maybe it's better to just go forward and implement in this patch (since it would only involve Qt code). :-) I think we could first move to the QML supporting what we have now, then start upstreaming multiple selection support (that Grob branch have) in a separate patch. >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117 >> + case IsSeparatorRole: > > Why not just SeparatorRole? In QML usage, I though model.isSeparator was better than model.separator.

(In reply to comment #5) > (From update of attachment 117415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review > > Thanks for the comments, Kenne. > > >> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:465 > >> +QDeclarativeComponent* QQuickWebViewExperimental::popupMenu() const > > > > I dont like the name popup... It is not really a popup, that highly depends on the implementation. What about selectorMenu ? or similar. It comes from the select tag, which has options. > > > > Maybe optionMenu or just OptionSelector? like FileSelector. > > Maybe ItemSelector is better then? (like Grob branch) Yeah, I kind of like that > >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:53 > >> + IsSeparatorRole = Qt::UserRole + 2 > > > > How do we handle indentation? (grouped options)? Also what about multi selection? pre selections ? Did you check the grob webkit patches? > > I mentioned grouped options in a FIXME, maybe it's better to just go forward and implement in this patch (since it would only involve Qt code). :-) > > I think we could first move to the QML supporting what we have now, then start upstreaming multiple selection support (that Grob branch have) in a separate patch. That is fine :-) I just wanted to make sure you considered it. Also preselections (already selected items). > >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:117 > >> + case IsSeparatorRole: > > > > Why not just SeparatorRole? > > In QML usage, I though model.isSeparator was better than model.separator. I agree :-)

Comment on attachment 117415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221 > +#include "WebPopupMenuProxyQt.moc" > +#include "moc_WebPopupMenuProxyQt.cpp" You should only need one of these

Comment on attachment 117415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221 >> +#include "moc_WebPopupMenuProxyQt.cpp" > > You should only need one of these I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something?

Created attachment 117460 [details] Patch

(In reply to comment #8) > (From update of attachment 117415 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review > > >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221 > >> +#include "moc_WebPopupMenuProxyQt.cpp" > > > > You should only need one of these > > I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something? Yes, the "moc_***" file is generated when you add your h file to HEADERS in the .pro file.

Comment on attachment 117415 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117415&action=review >>>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221 >>>> +#include "moc_WebPopupMenuProxyQt.cpp" >>> >>> You should only need one of these >> >> I have a Q_OBJECT in my .cpp file and another in my .h file. I thought the first moc inclusion would trigger the moc to run on cpp file, and the second would include the moc for the .h in our existing .cpp. Am I missing something? > > Yes, the "moc_***" file is generated when you add your h file to HEADERS in the .pro file. I think these two includes are correct and necessary. Including "WebPopupMenuProxyQt.moc" triggers the moc utility run on WebPopupMenuProxy.cpp -- we want that because we declare a QObject subclass in this source. The moc utility will run in .h file in our case (as Noam pointed out, because of HEADERS), generating "moc_WebPopupMenuProxyQt.cpp". If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros). Another tricky (that qmake understands) is that if we do include "moc_XXX.cpp" on our source, it won't compile "moc_XXX.cpp" by itself. This is perfect, because our source file includes "config.h". In summary: removing the first and we don't get extra stuff from QObject declared in the cpp file, removing the second and we end up compiling moc_XXX.cpp standalone, which doesn't work. I tested removing each one before uploading the last patch.

> If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros). > Fair enough, though deserves a comment in code!

(In reply to comment #12) > > If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros). > > > > Fair enough, though deserves a comment in code! Need some help writing this in a concise manner :-) "The first include triggers moc to run on this cpp file -- and includes it (XXX.moc). The second indicates qmake that the generated file for our header (moc_XXX) isn't compiled standalone. This is important because the generated file doesn't include "config.h" but include WebKit headers." Or maybe something like: #include "XXX.moc" // Because we define QObjects in XXX.cpp #include "moc_XXX.cpp" // Because generated file for header doesn't include "config.h" Note that the second include is standard practice in our sources already.

> #include "XXX.moc" // Because we define QObjects in XXX.cpp This one doesn't need a comment. > #include "moc_XXX.cpp" // Because generated file for header doesn't include "config.h" // To make sure moc uses the right defines that originate in config.h, we include the moc file here instead of in the project file HEADERS directive.

Comment on attachment 117460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117460&action=review I love the API :) > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:93 > + Vector<Item> m_items; Super nitpick that you're completely free to ignore and I'm just saying because I'm bored ;-) : You could declare vector typetraits for the item type, so that the vector can move the items efficiently. Another approach would be to use QVector and Q_DECLARE_TYPEINFO(Item, Q_MOVEABLE_TYPE) (IIRC)

(In reply to comment #12) > > If I stopped here, qmake would try to compile "moc_WebPopupMenuProxyQt.cpp" by itself, and it would fail, because it includes "WebPopupMenuProxyQt.h", that includes WebKit headers, but doesn't include "config.h" from WebKit (so no ENABLE() and USE() and PLATFORM() macros). > > > > Fair enough, though deserves a comment in code! Ah, yepp, in that case you need both. Comment makes sense, thanks for adding.

Comment on attachment 117460 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117460&action=review > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:47 > + Add a property for the list of items here, as discussed. I think we also want a int property initalSelection, that you then could map to ListView.selectedIndex if you visualize the items as a ListView. > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:64 > + void dismiss() { emit dismissed(); } These can also probably be normal functions with Q_INVOCABLE? > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:67 > + void selectedIndex(int); I don't think we need to keep track of this, as long as we have the initialSelection. > Tools/MiniBrowser/qt/qml/ItemSelector.qml:63 > + onClicked: selectorModel.selectIndex(model.index) The item should change the current listView selection. The list view should then hook into selectionChanged, and then submit the dialog. Some dialogs might implement it by requiring the user to press ok for example.

Created attachment 117887 [details] Patch

Comment on attachment 117887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117887&action=review Looks good to me > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24 > + selectorModel.items.reject() I wonder if rejectChanged/acceptChanges would be more clear > Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/select.html:16 > +<option value="__open__" >Open</option> > +<option value="__closed__" >Closed</option> > +<option value="__all__" >All</option> you python you :-) > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78 > + { } > + QString text; newline > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:103 > + ItemsModel* items() { return &m_items; } ItemModel no? why the s?

Created attachment 118561 [details] Patch

Comment on attachment 117887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117887&action=review >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24 >> + selectorModel.items.reject() > > I wonder if rejectChanged/acceptChanges would be more clear I rather stay with accept/reject since was the one discussed with Tor-Arne for this patch. >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/select.html:16 >> +<option value="__all__" >All</option> > > you python you :-) Hehe, just copied from webkit.org bugzilla search page :-) >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:78 >> + QString text; > > newline Fixed. >> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:103 >> + ItemsModel* items() { return &m_items; } > > ItemModel no? why the s? Renamed to PopupMenuItemModel.

Comment on attachment 118561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118561&action=review > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:9 > + property int shouldSelectIndexFirst This sounds like a bool. can you find a better name? > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:10 > + property int shouldSelectIndex same with this > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:21 > + selectorModel.items.select(WebView.view.shouldSelectIndexFirst) Now that we've split up the selector model and the items model, you can name the selector model "model" > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24 > + selectorModel.items.reject() reject() and accept() should be on the popup model, not on the individual items

Created attachment 118591 [details] Patch

Comment on attachment 118561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118561&action=review The latest patch also fixes some problems I had in the testing. Now should be OK. >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:9 >> + property int shouldSelectIndexFirst > > This sounds like a bool. can you find a better name? Fixed. >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:10 >> + property int shouldSelectIndex > > same with this Fixed. >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:21 >> + selectorModel.items.select(WebView.view.shouldSelectIndexFirst) > > Now that we've split up the selector model and the items model, you can name the selector model "model" Fixed. >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24 >> + selectorModel.items.reject() > > reject() and accept() should be on the popup model, not on the individual items Fixed.

Comment on attachment 118561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118561&action=review >>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24 >>> + selectorModel.items.reject() >> >> reject() and accept() should be on the popup model, not on the individual items > > Fixed. For the record, they weren't in individual items. They were "invokables" in our model that contained items. But per IRC discussion, I moved them to the "main model" of our item selector.

Committed r102573: <http://trac.webkit.org/changeset/102573>