Bug 34267 – [Qt] Support kinetic scrolling on Maemo 5

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Andreas Kling]

Basically by forward-porting code from the Qt/Maemo branch.

[Andreas Kling]

Created attachment 47620 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 47620 [details]
Patch

> 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 @@
> [phone]  Andreas Kling  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] Support kinetic scrolling on Maemo 5
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=34267
> +
> +        Patch by Ralf Engels <[e-mail]> and
> +        Robert Griebl <[e-mail]>
> +
> +        * 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  <[e-mail]>
>  
>          Reviewed by Simon Hausmann.

[Andreas Kling]

Created attachment 47625 [details]
Same patch with issues addressed

How would an infinite loop occur? Can a frame somehow be its own grandparent?

[Kenneth Rohde Christiansen]

Comment on attachment 47625 [details]
Same patch with issues addressed

LGTM, but when we enable frameflattening it needs to be looked at again, as QWebFrame* scrollingFrameAt(const QPoint& pos) const becomes useless.

[WebKit Commit Bot]

Comment on attachment 47625 [details]
Same patch with issues addressed

Clearing flags on attachment: 47625

Committed r54034: <http://trac.webkit.org/changeset/54034>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

doing this w/o a null check is really safe ?



+    // Returns the innermost frame at the given position that can scroll.
+    QWebFrame* scrollingFrameAt(const QPoint& pos) const
+    {
+        QWebView* view = static_cast<QWebView*>(widget());

(..)

void attachToWidget()
+    {
+        QWebView* view = static_cast<QWebView*>(widget());
+        QWebFrame* mainFrame = view->page()->mainFrame();


what about QGWV ?

[Antonio Gomes]

err, ignore my concern. that is code inside qwebview.cpp ...

(In reply to comment #7)
> doing this w/o a null check is really safe ?
> +    // Returns the innermost frame at the given position that can scroll.
> +    QWebFrame* scrollingFrameAt(const QPoint& pos) const
> +    {
> +        QWebView* view = static_cast<QWebView*>(widget());
> (..)
> void attachToWidget()
> +    {
> +        QWebView* view = static_cast<QWebView*>(widget());
> +        QWebFrame* mainFrame = view->page()->mainFrame();
> 
> what about QGWV ?

[Simon Hausmann]

(In reply to comment #5)
> (From update of attachment 47625 [details])
> Clearing flags on attachment: 47625
> 
> Committed r54034: <http://trac.webkit.org/changeset/54034>

Cherry-picked into qtwebkit-4.6 with commit 68245b52f892a1effdfef9d70d8f9fa888d09e9f