[webkit-reviews] review denied: [Bug 34267] [Qt] Support kinetic scrolling on Maemo 5 : [Attachment 47620] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jan 28 08:22:38 PST 2010


Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 34267: [Qt] Support kinetic scrolling on Maemo 5
https://bugs.webkit.org/show_bug.cgi?id=34267

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

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
> diff --git a/WebKit/qt/Api/qwebview.cpp b/WebKit/qt/Api/qwebview.cpp
> index 9ef2e55..4eeb8ad 100644
> --- a/WebKit/qt/Api/qwebview.cpp
> +++ b/WebKit/qt/Api/qwebview.cpp
> @@ -58,6 +58,107 @@ void QWebViewPrivate::_q_pageDestroyed()
>      view->setPage(0);
>  }
>  
> +#ifdef Q_WS_MAEMO_5
> +#include "qabstractkineticscroller.h"
> +
> +class QWebViewKineticScroller : public QAbstractKineticScroller {
> +public:
> +    QWebViewKineticScroller()
> +	   : QAbstractKineticScroller()
> +    {}

I would put the above on one line, since we have no implementation anyway

> +
> +    // remember the frame where the button was pressed
> +    bool eventFilter(QObject *o, QEvent *e)

Almost all of qwebview.cpp uses ev and not e, please change

> +    {
> +	   switch (e->type()) {
> +	   case QEvent::MouseButtonPress: {
> +	       QWebFrame *hitFrame = scrollingFrameAt(static_cast<QMouseEvent
*>(e)->pos());

coding style, * should be aligned to the left

> +	       if (hitFrame)
> +		   _frame = hitFrame;

please use m_frame, which is webkit style.

> +	       break;
> +	   }
> +	   default:
> +	       break;
> +	   }
> +	   return QAbstractKineticScroller::eventFilter(o, e);
> +    }
> +
> +protected:
> +    QWebFrame *currentFrame() const

here as well

> +    {
> +	   if (!_frame.isNull())
> +	       return _frame.data();
> +
> +	   QWebView *web = static_cast<QWebView *>(widget());

web is a bad variable name, maybe use view instead

> +	   QWebFrame *frame = web->page()->mainFrame();

* again, please also run check-webkit-style on it

> +	   return frame;
> +    }
> +
> +    /*! Returns the innermost frame at the given position that can scroll.
*/

use // commenting

> +    QWebFrame *scrollingFrameAt(const QPoint &pos) const

coding style, * and &

> +    {
> +	   QWebView *web = static_cast<QWebView *>(widget());

view

> +	   QWebFrame *mainFrame = web->page()->mainFrame();
> +	   QWebFrame *hitFrame = mainFrame->hitTestContent(pos).frame();

coding style

> +	   QSize range = hitFrame->contentsSize() -
hitFrame->geometry().size();
> +
> +	   while (hitFrame && range.width() <= 1 && range.height() <= 1)
> +	       hitFrame = hitFrame->parentFrame();

Are you sure infinite loops can never occour here?

> +
> +	   return hitFrame;
> +    }
> +
> +    void attachToWidget()
> +    {
> +	   QWebView *web = static_cast<QWebView *>(widget());
> +	   QWebFrame *mainFrame = web->page()->mainFrame();
> +	   mainFrame->setScrollBarPolicy(Qt::Vertical, Qt::ScrollBarAlwaysOff);

> +	   mainFrame->setScrollBarPolicy(Qt::Horizontal,
Qt::ScrollBarAlwaysOff);
> +	   web->installEventFilter(this);
> +    }
> +
> +    void removeFromWidget()
> +    {
> +	   QWebView *web = static_cast<QWebView *>(widget());
> +	   web->removeEventFilter(this);
> +	   QWebFrame *mainFrame = web->page()->mainFrame();
> +	   mainFrame->setScrollBarPolicy(Qt::Vertical, Qt::ScrollBarAsNeeded);
> +	   mainFrame->setScrollBarPolicy(Qt::Horizontal,
Qt::ScrollBarAsNeeded);

Shouldn't you save the old scrollbar policyt instead and reapply it?

> +    }
> +
> +    QRect positionRange() const
> +    {
> +	   QRect r;
> +	   QWebFrame *frame = currentFrame();

style

> +	   r.setSize(frame->contentsSize() - frame->geometry().size());
> +	   return r;
> +    }
> +
> +    QPoint position() const
> +    {
> +	   QWebFrame *frame = currentFrame();

style

> +	   return frame->scrollPosition();
> +    }
> +
> +    QSize viewportSize() const
> +    {
> +	   return static_cast<QWebView *>(widget())->page()->viewportSize();

style.

> +    }
> +
> +    void setPosition(const QPoint &p, const QPoint &overShootDelta)
> +    {
> +	   Q_UNUSED(overShootDelta);

not webkit style, just leave out the second param. (please use point as well,
not p)

or do

void setPosition(const QPoint& point, const QPoint& /* overShootDelta */)

> +
> +	   QWebFrame *frame = currentFrame();
> +	   frame->setScrollPosition(p);
> +    }
> +
> +    QPointer<QWebFrame> _frame;
> +};
> +
> +#endif // Q_WS_MAEMO_5
> +
> +
>  /*!
>      \class QWebView
>      \since 4.4
> @@ -157,7 +258,11 @@ QWebView::QWebView(QWidget *parent)
>  #if QT_VERSION >= QT_VERSION_CHECK(4, 6, 0)
>      setAttribute(Qt::WA_AcceptTouchEvents);
>  #endif
> -
> +#if defined(Q_WS_MAEMO_5)
> +    QAbstractKineticScroller *ks = new QWebViewKineticScroller();

use scroller instead of ks, *

> +    ks->setWidget(this);
> +    setProperty("kineticScroller", QVariant::fromValue(ks));
> +#endif
>      setAcceptDrops(true);
>  
>      setMouseTracking(true);
> diff --git a/WebKit/qt/ChangeLog b/WebKit/qt/ChangeLog
> index f9aa123..f295572 100644
> --- a/WebKit/qt/ChangeLog
> +++ b/WebKit/qt/ChangeLog
> @@ -1,3 +1,27 @@
> +2010-01-28  Andreas Kling  <andreas.kling at nokia.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   [Qt] Support kinetic scrolling on Maemo 5
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=34267
> +
> +	   Patch by Ralf Engels <ralf.engels at nokia.com> and
> +	   Robert Griebl <rgriebl at trolltech.com>
> +
> +	   * Api/qwebview.cpp:
> +	   (QWebViewKineticScroller::QWebViewKineticScroller):
> +	   (QWebViewKineticScroller::eventFilter):
> +	   (QWebViewKineticScroller::currentFrame):
> +	   (QWebViewKineticScroller::scrollingFrameAt):
> +	   (QWebViewKineticScroller::attachToWidget):
> +	   (QWebViewKineticScroller::removeFromWidget):
> +	   (QWebViewKineticScroller::positionRange):
> +	   (QWebViewKineticScroller::position):
> +	   (QWebViewKineticScroller::viewportSize):
> +	   (QWebViewKineticScroller::setPosition):
> +	   (QWebView::QWebView):
> +
>  2010-01-28  Kenneth Rohde Christiansen  <kenneth at webkit.org>
>  
>	   Reviewed by Simon Hausmann.


More information about the webkit-reviews mailing list