Bug 83033

The related discussion thread can be found here: https://lists.webkit.org/pipermail/webkit-qt/2012-March/002592.html

Created attachment 135350 [details] proposed patch

Comment on attachment 135350 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135350&action=review Cool stuff :) Here are some minor comments, looks good to me otherwise. > Source/WebKit2/ChangeLog:23 > + benefits of controlling the exposed API, therefor the experiment had therefor*e* > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:528 > + // Disable mouse events on the flickable web view so we do not > + // select text during pan gestures on platforms which send both > + // touch and mouse events simultaneously. > + q->setAcceptedMouseButtons(Qt::NoButton); > + q->setAcceptHoverEvents(false); I guess this should be temporary? Anyhow it could go in the QQuickWebViewFlickablePrivate constructor as this doesn't relate to web process stuff. > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1394 > + QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::NoButton, Qt::NoButton, Qt::NoModifier); Just to make it relatively more future proof, would Qt::LeftButton instead of Qt::NoButton do the job too? > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:208 > + void handlePress(const QPointF& position, qint64 eventTimestampMillis); > + void handleMove(const QPointF& position, qint64 eventTimestampMillis); > + void handleRelease(const QPointF& position, qint64 eventTimestampMillis); Those should probably be hidden behind QQuickWebViewPrivate. Could you also rename them to something that tells that those event goes to flickable? Something like delegatePressToFlickable, or flickableMousePress maybe. > Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:79 > - interactionEngine()->panGestureStarted(m_touchBegin.data()); > + interactionEngine()->panGestureStarted(touchPoint.pos(), event->timestamp()); You could probably get rid of m_touchBegin. > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:-259 > - QPointF newPos = -m_content->pos(); Ugh, we really had both newPos and newPosition variables in this method?

