Bug 74601

Created attachment 119622 [details] proposed patch

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

Created attachment 119868 [details] proposed patch

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

Created attachment 120023 [details] proposed patch

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

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

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

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

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

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