Bug 67938 - [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox
Summary: [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords:
Depends on:
Blocks: 62191
  Show dependency treegraph
 
Reported: 2011-09-12 09:48 PDT by Caio Marcelo de Oliveira Filho
Modified: 2011-09-19 10:13 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.83 KB, patch)
2011-09-12 09:56 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (13.83 KB, patch)
2011-09-14 12:30 PDT, Caio Marcelo de Oliveira Filho
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-09-12 09:48:34 PDT
[Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox
Comment 1 Caio Marcelo de Oliveira Filho 2011-09-12 09:56:19 PDT
Created attachment 107058 [details]
Patch
Comment 2 Caio Marcelo de Oliveira Filho 2011-09-12 09:58:45 PDT
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).
Comment 3 Andreas Kling 2011-09-14 10:25:33 PDT
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.
Comment 4 Caio Marcelo de Oliveira Filho 2011-09-14 12:30:44 PDT
Created attachment 107374 [details]
Patch
Comment 5 Caio Marcelo de Oliveira Filho 2011-09-14 12:33:41 PDT
(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.
Comment 6 Andreas Kling 2011-09-19 09:16:29 PDT
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.
Comment 7 Caio Marcelo de Oliveira Filho 2011-09-19 10:13:37 PDT
Committed r95436: <http://trac.webkit.org/changeset/95436>