Bug 34267 - [Qt] Support kinetic scrolling on Maemo 5
Summary: [Qt] Support kinetic scrolling on Maemo 5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 08:08 PST by Andreas Kling
Modified: 2010-02-01 19:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2010-01-28 08:12 PST, Andreas Kling
kenneth: review-
kenneth: commit-queue-
Details | Formatted Diff | Diff
Same patch with issues addressed (4.99 KB, patch)
2010-01-28 09:29 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-01-28 08:08:57 PST
Basically by forward-porting code from the Qt/Maemo branch.
Comment 1 Andreas Kling 2010-01-28 08:12:23 PST
Created attachment 47620 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2010-01-28 08:22:39 PST
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 @@
> +2010-01-28  Andreas Kling  <andreas.kling@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@nokia.com> and
> +        Robert Griebl <rgriebl@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@webkit.org>
>  
>          Reviewed by Simon Hausmann.
Comment 3 Andreas Kling 2010-01-28 09:29:55 PST
Created attachment 47625 [details]
Same patch with issues addressed

How would an infinite loop occur? Can a frame somehow be its own grandparent?
Comment 4 Kenneth Rohde Christiansen 2010-01-28 12:59:38 PST
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.
Comment 5 WebKit Commit Bot 2010-01-28 17:49:24 PST
Comment on attachment 47625 [details]
Same patch with issues addressed

Clearing flags on attachment: 47625

Committed r54034: <http://trac.webkit.org/changeset/54034>
Comment 6 WebKit Commit Bot 2010-01-28 17:49:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Antonio Gomes 2010-01-29 05:37:38 PST
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 ?
Comment 8 Antonio Gomes 2010-01-29 05:39:22 PST
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 ?
Comment 9 Simon Hausmann 2010-02-01 19:50:18 PST
(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