Bug 74601 – [Qt][WK2] Pinch zoom should affect the page size

• Component: New Bugs
• Status: RESOLVED FIXED

[Andras Becsi]

Currently we are using the scale property of the QQuickWebPage for pinch zoom which does not change the page width and height but applies a scale transform internally.
However for layout and anchoring to work correctly in QML we need pinching to affect the page size.
This can be done by applying the scale to the page size and the transformation matrix of the drawing area, and not touching the scale property of the page itself.
This has a consequence that the page item's coordinate system is no longer reliable, so we need to receive the input events in viewport coordinates and translate them to "web-coordinates" when we create NativeWebEvents.

[Andras Becsi]

Created attachment 119622 [details]
proposed patch

[WebKit Review Bot]

Attachment 119622 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:333:  Missing space before ( in foreach(  [whitespace/parens] [5]
Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:109:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3 in 21 files


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

[Simon Hausmann]

Comment on attachment 119622 [details]
proposed patch

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

The style queue has some comments, and so do I :)

In general I think this is the right thing to do. I'm even starting to think that is may also be a good thing if the _view_ becomes the entity to receive all events from the QQuickCanvas and therefore also has the focus. It's a nice way to encapsulate the fact that we may be using a child item as flickable, with a page somewhere deeper. So if somebody wants to "remotely" control the view with synthetic events, then it's very convenient if the view can be the receiving object.

I'm going to say r- because of the style issues and because I have the strong feeling that the coordinate translations for the events can be made more efficient and easier.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:337
> +    QList<QPointF> webPagePoints;
> +
> +    foreach(QTouchEvent::TouchPoint point, event->touchPoints())
> +        webPagePoints.append(m_viewport->mapToWebCoordinates(point.pos()));
> +
> +
> +    m_webPageProxy->handleTouchEvent(NativeWebTouchEvent(event, webPagePoints));

I'm not a fan of the pattern of passing an extra parameter to the event constructor with the translated points. What this means in practice is

   1) We extract each point from the native Qt event...
   2) ...and call mapToWebCoordinates.
   3) Each invocation will call transformToItem() which creates the same temporary QTransform every time.
   4) The transformed point is appended to a separately allocated temporary array (which btw could be a vector with reservations).
   5) The WK2 even is created from separate coordinates.

I wonder if it would be faster and easier (in terms of readability) to alternatively pass the
transform as a parameter to the event constructor.

[Andras Becsi]

Created attachment 119868 [details]
proposed patch

[Andras Becsi]

(In reply to comment #3)
> (From update of attachment 119622 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119622&action=review
> 
> The style queue has some comments, and so do I :)

Fixed the style issues.

> I'm going to say r- because of the style issues and because I have the strong feeling that the coordinate translations for the events can be made more efficient and easier.
> 
> I wonder if it would be faster and easier (in terms of readability) to alternatively pass the
> transform as a parameter to the event constructor.

Done.

[Simon Hausmann]

Comment on attachment 119868 [details]
proposed patch

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

Nice progress! I think we need another iteration, but this is much better already :)

> Source/WebKit2/Shared/NativeWebTouchEvent.h:49
> -    const QTouchEvent m_nativeEvent;
> +    const QTouchEvent* m_nativeEvent;

I don't think that this is going to work. If the web page has touch event listeners, then touch events received in the ui process will be also sent to the web process. Once the event has been processed there, doneWithTouchEvent will be called on the uiclient, with the a copy of the original NativeWebTouchEvent saved earlier and nativeEvent() will be called in QtWebPageEventHandler::doneWithTouchEvent(). Note that the call on the uiclient is asynchronous with respect to the qt touch event handler.

As you can see, this mechanism relies on the ability to safely create _copies_ of NativeWebTouchEvent instances, which breaks after this change, because the point that is then stored becomes dangling as soon as we return from the original Qt touch event handler function.

> Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:188
> -    Vector<WebPlatformTouchPoint> m_touchPoints;
> +    Vector<WebPlatformTouchPoint, 6> m_touchPoints;

I appreciate this part :)

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36
> +static inline bool isValid(QSizeF size)
> +{
> +    return !size.isNull() && size.isValid();
> +}

This function makes the code hard to read by subtly changing the "meaning" of validity for QSizeF.

You can write

   if (size.isValid())

and with your function in the same file you can write

   if (isValid(size))

and it is _totally_ unclear what the difference between the two is.

AFAICS what you're after is !isEmpty():

   if (!size.isEmpty())

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:248
> +    if (!isValid(scaledSize))
> +        return;

Is there any particular reason to keep the item at its old size in case the new size is empty?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:251
> +    update();

Hmm, are you sure that a call to QQuickItem::update() is needed here?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:271
> +    ASSERT(0 < scale);

Please use ASSERT(scale > 0) - we usually put the constant on the right hand side and the variable on the left.

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:284
> +    qreal unScale = 1 / d->renderingScale;

