Bug 67938 – [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

[Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox

[Caio Marcelo de Oliveira Filho]

Created attachment 107058 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Less general than the bug 62191, but I see it as a starting point. With this patch QDesktopWebView will have a working combobox.

Then we can figure out the best way to allow this to be customized by API users (or decide whether we want that).

[Andreas Kling]

Comment on attachment 107058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107058&action=review

> Source/WebKit2/ChangeLog:10
> +        explicitly avoid running a nested mainloop.

avoid -> avoids

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2308
> +#if !PLATFORM(QT)
>      protectedActivePopupMenu->invalidate();
> +#endif

We need a comment here, explaining that we need to hang on to the pointer since this is a 2-step process on Qt.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.cpp:48
> +    QWidget* parentWidget = m_webViewItem->canvas();
> +    comboBox->setParent(parentWidget);

No need for the parentWidget variable here.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.cpp:95
> +    for (int i = 0; i < items.size(); ++i) {

int i -> size_t i

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.h:44
> +    Q_DISABLE_COPY(WebPopupMenuProxyQtDesktop)

Is this necessary?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.h:58
> +    void selectIndex(int index);

I'd call this "setSelectedIndex(int)"

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.h:69
> +    int32_t m_selectedIndex;
> +    QSGItem* m_webViewItem;

Nit: Reverse the order of these two members, since QSGItem* is bigger than int32_t on 64-bit.

[Caio Marcelo de Oliveira Filho]

Created attachment 107374 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #3)
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.h:44
> > +    Q_DISABLE_COPY(WebPopupMenuProxyQtDesktop)
> 
> Is this necessary?

Not strictly. I took it off since it seems to cause more noise than benefit.

All the other comments were addressed by the new attached patch.

[Andreas Kling]

Comment on attachment 107374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107374&action=review

Looks good, let's start with this!
r=me with this tiny (perhaps personal) style nit:

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.cpp:87
> +    Q_ASSERT(comboBox);

Q_ASSERT -> ASSERT

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQtDesktop.cpp:92
> +    Q_ASSERT(model);

Ditto.

[Caio Marcelo de Oliveira Filho]

Committed r95436: <http://trac.webkit.org/changeset/95436>