(In reply to comment #3) > (From update of attachment 135350 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135350&action=review > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:528 > > + // Disable mouse events on the flickable web view so we do not > > + // select text during pan gestures on platforms which send both > > + // touch and mouse events simultaneously. > > + q->setAcceptedMouseButtons(Qt::NoButton); > > + q->setAcceptHoverEvents(false); > > I guess this should be temporary? Anyhow it could go in the QQuickWebViewFlickablePrivate constructor as this doesn't relate to web process stuff. Yes, this is temporal until Shawn's event propagation patch lands to Qt. We can remove it as soon as our Qt version has that patch. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1394 > > + QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::NoButton, Qt::NoButton, Qt::NoModifier); > > Just to make it relatively more future proof, would Qt::LeftButton instead of Qt::NoButton do the job too? Sure. Since we bypass the canvas with the current implementation of Flickable only the position an the timestamp matter, but this could change, so you are right. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:208 > > + void handlePress(const QPointF& position, qint64 eventTimestampMillis); > > + void handleMove(const QPointF& position, qint64 eventTimestampMillis); > > + void handleRelease(const QPointF& position, qint64 eventTimestampMillis); > > Those should probably be hidden behind QQuickWebViewPrivate. > Could you also rename them to something that tells that those event goes to flickable? Something like delegatePressToFlickable, or flickableMousePress maybe. They are already private inside the WebView, and are used internally in the interaction engine. Do you think that making the private class accessible from the interaction engine would be better? But sure, I can rename them. > > > Source/WebKit2/UIProcess/qt/QtPanGestureRecognizer.cpp:79 > > - interactionEngine()->panGestureStarted(m_touchBegin.data()); > > + interactionEngine()->panGestureStarted(touchPoint.pos(), event->timestamp()); > > You could probably get rid of m_touchBegin. Yes, that's part of the gesture recognizer refactoring. I didn't want to touch it in this patch.

Tested this patch on a N9 with snowshoe and the panning orientation issue is gone.

(In reply to comment #4) > > > Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:208 > > > + void handlePress(const QPointF& position, qint64 eventTimestampMillis); > > > + void handleMove(const QPointF& position, qint64 eventTimestampMillis); > > > + void handleRelease(const QPointF& position, qint64 eventTimestampMillis); > > > > Those should probably be hidden behind QQuickWebViewPrivate. > > Could you also rename them to something that tells that those event goes to flickable? Something like delegatePressToFlickable, or flickableMousePress maybe. > > They are already private inside the WebView, and are used internally in the interaction engine. > > Do you think that making the private class accessible from the interaction engine would be better? > > But sure, I can rename them. > Usually we try to make sure that API classes contain only useful public methods, and hide all private implementation methods in the private class to keep the public one clean and help maintaining binary compatibility. We're not planning to make the class public yet so it doesn't matter so much, but I think we should keep it clean anyhow to make it easy if we decide to do so at some point.

Laszlo, do you think this is something you guys can do, too?

Comment on attachment 135350 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135350&action=review > Source/WebKit2/ChangeLog:25 > + the conclusion that there is no way around the public inheritance from > + QQuickFlickable. Which I think is a totally sane approach :-) > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:78 > + , m_useDefaultContentItemSize(true) Wasn't the old name better? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:524 > + // Disable mouse events on the flickable web view so we do not I would add FIXME: Temporary workaround code which should be removed when bug #bbb is fixed > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:780 > +void QQuickWebViewExperimental::setUseDefaultContentItemSize(bool enable) Any example on how this should be used? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1381 > +QPointF QQuickWebView::contentPos() const AS these are not in the flickable API do they make sense? Why isn't contentX and contentY enough? >>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:208 >>> + void handleRelease(const QPointF& position, qint64 eventTimestampMillis); >> >> Those should probably be hidden behind QQuickWebViewPrivate. >> Could you also rename them to something that tells that those event goes to flickable? Something like delegatePressToFlickable, or flickableMousePress maybe. > > They are already private inside the WebView, and are used internally in the interaction engine. > > Do you think that making the private class accessible from the interaction engine would be better? > > But sure, I can rename them. flickableMousePress would be quite clear > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:467 > + m_viewport->handlePress(position, eventTimestampMillis); Why not just have handlePress implemented here in the engine? why pass it to the m_viewport? I mean all you need to call here is m_viewport->mousePressEvent(&mouseEvent);

Created attachment 135554 [details] proposed patch

(In reply to comment #6) > (In reply to comment #4) > Usually we try to make sure that API classes contain only useful public methods, and hide all private implementation methods in the private class to keep the public one clean and help maintaining binary compatibility. We're not planning to make the class public yet so it doesn't matter so much, but I think we should keep it clean anyhow to make it easy if we decide to do so at some point. The functions have to call the base class implementations of a virtual function, and the QQuickWebViewPrivate class can not access this base implementation in QQuickFlickable, so these calls have to be in the QQuickWebView class.

(In reply to comment #8) > (From update of attachment 135350 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135350&action=review > > > Source/WebKit2/ChangeLog:25 > > + the conclusion that there is no way around the public inheritance from > > + QQuickFlickable. > > Which I think is a totally sane approach :-) > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:78 > > + , m_useDefaultContentItemSize(true) > > Wasn't the old name better? Since we do not have glue code any more I had to expose the property in experimental, else we would need to do: contentWidth: experimental.page.width contentHeight: experimental.page.height Since this is how the flickable API works. But for the WebView this behaviour is the default. If the user wants to have additional items on the contentItem, he has to set experimental.useDefaultContentItemSize = false and set the width and height if he wants to add other items to the Flickable. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:524 > > + // Disable mouse events on the flickable web view so we do not > > I would add FIXME: Temporary workaround code which should be removed when bug #bbb is fixed > Done. > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:780 > > +void QQuickWebViewExperimental::setUseDefaultContentItemSize(bool enable) > > Any example on how this should be used? See above. > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1381 > > +QPointF QQuickWebView::contentPos() const > > AS these are not in the flickable API do they make sense? Why isn't contentX and contentY enough? I kept these, since else we would need the same boilerplate code in each function in the interaction engine, which sets the position explicitly. These are not exposed in QML. > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:467 > > + m_viewport->handlePress(position, eventTimestampMillis); > > Why not just have handlePress implemented here in the engine? why pass it to the m_viewport? I mean all you need to call here is m_viewport->mousePressEvent(&mouseEvent); This would trigger an infinite loop, since it would send the event to the web process. We intercep the events to the flickable, and only send mouse events when we want to pan.

Comment on attachment 135554 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135554&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1410 > +void QQuickWebView::handleFlickableMousePress(const QPointF& position, qint64 eventTimestampMillis) > +{ > + QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::LeftButton, Qt::NoButton, Qt::NoModifier); > + mouseEvent.setTimestamp(eventTimestampMillis); > + QQuickFlickable::mousePressEvent(&mouseEvent); > +} so from the InteractionEngine you call this (handleFlickableMousePress) How is that different that InteractionEngine::... { QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::LeftButton, Qt::NoButton, Qt::NoModifier); mouseEvent.setTimestamp(eventTimestampMillis); m_viewport->mousePressEvent(&mouseEvent); } ?

(In reply to comment #12) > (From update of attachment 135554 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=135554&action=review > > > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1410 > > +void QQuickWebView::handleFlickableMousePress(const QPointF& position, qint64 eventTimestampMillis) > > +{ > > + QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::LeftButton, Qt::NoButton, Qt::NoModifier); > > + mouseEvent.setTimestamp(eventTimestampMillis); > > + QQuickFlickable::mousePressEvent(&mouseEvent); > > +} > > so from the InteractionEngine you call this (handleFlickableMousePress) > > How is that different that > > InteractionEngine::... { > QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::LeftButton, Qt::NoButton, Qt::NoModifier); > mouseEvent.setTimestamp(eventTimestampMillis); > m_viewport->mousePressEvent(&mouseEvent); > } > > ? m_viewport->mousePressEvent(&mouseEvent) will send the event to the web process, and _not_ to the Flickable. That is the whole point of the subclassing.

Comment on attachment 135554 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=135554&action=review >>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1410 >>> +} >> >> so from the InteractionEngine you call this (handleFlickableMousePress) >> >> How is that different that >> >> InteractionEngine::... { >> QMouseEvent mouseEvent(QEvent::MouseButtonPress, position, Qt::LeftButton, Qt::NoButton, Qt::NoModifier); >> mouseEvent.setTimestamp(eventTimestampMillis); >> m_viewport->mousePressEvent(&mouseEvent); >> } >> >> ? > > m_viewport->mousePressEvent(&mouseEvent) will send the event to the web process, and _not_ to the Flickable. > That is the whole point of the subclassing. qobject_cast<QQuickFlickable>(m_viewport)->mousePressEvent(&mouseEvent) ?

Comment on attachment 135554 [details] proposed patch Clearing flags on attachment: 135554 Committed r113172: <http://trac.webkit.org/changeset/113172>

All reviewed patches have been landed. Closing bug.

(In reply to comment #14) > (From update of attachment 135554 [details]) > > m_viewport->mousePressEvent(&mouseEvent) will send the event to the web process, and _not_ to the Flickable. > > That is the whole point of the subclassing. > > qobject_cast<QQuickFlickable>(m_viewport)->mousePressEvent(&mouseEvent) ? mousePressEvent and co. are virtual functions, so calling them on a base class pointer would not help.

Ossy, if the next update is comming please make sure that the patch from http://codereview.qt-project.org/22137 is also in the hash. Until then there will be warnings on the MiniBrowser console for touch release, about the WebView not being the mouse grabber item. This warning is harmless, and will disappear with the above patch.

(In reply to comment #17) > (In reply to comment #14) > > (From update of attachment 135554 [details] [details]) > > > m_viewport->mousePressEvent(&mouseEvent) will send the event to the web process, and _not_ to the Flickable. > > > That is the whole point of the subclassing. > > > > qobject_cast<QQuickFlickable>(m_viewport)->mousePressEvent(&mouseEvent) ? > > mousePressEvent and co. are virtual functions, so calling them on a base class pointer would not help. You can call a specific virtual method implementation: qobject_cast<QQuickFlickable>(m_viewport)->QQuickFlickable::mousePressEvent(&mouseEvent)