I think we would rather write

    qreal unScale = 1. / d->renderingScale;

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:295
> +    return toItemTransform;

I wonder if it would be easier to write

return QTransform(scale, 0, 0, 0, scale, 0, x(), y(), 1);

After all this function is called a lot.

What do you think?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:478
>      useTraditionalDesktopBehaviour = enable;
> +    pageView->setUsesTraditionalDesktopBehaviour(enable);

Hmm, would it perhaps make sense to remove the boolean from QQuickWebViewPrivate altogether? I'm not a fan of duplicating state variables :)

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:176
> -        && (ev->pos() - m_lastClick).manhattanLength() < qApp->styleHints()->startDragDistance()) {
> +        && (webPagePoint.toPoint() - m_lastClick).manhattanLength() < qApp->styleHints()->startDragDistance()) {

I think you could save yourself the toPoint() rounding by simply changing the type of m_lastClick to a QPointF.

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:403
> -    ev->ignore();
> +    event->ignore();

Nice catch :)

[Kenneth Rohde Christiansen]

Comment on attachment 119868 [details]
proposed patch

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

> Source/WebKit2/ChangeLog:14
> +        Thus the page item's coordiante system is no longer a direct representation

coordiante? coordinate!

> Source/WebKit2/Shared/NativeWebMouseEvent.h:52
> +    explicit NativeWebMouseEvent(QMouseEvent*, const QTransform& toWebCoordinates, int eventClickCount);

toWebCoordinates is a bad name.

> Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:110
> +WebMouseEvent WebEventFactory::createWebMouseEvent(QMouseEvent* event, const QTransform& toWebCoordinates, int eventClickCount)

I believe it is called the css coordinate system. Also what about the name fromItemTransform ?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:62
> +    if (d->useTraditionalDesktopBehaviour && newGeometry.size() != oldGeometry.size())

I would do a 

if (d-> ...Behavior)
   return;

