[webkit-reviews] review denied: [Bug 40082] [Qt] Dropdown box is seen twice in a webpage. : [Attachment 57809] Added better comment to the empty paint()

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 4 12:43:05 PDT 2010


Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Viatcheslav
Ostapenko <ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 40082: [Qt] Dropdown box is seen twice in a webpage.
https://bugs.webkit.org/show_bug.cgi?id=40082

Attachment 57809: Added better comment to the empty paint()
https://bugs.webkit.org/attachment.cgi?id=57809&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
We are getting there!

I do not like the ChangeLog entry. First of all, it doesn't start by saying
what the actual problem is (neither does the bug title, which just explains the
solution), but instead explain very technically what is causing it and how you
are fixing in. That part is good!

When you write a ChangeLog please consider: what, why, how. What is the issue
at hand/what are you implementing, why are you doing this, how are you
doing/fixing it.

>  
> +class QFallbackWebPopupGraphicsProxyWidget : public QGraphicsProxyWidget {

I would not prefix this with Q* as this is not part of the our API, instead I
would use Qt*

Actually you are implementing it inside the QtFallbackWebPopup, which has
class QtFallbackWebPopupCombo : public QComboBox {
So the above is actually what you are wrapping.

QtFallbackWebPopupComboProxyWidget would be fine with me.

> +public:
> +    QFallbackWebPopupGraphicsProxyWidget(QGraphicsItem *parent = 0,
Qt::WindowFlags wFlags = 0) : 
> +				       QGraphicsProxyWidget(parent, wFlags) {}

Wrong indentation, and wrong placement of * 

> +/*
> +    Drawing the dropdowns consist of two parts: drawing the combobox itself
and.
> +    drawing the dropdown (or popup as its called). When not using graphics
view,.
> +    Webkit renders the combobox itself via RenderTheme, and draws the popup
using.
> +    QComboBox::showPopup. When using QGraphicsWebView the combobox gets
drawn twice.
> +    because the popup is rendered via a QGraphicsProxyWidget that is
wrapping a QComboBox..
> +    So, in graphics scene there is a graphics item having the QComboBox on
top of.
> +    the QGraphicsWebView, and the QGraphicsWebView has already drawn the
combobox once.
> +
> +    When clicking somewhere on the page, we are calling hidePopup() for the
popup and.
> +    proxy->setVisible(false) gets called specifically. However, when
QGraphicsView loses.
> +    the focus, popups are removed but the graphics item (combobox) itself is
not. So,.
> +    graphics view really works as expected. Its not even supposed to remove
any comboboxes.
> +    added to it when the focus changes, just to close any open popups. Its
not possible.
> +    to get focus events to QWebPopup so we cannot just call
proxy->setVisible(false).
> +    when losing the focus.
> +
> +    The real problem is that we are overdrawing the combobox when using the
proxy widget.
> +    One way to fix this would be to make sure we are not drawing twice. Just
inherit from.
> +    QGraphicsProxyWidget and overwrite the paint method and dont draw
anything there.
> +*/									       
						     
> +    void paint(QPainter*, const QStyleOptionGraphicsItem*, QWidget*) {}
> +};

Comments inline in code usuablly uses // and not /* *** */. Please change that.


> +
>  QtFallbackWebPopupCombo::QtFallbackWebPopupCombo(QtFallbackWebPopup&
ownerPopup)
>      : m_ownerPopup(ownerPopup)
>  {
> @@ -115,7 +143,7 @@ void QtFallbackWebPopup::show()
>      QRect rect = geometry();
>      if (QGraphicsWebView *webView =
qobject_cast<QGraphicsWebView*>(pageClient()->pluginParent())) {
>	   if (!m_proxy) {
> -	       m_proxy = new QGraphicsProxyWidget(webView);
> +	       m_proxy = new QFallbackWebPopupGraphicsProxyWidget(webView);
>	       m_proxy->setWidget(m_combo);
>	   } else
>	       m_proxy->setVisible(true);


More information about the webkit-reviews mailing list