Bug 74601 - [Qt][WK2] Pinch zoom should affect the page size
Summary: [Qt][WK2] Pinch zoom should affect the page size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Andras Becsi
URL:
Keywords: Qt
Depends on: 75542
Blocks: 74403
  Show dependency treegraph
 
Reported: 2011-12-15 04:32 PST by Andras Becsi
Modified: 2012-01-09 04:35 PST (History)
6 users (show)

See Also:


Attachments
proposed patch (59.76 KB, patch)
2011-12-16 09:35 PST, Andras Becsi
hausmann: review-
Details | Formatted Diff | Diff
proposed patch (57.71 KB, patch)
2011-12-19 08:39 PST, Andras Becsi
hausmann: review-
Details | Formatted Diff | Diff
proposed patch (61.36 KB, patch)
2011-12-20 06:44 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2011-12-15 04:32:32 PST
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.
Comment 1 Andras Becsi 2011-12-16 09:35:43 PST
Created attachment 119622 [details]
proposed patch
Comment 2 WebKit Review Bot 2011-12-16 09:39:06 PST
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.
Comment 3 Simon Hausmann 2011-12-19 05:47:23 PST
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.
Comment 4 Andras Becsi 2011-12-19 08:39:51 PST
Created attachment 119868 [details]
proposed patch
Comment 5 Andras Becsi 2011-12-19 09:36:10 PST
(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.
Comment 6 Simon Hausmann 2011-12-20 01:49:24 PST
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 :)
Comment 7 Kenneth Rohde Christiansen 2011-12-20 02:19:39 PST
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.
Comment 8 Andras Becsi 2011-12-20 06:44:13 PST
Created attachment 120023 [details]
proposed patch
Comment 9 Andras Becsi 2011-12-20 06:53:48 PST
(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.
Comment 10 Andras Becsi 2011-12-20 07:06:12 PST
(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.
Comment 11 Kenneth Rohde Christiansen 2011-12-21 04:02:17 PST
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?
Comment 12 Andras Becsi 2012-01-04 05:26:21 PST
Committed r104028: <http://trac.webkit.org/changeset/104028>
Comment 13 Andras Becsi 2012-01-04 05:28:46 PST
Comment on attachment 120023 [details]
proposed patch

Clearing flags.
Comment 14 Andras Becsi 2012-01-04 06:39:44 PST
Reopen and investigate the impact on desktop behaviour which is used by WTR.
Comment 15 Andras Becsi 2012-01-09 04:35:59 PST
Committed r104450: <http://trac.webkit.org/changeset/104450>