Bug 83033 – [Qt][WK2] Make the WebView a subclass of Flickable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andras Becsi]

The current experiment with the internal Flickable instance showed that the approach of hiding the Flickable raises more problems than it solves and had the conclusion that there is no way around the publicly inheritance from QQuickFlickable.

This also means a dependency to quick-private in all projects that include the WebView header.

This change solves several issues related to event propagation, orientation and integration of the WebView item with other QML items.
The drawback is the dependency to private C++ API of the QtDeclarative module, which the WebView already has in some extent because it makes use of the Qt scene graph render-nodes.

[Andras Becsi]

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

[Andras Becsi]

Created attachment 135350 [details]
proposed patch

[Jocelyn Turcotte]

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?

[Andras Becsi]

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

[Lauro Moura Maranhao Neto]

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

[Jocelyn Turcotte]

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

[Simon Hausmann]

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

[Kenneth Rohde Christiansen]

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);

[Andras Becsi]

Created attachment 135554 [details]
proposed patch

[Andras Becsi]

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

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

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);
}

?

[Andras Becsi]

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

[Kenneth Rohde Christiansen]

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

[Andras Becsi]

Comment on attachment 135554 [details]
proposed patch

Clearing flags on attachment: 135554

Committed r113172: <http://trac.webkit.org/changeset/113172>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Andras Becsi]

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

[Andras Becsi]

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.

[Hugo Parente Lima]

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