Bug 66664

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.

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 ??

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

(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.

I'll finish this.

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

(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?

(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".

(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.

(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.

Created attachment 109166 [details] proposed patch

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.

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

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.

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?

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

(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.

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.

(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.

(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.

(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.

(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.

Created attachment 112177 [details] proposed patch

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*

Created attachment 112327 [details] proposed patch addressing review comments

(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.

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 :/

> > 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.

Created attachment 112338 [details] proposed patch

(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.

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.

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

Comment on attachment 112338 [details] proposed patch Clearing flags.

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?

(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.

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