Bug 66664 – [Qt][WK2] Add the animations on the ViewportInteractionEngine

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Benjamin Poulain]

The ViewportInteractionEngine is supposed to animated the content back into the viewport. Currently it just jumps back to position.

[Benjamin Poulain]

Created attachment 104891 [details]
Patch

Here is the current patch.
It should work ok-ish but I had no time to clean it and I am going home.

[Kenneth Rohde Christiansen]

Comment on attachment 104891 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104891&action=review

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:208
> +void ViewportInteractionEngine::emitContentGeometryChangedWithCurrentGeometry()

current viewport geometry? That name is a big mouthful :-)

Anyway, you are using this in two places only and it might be more clear to actually just have the code there. Then noone will be in doubt about the coord system.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:282
> -void ViewportInteractionEngine::animateContentPositionIntoBoundariesIfNeeded()
> +void ViewportInteractionEngine::animateContentIntoBoundariesIfNeeded()

So they had to be joined after all?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:289
> +    const qreal boundedScale = qBound(m_constraints.minimumScale, currentScale, m_constraints.maximumScale);

Can't you copy our innerBound and outerBound methods from the N9 browser? We are bound to use these in more places and we are going to allow zooming a bit further out than the min scale for instance and then showing a bouncing back animation.

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:299
> +        // Revert the change in order to animated both the scale and position.

Something wrong with the English here. I guess you mean 'animate'

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:338
> +        // Already commit final visible rect so new tiles can already be rendered.

two already's in there. // Commit the final visible rect already now, so that new tiles can be rendered immediately  ??

[Benjamin Poulain]

Uh, that was not for review Kenneth, the patch is unfinished, check the comment :-D
Feel free to take over.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> Uh, that was not for review Kenneth, the patch is unfinished, check the comment :-D
> Feel free to take over.

If you are not going to finish it, I can work on it next week.

[Andras Becsi]

I'll finish this.

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> I'll finish this.

Great :-) I'll try to review and give comments.

[Misha]

