[Webkit-unassigned] [Bug 40082] [Qt] Dropdown box is seen twice in a webpage.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40082


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57809|review?                     |review-
               Flag|                            |




--- Comment #5 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-06-04 12:43:06 PST ---
(From update of attachment 57809)
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);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.