[webkit-reviews] review denied: [Bug 62191] [Qt] Creating webkit2 popup menu hooks and adding popup menus to MiniBrowser. : [Attachment 104171] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Igor Trindade Oliveira
<itrindade.oliveira at gmail.com>'s request for review:
Bug 62191: [Qt] Creating webkit2 popup menu hooks and adding popup menus to
MiniBrowser.
https://bugs.webkit.org/show_bug.cgi?id=62191

Attachment 104171: Patch
https://bugs.webkit.org/attachment.cgi?id=104171&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=104171&action=review


I talked with Jens about using the desktop QML components. That project is just
not gonna cut it, and we should not rely on it for now.

Since we have to get rid of QWidget, Jens suggest we put all dependencies to
QWidget in plugins. That will be quite some work, but that is something to keep
in mind.

I apologize for blocking this patch for a while. I believed the Desktop
components were going to make it to Qt5.


When it comes to this patch...this patch looks very weird. Nothing is used in
the standard way. That might be correct, but that requires explanations.

> Source/WebKit2/ChangeLog:7
> +	   This patch implements a simple QtWebKit WebKit2 popup menu in
DesktopWebView.
> +

This changelog needs a lot more explanations.

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

This invalidate() is here for good reasons. The way WebPopupMenuProxy::
showPopupMenu() is designed is to be used as a blocking API.

I would prefer to refactor this assumption if necessary instead of adding a
#ifdef for Qt. Otherwise you are abusing the design of the class and it is
opening gates for future bugs when refactoring WebPopupMenuProxy.

> Source/WebKit2/UIProcess/qt/QComboBoxWebPopupMenuProxy.cpp:116
> +
> +    QMouseEvent event(QEvent::MouseButtonPress, QCursor::pos(),
Qt::LeftButton,
> +			 Qt::LeftButton, Qt::NoModifier);
> +    QCoreApplication::sendEvent(m_combo, &event);

This needs comments because that looks really weird.