Summary: | [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||
Component: | New Bugs | Assignee: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, igor.oliveira, kling, luiz | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 62191 | ||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2011-09-12 09:48:34 PDT
Created attachment 107058 [details]
Patch
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 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. Created attachment 107374 [details]
Patch
(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 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. Committed r95436: <http://trac.webkit.org/changeset/95436> |