[webkit-reviews] review denied: [Bug 74601] [Qt][WK2] Pinch zoom should affect the page size : [Attachment 119622] 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 119622: proposed patch
https://bugs.webkit.org/attachment.cgi?id=119622&action=review

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