(In reply to comment #6)
> (In reply to comment #5)
> > I'll finish this.
> 
> Great :-) I'll try to review and give comments.

Why do we need two animations?
Brining the page into boundaries after pan (flick) should be done by kinetic (physics) engine (which we hopefully will bring in). So the only case when we need to animateContentIntoBoundariesIfNeeded() is when we "overzoom". In this case we need to change scale and pos at the same time, so we can use one property animation. Am I missing something?

[Benjamin Poulain]

(In reply to comment #7)
> Brining the page into boundaries after pan (flick) should be done by kinetic (physics) engine (which we hopefully will bring in). So the only case when we need to animateContentIntoBoundariesIfNeeded() is when we "overzoom". In this case we need to change scale and pos at the same time, so we can use one property animation. Am I missing something?

In the end, ViewportInteractionEngine will be the so called "physics engine".

[Misha]

(In reply to comment #8)
> (In reply to comment #7)
> > Brining the page into boundaries after pan (flick) should be done by kinetic (physics) engine (which we hopefully will bring in). So the only case when we need to animateContentIntoBoundariesIfNeeded() is when we "overzoom". In this case we need to change scale and pos at the same time, so we can use one property animation. Am I missing something?
> 
> In the end, ViewportInteractionEngine will be the so called "physics engine".

Does it mean that we going to have physics implementation in ViewportInteractionEngine or just call some Qt API (which one?) to do kinetic.

[Benjamin Poulain]

(In reply to comment #9)
> > In the end, ViewportInteractionEngine will be the so called "physics engine".
> 
> Does it mean that we going to have physics implementation in ViewportInteractionEngine or just call some Qt API (which one?) to do kinetic.

At first, the ViewportInteractionEngine should mimic the behavior of QML kinetic scrolling.

This particular bug is probably not the best place to discuss the design of Qt/QtWebKit's kinetic implementation. The mailing list is probably a better choice.

[Andras Becsi]

Created attachment 109166 [details]
proposed patch

[WebKit Review Bot]

Attachment 109166 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 109166 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109166&action=review

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:40
> +bool QtPanGestureRecognizer::recognize(const QTouchEvent* event, double timestamp)

is double what is normally used for timestamps in WebKit?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:33
> +static const int panThrowAnimationThreshold = shortDuration;

Throw?

> Source/WebKit2/UIProcess/qt/ViewportInteractionEngine.cpp:53
> +static inline const QPointF adjustPositionIntoBoundaries(const QSGItem* viewport, const QRectF& requestedGeometry)
> +{

I don't like the way this is done. I would like instead that you calculate the target scale and content point that should be in the center position of the viewport. This is what we are doing in Grob, and it handles more cases, such that you zoom out and move the content up (or down) a lot, and the visible point in the content seen in the center of the viewport will zoom back into the center of the viewport if possible. The code in grob is also a lot easier to understand. This code is really hard to understand

[Simon Hausmann]

Andras, can we look at this together next week? Kenneth just explained to me the idea of animating between points instead of animating the geometries. We should try to re-use some code from Grob here.

[Misha]

How about ViewportInteractionEngine having property - "viewableRect" which represent viewport rect in content coord and we animate this property.

The animation is done by animateScaleToHotspot(const QPointF& viewportHotSpot, const qreal destScale) which is invoked from animateContentScaleIntoBoundariesIfNeeded() with viewport center and new scale passed as args. The same function can be re-used for "double tap zoom".
I think it's prety much the same as Kenneth suggested in his comment and what was done in the past.

I'm attaching the patch to illustrate (not for review).
 
Also, I think we don't need animateContentPositionIntoBoundariesIfNeeded() at all. This functionality should be done by the same code that does kinetic and overshoot (and which we currently missing :) ). Does anybody working on this?

Comments?

[Misha]

Created attachment 109228 [details]
zoom animation patch (not for review)

[Andras Becsi]

(In reply to comment #15)
> Also, I think we don't need animateContentPositionIntoBoundariesIfNeeded() at all. This functionality should be done by the same code that does kinetic and overshoot (and which we currently missing :) ). Does anybody working on this?
> 

My patch tries to mimic a kinetic movement although the naming, I realize, is somewhat misleading (startPanThrowAnimation). But it does not overshoot there is a rubberband-like animation only if the user tries to pan further when the content is already at the edge. I just reuse the position animation and corresponding position correction (based on requested geometry) in scaling.

[Andras Becsi]

Comment on attachment 109166 [details]
proposed patch

(In reply to comment #14)
> Andras, can we look at this together next week? Kenneth just explained to me the idea of animating between points instead of animating the geometries. We should try to re-use some code from Grob here.

Sure. I just uploaded the patch to get feedback. Although git diff made the patch hard to understand, I agree.

I basically use the geometry for position correction so the kinetic "pan throw animation", which's intensity is proportional with scale and gesture trajectory does not overshoot. This position correction needs the geometry, which is "dirty" when a zoom animation is ongoing, so I pass the requested geometry basically for position correction.
But using the content position in the center of the viewport for animations should be a more practical approach, if I understand Kenneth's idea correctly.

[Misha]

(In reply to comment #17)
> (In reply to comment #15)
> > Also, I think we don't need animateContentPositionIntoBoundariesIfNeeded() at all. This functionality should be done by the same code that does kinetic and overshoot (and which we currently missing :) ). Does anybody working on this?
> > 
> 
> My patch tries to mimic a kinetic movement although the naming, I realize, is somewhat misleading (startPanThrowAnimation). But it does not overshoot there is a rubberband-like animation only if the user tries to pan further when the content is already at the edge. I just reuse the position animation and corresponding position correction (based on requested geometry) in scaling.


Right, but your code does it only when user pan the page. It wouldn't work if user made a flick, would it? My point was that what is done in animateContentPositionIntoBoundariesIfNeeded() or in your startPanThrowAnimation() should be handled in the same code that handles the flick. Something like QScroller does.

[Andras Becsi]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #15)
> > > Also, I think we don't need animateContentPositionIntoBoundariesIfNeeded() at all. This functionality should be done by the same code that does kinetic and overshoot (and which we currently missing :) ). Does anybody working on this?
> > > 
> > 
> > My patch tries to mimic a kinetic movement although the naming, I realize, is somewhat misleading (startPanThrowAnimation). But it does not overshoot there is a rubberband-like animation only if the user tries to pan further when the content is already at the edge. I just reuse the position animation and corresponding position correction (based on requested geometry) in scaling.
> 
> 
> Right, but your code does it only when user pan the page. It wouldn't work if user made a flick, would it? My point was that what is done in animateContentPositionIntoBoundariesIfNeeded() or in your startPanThrowAnimation() should be handled in the same code that handles the flick. Something like QScroller does.

It should work for a flick gesture too because for the ViewportInteractionEngine a flick is just a fast pan gesture so you get a long trajectory and you scroll fast through the page. So it is already handled by the same code but I might misunderstood your point.

[Misha]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (In reply to comment #15)
> > > > Also, I think we don't need animateContentPositionIntoBoundariesIfNeeded() at all. This functionality should be done by the same code that does kinetic and overshoot (and which we currently missing :) ). Does anybody working on this?
> > > > 
> > > 
> > > My patch tries to mimic a kinetic movement although the naming, I realize, is somewhat misleading (startPanThrowAnimation). But it does not overshoot there is a rubberband-like animation only if the user tries to pan further when the content is already at the edge. I just reuse the position animation and corresponding position correction (based on requested geometry) in scaling.
> > 
> > 
> > Right, but your code does it only when user pan the page. It wouldn't work if user made a flick, would it? My point was that what is done in animateContentPositionIntoBoundariesIfNeeded() or in your startPanThrowAnimation() should be handled in the same code that handles the flick. Something like QScroller does.
> 
> It should work for a flick gesture too because for the ViewportInteractionEngine a flick is just a fast pan gesture so you get a long trajectory and you scroll fast through the page. So it is already handled by the same code but I might misunderstood your point.


Yes I see. At first I missed that it will do flick as well.
But would it be better to use some standard platform component to do kinetics?
We already have QScroller and QSGFlickable and now going to have third implementation? QSGFlickable doesn't have public API but QScroller does, although it has to be re-factored for Qt5.
Any thoughts/comments on this? I'm asking it here because it might affect the way this patch is done.

[Andras Becsi]

(In reply to comment #21)
> But would it be better to use some standard platform component to do kinetics?
> We already have QScroller and QSGFlickable and now going to have third implementation? QSGFlickable doesn't have public API but QScroller does, although it has to be re-factored for Qt5.
> Any thoughts/comments on this? I'm asking it here because it might affect the way this patch is done.

The ViewportInteractionEngine is supposed to handle the animations either by means of predefined constrains or by some Qt / platform kinetics functionality. Since the mentioned functionality is not complete in Qt5 yet the plan is to use constrains for animations to clarify and incorporate the needed functionality into the underlying infrastructure.
Later on if the functionality is fully present in Qt5 / the underlying platform we can switch to that. But this is way out of scope for this particular bug.

[Andras Becsi]

Created attachment 112177 [details]
proposed patch

[Kenneth Rohde Christiansen]

Comment on attachment 112177 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112177&action=review

> Source/WebKit2/ChangeLog:9
> +        QScroller also handles flick gestures and animates overshoot.

Not just overshoot?

> Source/WebKit2/ChangeLog:10
> +        If a pinch gesture ends the scale animation scales the content into boundaries

Missing comma here, which makes this hard to read.

If a pinch gesture ends, the scale animated will scale and reposition the content such that it is within valid boundaries. The content is by default animated such that the content position as seen in the center of the visible viewport stays in the center, if that does not mean that the content would be out of valid bounds.

> Source/WebKit2/ChangeLog:14
> +        Also simplify the ViewportUpdateGuard by having an integer counter instead of
> +        the boolean to defer the update requests.

This patch also simplifies the View... by using reference counting instead of a boolean for defering update requests.

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:40
> +bool QtPanGestureRecognizer::recognize(const QTouchEvent* event, qint64 timestampMS)

Source/WebCore/dom/DOMTimeStamp.h

36 typedef unsigned long long DOMTimeStamp;
37 
38 inline DOMTimeStamp convertSecondsToDOMTimeStamp(double seconds)
39 {
40     return static_cast<DOMTimeStamp>(seconds * 1000.0);
41 }
42 
43 inline double convertDOMTimeStampToSeconds(DOMTimeStamp milliseconds)
44 {
45     return milliseconds / 1000.0;
46 }


DOMTimeStamp eventTimeMillis ?

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:99
> +                m_viewportInteractionEngine->pinchGestureStarted(computeTouchCenter(point1, point2));

I dislike the name of that method (I know it is not yours)

computePinchCenter(finger1, finger2)?

> Source/WebKit2/UIProcess/qt/QtTouchWebPageProxy.cpp:63
> +        qint64 timestampMS = static_cast<qint64>(event.timestamp() * 1000);
> +        m_panGestureRecognizer.recognize(event.nativeEvent(), timestampMS);

Here you could use the methods from DOMTimeStamp

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:29
> +#include <QScrollEvent>
> +#include <QScrollPrepareEvent>
> +#include <QScrollerProperties>

Is that sorted correctly?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:34
> +static const int kScaleAnimationDuration = 400;

Millis?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:51
> +        ++viewportInteractionEngine->m_pendingUpdates;

I would add a () for clarity

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:-70
> -        if (!wasUpdatingContent) {
> -            if (previousPosition != viewportInteractionEngine->m_content->pos()
> -                    || previousSize.width() != viewportInteractionEngine->m_content->width()
> -                    || previousSize.height() != viewportInteractionEngine->m_content->height()
> -                    || previousScale != viewportInteractionEngine->m_content->scale())
> -                emit viewportInteractionEngine->viewportUpdateRequested();

Are we emitting now when nothing might have changed? I think it would make sense to have this as a separate change

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:56
> +        --viewportInteractionEngine->m_pendingUpdates;

same here

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:58
> +            emit viewportInteractionEngine->viewportTrajectoryVectorChanged(QPointF());

I think this line needs a comment

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:59
> +            // We must notify the change so the client can rely on us for all change of geometry.

changes*

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:90
> +void QtViewportInteractionEngine::updateVisibleRect(QVariant visibleRectVariant)

The type already makes it clear that it is a variant

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:92
> +    ViewportUpdateGuard guard(this);

Add a newline after this, so make its purpose more clear

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:94
> +    qreal scale = m_viewport->width() / visibleRect.width();

currentScale?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> +    m_content->setScale(scale);
> +    m_content->setPos(-visibleRect.topLeft() * scale);

Maybe add a 

inline void scaleInPoint(QSGItem*, qreal scale, QPointF inPoint).

On the other hand you could do as we are doing in Grob:

1053 QPointF WebViewport::positionAtTarget(const QPointF& unscaledPosition, const QPointF& viewportHotspot, qreal atAbsoluteScale, bool keepWithinBounds)
1054 {
1055     QPointF position = (unscaledPosition * atAbsoluteScale) - viewportHotspot;
1056 
1057     if (!keepWithinBounds)
1058         return position;
1059 
1060     QRectF visibleArea = visibleRect();
1061     QSizeF validSize = (viewportItem()->size() * (atAbsoluteScale / zoomScale())) - visibleArea.size();
1062 
1063     qreal x = qBound(visibleArea.x(), position.x(), validSize.width());
1064     qreal y = qBound(visibleArea.y(), position.y(), validSize.height());
1065 
1066     return QPointF(x, y);
1067 }

This does everything you need. It scaled a position into a hotspot given a scale. And it considers the bounds if needed (disregarded when pinch zooming)

Use it like this:


QPointF viewportHotspot = visibleCenter();
QPointF position = positionAtTarget(contentsPointInCenter, viewportHotspot, targetScale);

setPos(position);
setScale(targetScale);

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:108
> +    case QAbstractAnimation::Running:
> +        emit viewportTrajectoryVectorChanged(QPointF());
> +        m_pinchViewportUpdateDeferrer = adoptPtr(new ViewportUpdateGuard(this));
> +        break;

I guess this is a bit confusing

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:120
> +        const QRectF contentRect = contentRectInViewportCoordinate(m_content, m_viewport);

coordinate*s*

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:124
> +        // QScroller scrolls the content (the viewport moves in the direction of the input).
> +        // To get a panning behaviour (eg. moving the content in the direction of the input)
> +        // the coordinates need to be inverted and we need to set a position range for the inverted positions.

This can be simplified

// As we want to push the contents and not actually scroll it, we need to invert the positions here.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:125
> +        const QPointF contentPositionRangeTopLeft(qMin<qreal>(0, (contentRect.width() - viewportRect.width()) / 2), 0);

This is a bit difficult to understand

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:127
> +        const QSizeF contentPositionRangeSize(qMax<qreal>(0, contentRect.width() - viewportRect.width())
> +                                              , qMax<qreal>(0, contentRect.height() - viewportRect.height()));

I would write these out

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:136
> +        QScrollEvent* scrollEvent = static_cast<QScrollEvent *>(event);

* placement!

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:139
> +            ViewportUpdateGuard guard(this);

newline after the guard

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:161
> +void QtViewportInteractionEngine::animateContentIntoBoundariesIfNeeded(int animationDuration)

Why do we need teh animationDuration argument. It is not something we want to change

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:168
> +    contentScale = qBound(m_constraints.minimumScale, contentScale, m_constraints.maximumScale);

Please add the innerBoundedScale(...) and outerBoundedScale methods from grob for this. It is a lot more clear

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:189
> +    QRectF viewportRect = m_viewport->boundingRect();
> +    QPointF centerOfInterest = viewportRect.center();
> +    QSizeF contentSize = m_content->boundingRect().size();
> +    QPointF centerOnContent = m_content->mapFromItem(m_viewport, centerOfInterest);
> +    QRectF scaledViewportRect(centerOnContent - centerOfInterest / contentScale, viewportRect.size() / contentScale);
> +
> +    if (scaledViewportRect.right() > contentSize.width())
> +        scaledViewportRect.moveLeft(contentSize.width() - scaledViewportRect.width());
> +    if (scaledViewportRect.bottom() > contentSize.height())
> +        scaledViewportRect.moveTop(contentSize.height() - scaledViewportRect.height());
> +
> +    if (scaledViewportRect.width() > contentSize.width())
> +        scaledViewportRect.moveLeft((contentSize.width() - scaledViewportRect.width()) / 2);
> +    else if (scaledViewportRect.x() < 0)
> +        scaledViewportRect.moveLeft(0);
> +
> +    if (scaledViewportRect.height() > contentSize.height())
> +        scaledViewportRect.moveTop(0);
> +    else if (scaledViewportRect.y() < 0)
> +        scaledViewportRect.moveTop(0);

This must be possible to simplify. Look how smple it is in Grob:

 603     QPointF viewportHotspot = visibleCenter();
 604     QPointF pos = (viewportHotspot + position()) / currentScale;
 605 
 606     QPointF minOffset = viewportHotspot / boundedCurrentScale;
 607     QSizeF contentsSize = viewportItem()->boundingRect().size() / currentScale;
 608     QPointF maxOffset = QPointF(contentsSize.width(), contentsSize.height()) - minOffset;
 609     QPointF boundedPos;
 610     boundedPos.setX(qBound(minOffset.x(), pos.x(), maxOffset.x()));
 611     boundedPos.setY(qBound(minOffset.y(), pos.y(), maxOffset.y()));
 612 
 613     // Bounce back and center contents point in viewport center.
 614     if ((boundedCurrentScale != currentScale) || (boundedPos != pos)) {

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:191
> +    QRectF currentViewportRectOnContent = m_viewport->mapRectToItem(m_content, viewportRect);

I don't get the variable name

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:198
> +        m_scaleAnimation->setEasingCurve(QEasingCurve::OutCubic);

Shouldn't we use the one from the QScrollerProperties?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:298
> +    const QPointF movementOffset = pinchCenterInViewportCoordinate - m_lastPinchCenterInViewportCoordinate;

moveOffset. But isn't it more like the diffPosition

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:301
> +    QPointF newPosition(m_content->pos() + movementOffset - scaleOffset);

use targetAtPosition

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:118
> +    QPointF m_lastPinchCenterInViewportCoordinate;

coordinate*s*

[Andras Becsi]

Created attachment 112327 [details]
proposed patch addressing review comments

[Andras Becsi]

(In reply to comment #24)
> > Source/WebKit2/ChangeLog:9
> > +        QScroller also handles flick gestures and animates overshoot.
> 
> Not just overshoot?

Also flick. Overshoot is at the edges of the boundaries.

ChangeLog issues fixed.

> > Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:40
> > +bool QtPanGestureRecognizer::recognize(const QTouchEvent* event, qint64 timestampMS)
> 
> Source/WebCore/dom/DOMTimeStamp.h

> 
> DOMTimeStamp eventTimeMillis ?
> 
I do not see the point in converting to DOMTimeStamp and then back to milliseconds when we can convert it to milliseconds directly. This simplifies the code in the ViewportInteractionEngine.

> > Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:99
> > +                m_viewportInteractionEngine->pinchGestureStarted(computeTouchCenter(point1, point2));
> 
> I dislike the name of that method (I know it is not yours)
> computePinchCenter(finger1, finger2)?
This could be renamed in a follow-up.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:29
> > +#include <QScrollEvent>
> > +#include <QScrollPrepareEvent>
> > +#include <QScrollerProperties>
> 
> Is that sorted correctly?
Yes, it is.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:34
> > +static const int kScaleAnimationDuration = 400;
> 
> Millis?

Renamed to kScaleAnimationDurationMS.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:51
> > +        ++viewportInteractionEngine->m_pendingUpdates;
> 
> I would add a () for clarity
Done.

> Are we emitting now when nothing might have changed? I think it would make sense to have this as a separate change
> 
Reverted this part of the change.

> I think this line needs a comment
Fixed.

> The type already makes it clear that it is a variant
Fixed.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:96
> > +    m_content->setScale(scale);
> > +    m_content->setPos(-visibleRect.topLeft() * scale);
> 
> Maybe add a 
> 
> inline void scaleInPoint(QSGItem*, qreal scale, QPointF inPoint).
This kind of scaling is not used elsewhere, the codepath is used for the animation only, so having an inline function here seems to be pointless.
I added a comment, though, describing the process.

> This can be simplified
> 
> // As we want to push the contents and not actually scroll it, we need to invert the positions here.
Done.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:125
> > +        const QPointF contentPositionRangeTopLeft(qMin<qreal>(0, (contentRect.width() - viewportRect.width()) / 2), 0);
> 
> This is a bit difficult to understand
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:127
> > +        const QSizeF contentPositionRangeSize(qMax<qreal>(0, contentRect.width() - viewportRect.width())
> > +                                              , qMax<qreal>(0, contentRect.height() - viewportRect.height()));
> 
> I would write these out
Wrote them out.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:161
> > +void QtViewportInteractionEngine::animateContentIntoBoundariesIfNeeded(int animationDuration)
> 
> Why do we need teh animationDuration argument. It is not something we want to change
Was used for deciding whether to animate or apply the change immediately, changed to use the m_userInteractionFlags to decide this.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:168
> > +    contentScale = qBound(m_constraints.minimumScale, contentScale, m_constraints.maximumScale);
> 
> Please add the innerBoundedScale(...) and outerBoundedScale methods from grob for this. It is a lot more clear
Done.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:189
> > +    QRectF viewportRect = m_viewport->boundingRect();
> > +    QPointF centerOfInterest = viewportRect.center();
> > +    QSizeF contentSize = m_content->boundingRect().size();
> > +    QPointF centerOnContent = m_content->mapFromItem(m_viewport, centerOfInterest);
> > +    QRectF scaledViewportRect(centerOnContent - centerOfInterest / contentScale, viewportRect.size() / contentScale);
> > +
> > +    if (scaledViewportRect.right() > contentSize.width())
> > +        scaledViewportRect.moveLeft(contentSize.width() - scaledViewportRect.width());
> > +    if (scaledViewportRect.bottom() > contentSize.height())
> > +        scaledViewportRect.moveTop(contentSize.height() - scaledViewportRect.height());
> > +
> > +    if (scaledViewportRect.width() > contentSize.width())
> > +        scaledViewportRect.moveLeft((contentSize.width() - scaledViewportRect.width()) / 2);
> > +    else if (scaledViewportRect.x() < 0)
> > +        scaledViewportRect.moveLeft(0);
> > +
> > +    if (scaledViewportRect.height() > contentSize.height())
> > +        scaledViewportRect.moveTop(0);
> > +    else if (scaledViewportRect.y() < 0)
> > +        scaledViewportRect.moveTop(0);
> 
> This must be possible to simplify. Look how smple it is in Grob:
> 
Simplified. Now QScroller and the animation uses the same function to calculate the boundaries (calculateBoundariesForScale).

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:198
> > +        m_scaleAnimation->setEasingCurve(QEasingCurve::OutCubic);
> 
> Shouldn't we use the one from the QScrollerProperties?
The curve from QScrollerProperties is used by QSCroller for the kinetic deceleration after a flick gesture and not for the bounce-back (overshoot) at the edges, so we need a different curve here.
 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:298
> > +    const QPointF movementOffset = pinchCenterInViewportCoordinate - m_lastPinchCenterInViewportCoordinate;
> 

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:301
> > +    QPointF newPosition(m_content->pos() + movementOffset - scaleOffset);
> 
> use targetAtPosition
Simplified.

[Kenneth Rohde Christiansen]

Comment on attachment 112327 [details]
proposed patch addressing review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=112327&action=review

> Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:40
> +bool QtPanGestureRecognizer::recognize(const QTouchEvent* event, qint64 timestampMS)

I still prefer the name eventTimestampMillis

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:34
> +static const int kScaleAnimationDurationMS = 400;

Lets use Millis like the rest of webcore

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:52
> +static inline qreal outerBoundedScale(QtViewportInteractionEngine::Constraints constraints, qreal scale)

Why not make it a member and remove the constraisn

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:57
> +        return bindToScaleLimits(qBound(hardMin, scale, hardMax));

Are you sure these should be bounded

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:205
> +    const QPointF contentPositionRangeTopLeft(qMin<qreal>(0, (horizontalOffset) / 2), 0);

(horizontalOffset) ? not really needing () :-)

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:224
> +    QSizeF contentSize = m_content->boundingRect().size();

only used one place... no need for this

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:228
> +    QPointF minOffset = positionBoundaries.topLeft();
> +    QPointF maxOffset = positionBoundaries.bottomRight();

Better put these close to where they are being used

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:231
> +    QPointF viewportHotspotOnContent = m_content->mapFromItem(m_viewport, viewportHotspot);
> +    QPointF position = viewportHotspotOnContent - viewportHotspot / boundedScale;

I would just join these two

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:94
> +    QScroller* scroller() { return QScroller::scroller(this); }

ieck... that is a create method :/

[Kenneth Rohde Christiansen]

> > DOMTimeStamp eventTimeMillis ?

it is a better variable name

> I do not see the point in converting to DOMTimeStamp and then back to milliseconds when we can convert it to milliseconds directly. This simplifies the code in the ViewportInteractionEngine.

Clean API and it is uint64_t is a unsigned long long, plus you can use the nice conversion methods.

[Andras Becsi]

Created attachment 112338 [details]
proposed patch

[Andras Becsi]

(In reply to comment #27)
> > Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:40
> > +bool QtPanGestureRecognizer::recognize(const QTouchEvent* event, qint64 timestampMS)
> 
> I still prefer the name eventTimestampMillis
Renamed.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:34
> > +static const int kScaleAnimationDurationMS = 400;
> 
> Lets use Millis like the rest of webcore
Ditto.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:52
> > +static inline qreal outerBoundedScale(QtViewportInteractionEngine::Constraints constraints, qreal scale)
> 
> Why not make it a member and remove the constraisn
Done.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:57
> > +        return bindToScaleLimits(qBound(hardMin, scale, hardMax));
> 
> Are you sure these should be bounded
Yes. This is how it is done in grob, too.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:205
> > +    const QPointF contentPositionRangeTopLeft(qMin<qreal>(0, (horizontalOffset) / 2), 0);
> 
> (horizontalOffset) ? not really needing () :-)
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:224
> > +    QSizeF contentSize = m_content->boundingRect().size();
> 
> only used one place... no need for this
Done.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:228
> > +    QPointF minOffset = positionBoundaries.topLeft();
> > +    QPointF maxOffset = positionBoundaries.bottomRight();
> 
> Better put these close to where they are being used
Done.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:231
> > +    QPointF viewportHotspotOnContent = m_content->mapFromItem(m_viewport, viewportHotspot);
> > +    QPointF position = viewportHotspotOnContent - viewportHotspot / boundedScale;
> 
> I would just join these two
Done.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:94
> > +    QScroller* scroller() { return QScroller::scroller(this); }
> 
> ieck... that is a create method :/
Added a comment that explains that the function creates a singleton.

[Kenneth Rohde Christiansen]

Comment on attachment 112338 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112338&action=review

Great work!

Even if we later decide to not use QScroller, most of this code is reusable, so r=me

> Source/WebKit2/UIProcess/qt/QtPinchGestureRecognizer.cpp:99
> +                m_viewportInteractionEngine->pinchGestureStarted(computeTouchCenter(point1, point2));

I would like a follow up on this.

[Andras Becsi]

Committed r98460: <http://trac.webkit.org/changeset/98460>

[Andras Becsi]

Comment on attachment 112338 [details]
proposed patch

Clearing flags.

[Csaba Osztrogonác]

Reopen, because it broke API tests:
- before your patch: 68 passed, 4 failed, 1 skipped
- after your patch: 46 passed, 5 failed, 0 skipped

Could you fix it or should we roll out or API test failing doesn't matter?

[Andras Becsi]

(In reply to comment #34)
> Reopen, because it broke API tests:
> - before your patch: 68 passed, 4 failed, 1 skipped
> - after your patch: 46 passed, 5 failed, 0 skipped
> 
> Could you fix it or should we roll out or API test failing doesn't matter?

I'm going to look into this.

[Andras Becsi]

Crashing API tests fixed in http://trac.webkit.org/changeset/98858.
Closing bug.