if (newGeom...

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:243
> +void QQuickWebPage::updateSize()

Souldn't this go into the private?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:254
> +void QQuickWebPage::setContentSize(const QSizeF& size)

And this?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:282
> +QTransform QQuickWebPage::transformFromItem() const

QQuick uses

QTransform    itemTransform ( QQuickItem *, bool * ) const

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:284
> +    qreal unScale = 1 / d->renderingScale;

reverseScale?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:290
> +QTransform QQuickWebPage::transformToItem() const

Can't you just have a itemTransform? and uses .inverted?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage_p_p.h:55
> +    bool useTraditionalDesktopBehaviour : 1;

Does that really pack anything?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:262
> +    const QRectF visibleRectInWebCoordinates = q->mapRectToWebCoordinates(q->boundingRect()).intersected(pageView->boundingRect());

CSSCoordinates?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:775
> +QPointF QQuickWebView::mapToWebCoordinates(const QPointF& pointInViewCoordinates) const

As this is the view, I think mapToWebContent is more clear. There is no web coordinates and the content can be scaled as well, ie a mainframe can have a css scale.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:92
> +    QPointF mapToWebCoordinates(const QPointF& pointInViewCoordinates) const;
> +    QRectF mapRectToWebCoordinates(const QRectF& rectInViewCoordiantes) const;
> +    QPointF mapFromWebCoordinates(const QPointF& pointInWebCoordinates) const;
> +    QRectF mapRectFromWebCoordinates(const QRectF& rectInWebCoordiantes) const;

You have some serious wrong spelling there. I think the arguments are unneeded. You are in the View class and with proper method names, this is all clear.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:180
> +    m_content->setRenderingScale(itemScale);

setContentScale?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:497
> +void QtViewportInteractionEngine::panGestureEnded(const QPointF& touchPointInViewportCoordinates, qint64 eventTimestampMillis)

I think viewportTouchPoint is clear enough. I think this could be cleaned up everywhere

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:139
> +    QQuickWebPage* const m_content;

I think we should soon rename this to m_pageItem or so... and m_viewItem... this is too confusing when reading the code

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:164
> +    m_webPageProxy->handleMouseEvent(NativeWebMouseEvent(ev, toWebCoordinates, /*eventClickCount*/ 0));

Wouldn't something like the following be more understandable

m_webPageProxy->handleMouseEvent(NativeWebMouseEvent(ev, itemTransform, /*eventClickCount*/ 0));

It is ok to understand that an event might already have have a transform from its owner.

[Andras Becsi]

Created attachment 120023 [details]
proposed patch

[Andras Becsi]

(In reply to comment #6)
> (From update of attachment 119868 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119868&action=review
> 
> Nice progress! I think we need another iteration, but this is much better already :)
> 
> > Source/WebKit2/Shared/NativeWebTouchEvent.h:49
> > -    const QTouchEvent m_nativeEvent;
> > +    const QTouchEvent* m_nativeEvent;
> 
> I don't think that this is going to work.
Fixed.

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36
> > +static inline bool isValid(QSizeF size)
> > +{
> > +    return !size.isNull() && size.isValid();
> > +}
> 
> AFAICS what you're after is !isEmpty():
Fixed.

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:248
> > +    if (!isValid(scaledSize))
> > +        return;
> 
> Is there any particular reason to keep the item at its old size in case the new size is empty?

This is a redundant check, removed.

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:251
> > +    update();
> 
> Hmm, are you sure that a call to QQuickItem::update() is needed here?

Not needed, removed.
 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:271
> > +    ASSERT(0 < scale);
> 
> Please use ASSERT(scale > 0) - we usually put the constant on the right hand side and the variable on the left.

Done.

> 
> I wonder if it would be easier to write
> 
> return QTransform(scale, 0, 0, 0, scale, 0, x(), y(), 1);
> 
> After all this function is called a lot.
> 
> What do you think?
Right. Done.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:478
> >      useTraditionalDesktopBehaviour = enable;
> > +    pageView->setUsesTraditionalDesktopBehaviour(enable);
> 
> Hmm, would it perhaps make sense to remove the boolean from QQuickWebViewPrivate altogether? I'm not a fan of duplicating state variables :)
Removed.

> 
> I think you could save yourself the toPoint() rounding by simply changing the type of m_lastClick to a QPointF.

Done.

[Andras Becsi]

(In reply to comment #7)
> (From update of attachment 119868 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119868&action=review
> 
> > Source/WebKit2/ChangeLog:14
> > +        Thus the page item's coordiante system is no longer a direct representation
> 
> coordiante? coordinate!
D'oh, coordiantes were all over the place :)
Thanks.

> 
> > Source/WebKit2/Shared/NativeWebMouseEvent.h:52
> > +    explicit NativeWebMouseEvent(QMouseEvent*, const QTransform& toWebCoordinates, int eventClickCount);
> 
> toWebCoordinates is a bad name.
Renamed to fromItemTransform.
 
> I would do a 
> 
> if (d-> ...Behavior)
>    return;
> 
> if (newGeom...
Done.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:243
> > +void QQuickWebPage::updateSize()
> 
> Souldn't this go into the private?
Done.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:254
> > +void QQuickWebPage::setContentSize(const QSizeF& size)
> 
> And this?
This is also used in the view, so it might be better here.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:282
> > +QTransform QQuickWebPage::transformFromItem() const
> 
> QQuick uses
> 
> QTransform    itemTransform ( QQuickItem *, bool * ) const
I left this transformFromItem, I think itemTransform would make it confusing, since the target is not an item, To/From Item makes it clear.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:290
> > +QTransform QQuickWebPage::transformToItem() const
> 
> Can't you just have a itemTransform? and uses .inverted?
Since inverted() does a matrix inversion, I was afraid it could be slower than creating it, but looks like QTransform has fast paths for simple cases.

> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:262
> > +    const QRectF visibleRectInWebCoordinates = q->mapRectToWebCoordinates(q->boundingRect()).intersected(pageView->boundingRect());
> 
> CSSCoordinates?
Renamed.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:775
> > +QPointF QQuickWebView::mapToWebCoordinates(const QPointF& pointInViewCoordinates) const
> 
> As this is the view, I think mapToWebContent is more clear. There is no web coordinates and the content can be scaled as well, ie a mainframe can have a css scale.
> ...
> You have some serious wrong spelling there.
Renamed and fixed typos.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:180
> > +    m_content->setRenderingScale(itemScale);
> 
> setContentScale?
Renamed.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:497
> > +void QtViewportInteractionEngine::panGestureEnded(const QPointF& touchPointInViewportCoordinates, qint64 eventTimestampMillis)
> 
> I think viewportTouchPoint is clear enough. I think this could be cleaned up everywhere
Renamed.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:139
> > +    QQuickWebPage* const m_content;
> 
> I think we should soon rename this to m_pageItem or so... and m_viewItem... this is too confusing when reading the code
Right, I plan to do this in a follow-up.

[Kenneth Rohde Christiansen]

Comment on attachment 120023 [details]
proposed patch

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

Maybe simon has comments

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1002
> +    if (ev->type() == QEvent::InputMethod)
> +        return false; // This is necessary to avoid an endless loop in connection with QQuickItem::event().

What about looking into this? :) *hint*

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:162
> +    lastPos = webPagePoint;

webContentPoint?

[Andras Becsi]

Committed r104028: <http://trac.webkit.org/changeset/104028>

[Andras Becsi]

Comment on attachment 120023 [details]
proposed patch

Clearing flags.

[Andras Becsi]

Reopen and investigate the impact on desktop behaviour which is used by WTR.

[Andras Becsi]

Committed r104450: <http://trac.webkit.org/changeset/104450>