[webkit-reviews] review denied: [Bug 74601] [Qt][WK2] Pinch zoom should affect the page size : [Attachment 119868] proposed patch

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Andras Becsi
<abecsi at webkit.org>'s request for review:
Bug 74601: [Qt][WK2] Pinch zoom should affect the page size
https://bugs.webkit.org/show_bug.cgi?id=74601

Attachment 119868: proposed patch
https://bugs.webkit.org/attachment.cgi?id=119868&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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 :)