[webkit-reviews] review denied: [Bug 67938] [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox : [Attachment 107058] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 14 10:25:33 PDT 2011
Andreas Kling <kling at webkit.org> has denied Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 67938: [Qt] [WK2] Implement popup menus in QDesktopWebView using QComboBox
https://bugs.webkit.org/show_bug.cgi?id=67938
Attachment 107058: Patch
https://bugs.webkit.org/attachment.cgi?id=107058&action=review
------- Additional Comments from Andreas Kling <kling at webkit.org>
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.
More information about the webkit-reviews
mailing list