Bug 76275 - [Qt] [WK2] WebView should use Flickable instead of QScroller to handle positioning
Summary: [Qt] [WK2] WebView should use Flickable instead of QScroller to handle positi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Andras Becsi
URL:
Keywords: Qt
Depends on: 77111 77122
Blocks: 74403 76276
  Show dependency treegraph
 
Reported: 2012-01-13 07:49 PST by Andras Becsi
Modified: 2012-02-09 06:47 PST (History)
12 users (show)

See Also:


Attachments
proposed patch (37.08 KB, patch)
2012-01-18 11:18 PST, Andras Becsi
hausmann: review-
Details | Formatted Diff | Diff
proposed patch (41.87 KB, patch)
2012-01-25 07:56 PST, Andras Becsi
hausmann: review-
Details | Formatted Diff | Diff
proposed patch (41.70 KB, patch)
2012-01-27 05:58 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
rebased patch (41.68 KB, patch)
2012-01-27 06:39 PST, Andras Becsi
kenneth: review+
Details | Formatted Diff | Diff
proposed patch v2 (51.43 KB, patch)
2012-02-06 09:28 PST, Andras Becsi
no flags Details | Formatted Diff | Diff
proposed patch (50.57 KB, patch)
2012-02-07 09:51 PST, Andras Becsi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andras Becsi 2012-01-13 07:49:23 PST
The current WebView implementation uses QScroller to manage pan gestures, but other similar items in QML (ListView, GridView) all use Flickable as their base class.
Since we want the same behaviour as those items and want to drop the QWidgets dependency of QScroller (and soon QScroller is going away from Qt5) we need to change WebView and let Flickable handle the input. 
Since we do not want to depend on private API of the qtdeclarative module we cannot use inheritance, instead we have to dynamically instantiate Flickable and redirect events to the internal Flickable instance.
Comment 1 Andras Becsi 2012-01-18 11:18:27 PST
Created attachment 122970 [details]
proposed patch
Comment 2 Andras Becsi 2012-01-18 11:35:29 PST
(In reply to comment #1)
> Created an attachment (id=122970) [details]
> proposed patch

The API can be used the following way to place additional items on the WebView:

    WebView {
        id: webView
        experimental.page.y: 100

        experimental.flickableContent: [
            Rectangle {
                id: topItem
                height: 100
                width: webView.width
                x: webView.experimental.contentX
                color: "red"
                Text {
                    text: "<b><i>topItem</i></b>"
                    font.pointSize: 24
                    color: "white"
                    anchors {
                        horizontalCenter: parent.horizontalCenter
                        verticalCenter: parent.verticalCenter
                    }
                }
                anchors {
                    bottom: webView.experimental.page.top
                }
            }
        ]
        experimental.contentWidth: experimental.page.width
        experimental.contentHeight: experimental.page.height + topItem.height
...
    } //WebView

The contentItem is sized to the page by default until the user binds contentWidth and contentHeight.

I a follow-up patch I could add a search engine toolbar to MiniBrowser for example to demonstrate the usage.
The exposed Flickable API subset can of course be extended later as needed.
Comment 3 Kenneth Rohde Christiansen 2012-01-18 11:37:21 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

i need to look at this in detail tomorrow

> Source/WebKit2/ChangeLog:20
> +        The current WebView implementation uses QScroller to manage positioning but other similar
> +        items in QML (ListView, GridView) all use Flickable as their base class.
> +        Since QScroller will be dropped from Qt5 this patch removes the QScroller code
> +        and redirects pan gesures to a dynamically created internal Flickable instance (flickProvider)
> +        which handles the positioning.
> +        Since this implementation only uses public QML API it does not depend on declarative-private
> +        and propagates a small subset of the Flickable API as the public API of the WebView.
> +        This minimalistic API is accessible via the experimental extension and makes it possible
> +        in QML to add additional items (such as toolbars, scroll indicators and floating menus)
> +        aroud the page and use anchoring and binding to position them on the flickable contentItem
> +        of the WebView.
> +        After this change QtWebKit2 does no longer depend on QtWidgets and this dependency can be
> +        removed in a follow-up patch.

Could use some shorter sentences and some newlines.. very hard to read. Also has spelling errors, like gesures

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:216
> +    Q_D(const QQuickWebView);
> +    ASSERT(!d->flickProvider.isNull());
> +    return d->flickProvider->property("contentX").value<qreal>();

Seems like some template could be used here.... so much similar code!

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:269
> +    component.setData("import QtQuick 2.0\nFlickable {}", QUrl());

ieck

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:957
> +    return d->flickProvider->property("flickableData").value< QDeclarativeListProperty<QObject> >();

the first space is not needed

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:975
> +    Q_D(QQuickWebView);
> +    d->useDefaultContentWidth = false;

what is default content width? or what is this for? why would I override?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:234
> +    Q_PROPERTY(QQuickItem *contentItem READ contentItem CONSTANT)

style

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:80
> -        emit engine->contentResumeRequested();
> -
>          // Make sure that tiles all around the viewport will be requested.
>          emit engine->viewportTrajectoryVectorChanged(QPointF());
> +
> +        emit engine->contentResumeRequested();

Please explain this change in the changelog, or as a comment here

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:353
> +    if (cssScale == boundedCSSScale) {
> +        // Let the flickable move the content back into valid boundaries if the scale is valid.

That could be explained better.

// Our current scale value is valid, and we thus let the flickable enforce a valid position.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:354
> +        m_viewport->returnToBounds();

I don't like that name :-) Is that common in flickable?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:381
> +    // Flickable currently does not handle touch events directly, so we need to construct mouse events.

WTF! Can't you fix that is flickable?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:457
> +    // Immediately stop kinetic scrolling and if the view is out of bounds
> +    // move it back inside valid bounds immediately as well.
> +    m_viewport->returnToBounds();

Does this actually do that... ie, no animations?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:478
> -    // Stopping the scroller immediately stops kinetic scrolling and if the view is out of bounds it
> -    // is moved inside valid bounds immediately as well. This is the behavior that we want.
> -    scroller()->stop();
> +    interruptScrollAnimation();

why is the comment gone? why is the comment not needed
Comment 4 Kenneth Rohde Christiansen 2012-01-18 11:39:00 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=122970) [details] [details]
> > proposed patch
> 
> The API can be used the following way to place additional items on the WebView:

Is this similar to other uses of flickable? or is this your api? Do you have an example on how to use this with a normal flickable?
Comment 5 Allan Sandfeld Jensen 2012-01-18 12:30:38 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:246
> -    m_content->setPos(-boundPosition(endPosRange.topLeft(), newPos, endPosRange.bottomRight()));
> +    QPointF newPosition = -boundPosition(endPosRange.topLeft(), newPos, endPosRange.bottomRight());
> +    m_viewport->setContentX(newPosition.x());
> +    m_viewport->setContentY(newPosition.y());

Can the new position still be set with just one call for clarity?

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:468
> -    scroller()->handleInput(QScroller::InputPress,  viewportTouchPoint, eventTimestampMillis);
> +    handleInput(QEvent::TouchBegin, viewportTouchPoint, eventTimestampMillis);

Considering handleInput just converts touch-events into mouse-events, is this really needed? Could flickable be instructed more directly, and these functions be removed completely?
Comment 6 Tor Arne Vestbø 2012-01-19 01:55:19 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:382
> +    switch (eventType) {

not a full review, jus a quick comment: Samuel has added touch->mouse mocking in Qt itself, which is enabled by default, so this is probably not needed.
Comment 7 Kenneth Rohde Christiansen 2012-01-19 02:12:09 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:151
> +void QQuickWebView::returnToBounds()
> +{
> +    Q_D(QQuickWebView);
> +    ASSERT(!d->flickProvider.isNull());
> +    QMetaObject::invokeMethod(d->flickProvider.data(), "returnToBounds", Qt::DirectConnection);
> +}

If this is animating, how does the interaction engine know? It needs to.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:450
> +    return m_viewport->isFlicking();

IS this true when it is bouncing back? should it be?
Comment 8 Kenneth Rohde Christiansen 2012-01-19 02:20:02 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=122970) [details] [details]
> > proposed patch
> 
> The API can be used the following way to place additional items on the WebView:
> 
>     WebView {
>         id: webView
>         experimental.page.y: 100
> 
>         experimental.flickableContent: [

Is this a standard way of doing it? Why is it better to use this than just attach something to the page? the API seems so separate.

Why is it inherent flickable if I dont do things like x:webView.experimental.contentX ? Wouldn't a name like experimental.overlay not make more sense?


>             Rectangle {
>                 id: topItem
>                 height: 100
>                 width: webView.width
>                 x: webView.experimental.contentX

Can't we use experimental.page.x ? or is page like the viewport inside our view? ieck.

>                 color: "red"
>                 Text {
>                     text: "<b><i>topItem</i></b>"
>                     font.pointSize: 24
>                     color: "white"
>                     anchors {
>                         horizontalCenter: parent.horizontalCenter
>                         verticalCenter: parent.verticalCenter
>                     }
>                 }
>                 anchors {
>                     bottom: webView.experimental.page.top
>                 }
>             }
>         ]
>         experimental.contentWidth: experimental.page.width
>         experimental.contentHeight: experimental.page.height + topItem.height
> ...
>     } //WebView
> 
> The contentItem is sized to the page by default until the user binds contentWidth and contentHeight.

Can you explain this better? I dont get it.
Comment 9 Andras Becsi 2012-01-19 02:51:44 PST
(In reply to comment #3)
> (From update of attachment 122970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review
Comment on the quesitons

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:354
> > +        m_viewport->returnToBounds();
> 
> I don't like that name :-) Is that common in flickable?

Yes, this is how flickable calls it :)

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:381
> > +    // Flickable currently does not handle touch events directly, so we need to construct mouse events.
> 
> WTF! Can't you fix that is flickable?

The normal usecase for flickable would be to use a mousearea but we just need flickable as an engine. However as torarne said the touch-> mouse mocking is already in Qt, so if we have that hash we can omit this part and pass the touch event directly. I have to check.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:457
> > +    // Immediately stop kinetic scrolling and if the view is out of bounds
> > +    // move it back inside valid bounds immediately as well.
> > +    m_viewport->returnToBounds();
> 
> Does this actually do that... ie, no animations?
You are right, this animates, and currently I see no way to actually move the content back immediately, except using our setItemRectVisible, and patching qquickflickable to make cancelFlick invocable. 

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:478
> > -    // Stopping the scroller immediately stops kinetic scrolling and if the view is out of bounds it
> > -    // is moved inside valid bounds immediately as well. This is the behavior that we want.
> > -    scroller()->stop();
> > +    interruptScrollAnimation();
> 
> why is the comment gone? why is the comment not needed
interruptScrollAnimation and panGestureCancelled did exactly the same thing, and they even had the exact same comment.
Comment 10 Andras Becsi 2012-01-19 03:00:22 PST
(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=122970) [details] [details] [details]
> > > proposed patch
> > 
> > The API can be used the following way to place additional items on the WebView:
> 
> Is this similar to other uses of flickable? or is this your api? Do you have an example on how to use this with a normal flickable?

The API is a subset of the Flickable QML API, as you can see in the patch. 
The only difference is that we need to put everything into experimental, which hinders us from using the DefaultProperty to hide the list syntax and automatically parent the items to the contentItem of the Flickable.

We could add the default property to the experimental object, but I think in the current state being explicit here is better than hiding the actual logic.
To simplify my above example a bit:

experimental {
    flickableContent: [
     ...
    ]
}

would also do the trick, and this way the additional experimental cruft can be omitted.
Comment 11 Andras Becsi 2012-01-19 03:05:02 PST
(In reply to comment #5)
> (From update of attachment 122970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:246
> > -    m_content->setPos(-boundPosition(endPosRange.topLeft(), newPos, endPosRange.bottomRight()));
> > +    QPointF newPosition = -boundPosition(endPosRange.topLeft(), newPos, endPosRange.bottomRight());
> > +    m_viewport->setContentX(newPosition.x());
> > +    m_viewport->setContentY(newPosition.y());
> 
> Can the new position still be set with just one call for clarity?

Unfortunately it can not be set with one call because these Flickable functions do more than just positioning, they also reset the state of the Flickable.
I could introduce a wrapper but it would still need to call these two functions on the Flickable and would thus only add a third function call.

> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:468
> > -    scroller()->handleInput(QScroller::InputPress,  viewportTouchPoint, eventTimestampMillis);
> > +    handleInput(QEvent::TouchBegin, viewportTouchPoint, eventTimestampMillis);
> 
> Considering handleInput just converts touch-events into mouse-events, is this really needed? Could flickable be instructed more directly, and these functions be removed completely?

See torarne's comment. We can probably probably drop this part.
Comment 12 Andras Becsi 2012-01-19 03:11:14 PST
(In reply to comment #7)
> (From update of attachment 122970 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:151
> > +void QQuickWebView::returnToBounds()
> > +{
> > +    Q_D(QQuickWebView);
> > +    ASSERT(!d->flickProvider.isNull());
> > +    QMetaObject::invokeMethod(d->flickProvider.data(), "returnToBounds", Qt::DirectConnection);
> > +}
> 
> If this is animating, how does the interaction engine know? It needs to.
> 
> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:450
> > +    return m_viewport->isFlicking();
> 
> IS this true when it is bouncing back? should it be?

I believe it is true, since it is emitted by QQuickFlickable::movementEnding which is called if the timeline gets inactive, so at the end of the animation.
Comment 13 Andras Becsi 2012-01-19 03:25:43 PST
(In reply to comment #8)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=122970) [details] [details] [details]
> > > proposed patch
> > 
> > The API can be used the following way to place additional items on the WebView:
> > 
> >     WebView {
> >         id: webView
> >         experimental.page.y: 100
> > 
> >         experimental.flickableContent: [
> 
> Is this a standard way of doing it? Why is it better to use this than just attach something to the page? the API seems so separate.
> 
> Why is it inherent flickable if I dont do things like x:webView.experimental.contentX ? Wouldn't a name like experimental.overlay not make more sense?
I do not understand this question.

> 
> 
> >             Rectangle {
> >                 id: topItem
> >                 height: 100
> >                 width: webView.width
> >                 x: webView.experimental.contentX
> 
> Can't we use experimental.page.x ? or is page like the viewport inside our view? ieck.
No, we cannot. Flickable has a contentItem and all the items put into the flickable are the children of this contentItem, which is positioned according to the input. So page.x would be the position of the page on the contentItem and not the position of the contentItem.

Flickable::contentX  in turn is just -contentItem->x(), so we no longer need to invert coordinates, as we did with QScroller.
 
> > The contentItem is sized to the page by default until the user binds contentWidth and contentHeight.
> 
> Can you explain this better? I dont get it.

In Flickable you have to set contentWidth and contentHeight in QML else you won't be able to pan the content.
The decision for the WebView was to resize the contentItem automatically to the page size (this is useDefaultContentWidth and useDefaultContentHeight) so we have the old behaviour without changing the MiniBrowser implementation.
But if the user places additional items around the page with experimental.flickableContent then he needs to make sure he also sets the contentWidth and contentHeight accordingly.

This is the:
        experimental.contentWidth: experimental.page.width
        experimental.contentHeight: experimental.page.height + topItem.height

part in my example. After this we do not use the default size any more, instead we use the bindings set by the user to size the contentItem. Flickable uses the contentItem to calculate the allowed panning boundaries.
Comment 14 Michael Brüning 2012-01-19 03:51:44 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

>>> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:246
>>> +    m_viewport->setContentY(newPosition.y());
>> 
>> Can the new position still be set with just one call for clarity?
> 
> Unfortunately it can not be set with one call because these Flickable functions do more than just positioning, they also reset the state of the Flickable.
> I could introduce a wrapper but it would still need to call these two functions on the Flickable and would thus only add a third function call.

I think what Allan meant was whether we could wrap these two calls in one method (e.g. setContentPosition(QPointF)) instead of directly wrapping the calls to Flickable's setContentX and setContentY. 
But there might be a reason to keep those separate.
Comment 15 Andras Becsi 2012-01-19 04:22:58 PST
> Why is it inherent flickable if I dont do things like x:webView.experimental.contentX ? Wouldn't a name like experimental.overlay not make more sense?
> I do not understand this question.
But let me try to answer it nonetheless

x:webView.experimental.contentX

is needed to always position that toolBox inside the view, also if a pinch gesture is ongoing, but let it move vertically with the page.
We cannot anchor to the view, because the view is not a sibling nor the direct parent of the item.
Comment 16 Simon Hausmann 2012-01-19 13:23:55 PST
Comment on attachment 122970 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122970&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:270
> +    flickProvider.reset(qobject_cast<QQuickItem*>(component.create()));

Does this have to be a QScopedPointer when we also have the parent items delete children?
Comment 17 Simon Hausmann 2012-01-23 12:55:46 PST
Comment on attachment 122970 [details]
proposed patch

There are plenty of good comments in the review that justify another iteration of this patch. Taking it out of the review queue :)
Comment 18 Andras Becsi 2012-01-25 07:56:41 PST
Created attachment 123940 [details]
proposed patch
Comment 19 Andras Becsi 2012-01-25 08:08:55 PST
(In reply to comment #18)
> Created an attachment (id=123940) [details]
> proposed patch

This change needs the QQuickCanvas fix which is already staged, an enabled touch->mouse mocking, and also needs a public invokable cancelFlick method on QQuickFlickable, which I'm going to push ASAP, so it can hopefully show up in the Friday hashes.
Comment 20 Kenneth Rohde Christiansen 2012-01-25 14:09:50 PST
Comment on attachment 123940 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123940&action=review

> Source/WebKit2/ChangeLog:28
> +
> +

double newline

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:138
> +template<typename T> T QQuickWebViewPrivate::readFlickableProperty(const char* name) const

I believe this is normally written as two lines.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:295
> +    component.setData("import QtQuick 2.0\nFlickable {}", QUrl());

comment?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1027
> +QDeclarativeListProperty<QObject> QQuickWebViewExperimental::flickableContent()

Is this the name used by the rest of QML or something you invented?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:115
> +
> +    void returnToBounds();
> +    void cancelFlick();

// Implements Flickable API ?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:88
> +    template<typename T> T readFlickableProperty(const char* name) const;
> +    void writeFlickableProperty(const char* name, const QVariant& value);

I guess you could declare those two as inline free functions (maybe in an anonymous namespace) and avoid cluttering the private with them. Donno if that is a good idea or not :-)

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:174
> +    // These make sure that the contentItem is sized to the page
> +    // if the user did not add other flickable items in QML.
> +    // If the user adds items in QML he has to make sure to
> +    // also bind the contentWidth and contentHeight accordingly.

Is this in accordance with normal QML flickable behaviour? You could add the answer to the comment :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:176
> +    bool usePageWidthAsContentWidth;
> +    bool usePageHeightAsContentHeight;

The name makes me thing "oh when wouldn't I use that"? So maybe we could call them useRawPageWidth...

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:372
> +    // Reset the velocity samples of the flickable.

That is a cryptic comment for someone not familiar with the implementation

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:422
> +    // The pan gesture recognizer sends the current TouchUpdate event
> +    // when it recognized a pan gesture.
> +    // Construct a TouchBegin event out of the received event for the flickable.

a little consistency in when to break lines, please :-) At least in the same group

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:503
> +        // Sending the event to the flickProvider will stop the kinetic scrolling.

...animation.
Comment 21 Simon Hausmann 2012-01-26 03:56:13 PST
Comment on attachment 123940 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123940&action=review

Let's do another round with comments from me and Kenneth :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:148
> +template<typename T> T QQuickWebViewPrivate::readFlickableProperty(const char* name) const
> +{
> +    ASSERT(flickProvider);
> +    return flickProvider->property(name).value<T>();
> +}
> +
> +void QQuickWebViewPrivate::writeFlickableProperty(const char* name, const QVariant& value)
> +{
> +    ASSERT(flickProvider);
> +    flickProvider->setProperty(name, value);
> +}

I think the Qt naming conventions here would suggest

    T flickableProperty(const char* name) const

and

    void setFlickableProperty(const char* name, const QVariant& value)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:150
> +void QQuickWebView::handleInput(QInputEvent *event)

I suggest to rename this method. The name is too generic for what it does, i.e. a keyboard event is input (it's a QInputEvent), but this function doesn't handle it but just forwards it to the flickable. 

I don't like that this is a "public" method in QQuickWebView, but I guess that's easier than storing the private in the touch interaction engine for the moment.

I suggest to rename it to something more specific, such as handleTouchFlickEvent(QTouchEvent*); (yes, take a touch event, very specifically)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:154
> +    qApp->notify(d->flickProvider, event);

I think the correct way of writing this would be

    QCoreApplication::sendEvent(d->flickProvider, event);

It is functionally equivalent, but it doesn't require the instance itself and it's the pattern that is more common instead of calling notify directly.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:161
> +    QMetaObject::invokeMethod(d->flickProvider, "returnToBounds", Qt::DirectConnection);

Perhaps it would be worth it to resolve the QMetaMethod after flick provider creation and therefore avoid a string method lookup for every invocation.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:294
> +    QDeclarativeEngine engine(webView);
> +    QDeclarativeComponent component(&engine, webView);

Ugh, creating a separate engine and then reparenting the object out of it is ugly.

Could you try this instead?

QDeclarativeEngine* engine = qmlEngine(webView);

I _think_ the engine will be available already at that point.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:295
>> +    component.setData("import QtQuick 2.0\nFlickable {}", QUrl());
> 
> comment?

As well as QByteArrayLiteral("import QtQuick...") please :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1083
> +qreal QQuickWebViewExperimental::contentY() const
> +{
> +    Q_Q(const QQuickWebView);
> +    return q->contentY();
> +}

Instead of QQuickWebViewExperimental::contentY() calling QQuickWebView::contentY() which then calls QQuickWebViewPrivate::readFlickableProperty() I think it would be better
to eliminate the QQuickWebView::contentY() indirection.

The QQuickWebView::contentY() function - and the other methods like that - are only called from one place, QQuickWebViewExperimental. The QQuickWebView::contentY() function also
does _not_ represent public API of any sort, so I think it's easier to get rid of it and just have

qreal QQuickWebViewExperimental::contentY() const
{
    return d->flickableProperty<qreal>("contentY");
}

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:237
> +    Q_PROPERTY(qreal contentWidth READ contentWidth WRITE setContentWidth NOTIFY contentWidthChanged)
> +    Q_PROPERTY(qreal contentHeight READ contentHeight WRITE setContentHeight NOTIFY contentHeightChanged)
> +    Q_PROPERTY(qreal contentX READ contentX WRITE setContentX NOTIFY contentXChanged)
> +    Q_PROPERTY(qreal contentY READ contentY WRITE setContentY NOTIFY contentYChanged)
> +    Q_PROPERTY(QQuickItem* contentItem READ contentItem CONSTANT)
> +    Q_PROPERTY(QDeclarativeListProperty<QObject> flickableContent READ flickableContent)

I would be nice to have a comment here saying that the set of these properties are essentially "QML Flickable" API.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:88
>> +    void writeFlickableProperty(const char* name, const QVariant& value);
> 
> I guess you could declare those two as inline free functions (maybe in an anonymous namespace) and avoid cluttering the private with them. Donno if that is a good idea or not :-)

I would also prefer the template methods to be inline. Out-of-line template methods do not really work, but in this case they "do" because it's only used in one compilation unit.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:111
> -    void scrollStateChanged(QScroller::State);
> +    void contentMovingChanged();

I admit that I liked the term scrollStateChanged over contentMovingChanged. But that's just me :)
Comment 22 Tor Arne Vestbø 2012-01-26 05:10:56 PST
> > Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:176
> > +    bool usePageWidthAsContentWidth;
> > +    bool usePageHeightAsContentHeight;
> 
> The name makes me thing "oh when wouldn't I use that"? So maybe we could call them useRawPageWidth...

Changing it to "raw" does not remove the "when do I use that" question. And it adds "what does raw imply?", so IMHO it's a worse name.
Comment 23 Andras Becsi 2012-01-27 05:58:58 PST
Created attachment 124299 [details]
proposed patch
Comment 24 Kenneth Rohde Christiansen 2012-01-27 06:13:26 PST
Comment on attachment 124299 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124299&action=review

much better

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:114
> +    // Internal Flickable API used by the interaction engine

Dont hit me :-) but guess what is wrong with that line.

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p_p.h:176
> +    bool usePageWidthAsContentWidth;
> +    bool usePageHeightAsContentHeight;

What about something like "contentWidthIsOverridden" ?
Comment 25 Andras Becsi 2012-01-27 06:39:18 PST
Created attachment 124306 [details]
rebased patch
Comment 26 Andras Becsi 2012-01-27 11:28:11 PST
Comment on attachment 124306 [details]
rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124306&action=review

This patch is pending on a qtdeclarative patch not being in our hashes yet. It could be resolved beginning of next week.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:427
> +    m_viewport->handleTouchFlickEvent(&touchBegin);

I realized that the timestamp is not set on this touch begin event. I'm going to fix this next week.
Comment 27 Andras Becsi 2012-02-06 09:28:15 PST
Created attachment 125663 [details]
proposed patch v2

Refactored the previous patch and moved the glue code into its separate class not to pollute the qquickwebview.cpp.
This depends on the componentComplete patch and most probably won't apply without it to trunk.

Kenneth, Simon could you take another look?
Compared to the r+-ed patch - apart from the separation of the QtFlickProvider - in this patch all the methods and properties are resolved.
Comment 28 Simon Hausmann 2012-02-07 05:36:35 PST
Comment on attachment 125663 [details]
proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=125663&action=review

Mostly stylistic feedback, overall looks good to me. The only part that I think might be wrong is the passing of the QTouchEvent to ignore() it, but I might be missing something :)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:517
> +    QObject::connect(flickProvider, SIGNAL(contentWidthChanged()), q->experimental(), SIGNAL(contentWidthChanged()), Qt::DirectConnection);
> +    QObject::connect(flickProvider, SIGNAL(contentHeightChanged()), q->experimental(), SIGNAL(contentHeightChanged()), Qt::DirectConnection);
> +    QObject::connect(flickProvider, SIGNAL(contentXChanged()), q->experimental(), SIGNAL(contentXChanged()), Qt::DirectConnection);
> +    QObject::connect(flickProvider, SIGNAL(contentYChanged()), q->experimental(), SIGNAL(contentYChanged()), Qt::DirectConnection);

Minor thing: I suggest to get the d->experimental() pointer once instead of calling it for every connect() :)

Is the Qt::DirectConnection necessary?

> Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:144
> +    event->ignore();

I don't understand this part.

In the "best" case we receive a native touch even in QtWebPageEventHandler::touchEvent(QTouchEvent* event), which will create a NativeWebTouchEvent.
The NativeWebTouchEven will create a _copy_ of the QTouchEvent and provide a const QTouchEvent* via the nativeEvent() accessor. AFAICS that pointer is
passed all the way through here - the constness being casted away in the meantime - and then we call ignore() on the event. But that even is just a copy
that the NativeWebTouchEvent has and I don't think we do anything with the accepted/ignored result. In fact judging from the code in QtWebPageEventHandler
it seems we always accept touch events:


void QtWebPageEventHandler::touchEvent(QTouchEvent* event)
{
#if ENABLE(TOUCH_EVENTS)
    QTransform fromItemTransform = m_webPage->transformFromItem();
    m_webPageProxy->handleTouchEvent(NativeWebTouchEvent(event, fromItemTransform));
    event->accept();
#else
    ASSERT_NOT_REACHED();
    event->ignore();
#endif
}

(And with "best" case I mean if the page doesn't have any event listeners for touch. If it does we even more so operate on a copy of the original Qt touch event
as part of the asynchronous handling)

> Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:198
> +    return flickableProperty<bool>(m_flickable, m_moving);

This is a matter of taste, but now that you have the QMetaMethod I'd personally just use the API directly there:

    return m_moving.read(m_flickable).value<bool>();

The line is about the same length, but now you don't need the helper functions anymore because you've already resolved
the property and you're nicely encapsulating the flickable in QtFlickProvider (a very nice move, btw!)

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.h:27
> +#include <QtGui/QTouchEvent>

This is just me being picky, but I suggest that for new code we use #include <Classname> instead of #include <Module/Classname>. The Module part is only needed in header files that are part of the public API.
Comment 29 Andras Becsi 2012-02-07 09:51:06 PST
Created attachment 125865 [details]
proposed patch
Comment 30 Kenneth Rohde Christiansen 2012-02-07 13:29:39 PST
Comment on attachment 125865 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125865&action=review

> Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:91
> +    m_returnToBounds = resolveMetaMethod(m_flickable, "returnToBounds()");
> +    m_cancelFlick = resolveMetaMethod(m_flickable, "cancelFlick()");

I would like these to be called something like m_returnToBoundFn or so m_returnToBoundMethod or so, to make it a bit more clear in the rest of the code
Comment 31 Andras Becsi 2012-02-08 03:04:19 PST
(In reply to comment #30)
> (From update of attachment 125865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125865&action=review
> 
> > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:91
> > +    m_returnToBounds = resolveMetaMethod(m_flickable, "returnToBounds()");
> > +    m_cancelFlick = resolveMetaMethod(m_flickable, "cancelFlick()");
> 
> I would like these to be called something like m_returnToBoundFn or so m_returnToBoundMethod or so, to make it a bit more clear in the rest of the code

I agree, the Method suffix is needed for these. Can fix that when landing.
Comment 32 Kenneth Rohde Christiansen 2012-02-08 03:35:56 PST
You didnt answer this question from Simon:

Is the Qt::DirectConnection necessary?
Comment 33 Andras Becsi 2012-02-08 04:07:24 PST
(In reply to comment #32)
> You didnt answer this question from Simon:
> 
> Is the Qt::DirectConnection necessary?

I believe I answered it in the patch ;)
Comment 34 Andras Becsi 2012-02-09 06:35:17 PST
Comment on attachment 125865 [details]
proposed patch

Committed in http://trac.webkit.org/changeset/107232.
Comment 35 Andras Becsi 2012-02-09 06:41:33 PST
Comment on attachment 125865 [details]
proposed patch

The minimum required Qt5 hash for trunk is 11bdb40bfda68291b9767b7908a2ab7dc3b75786 after this change.
Comment 36 Andras Becsi 2012-02-09 06:47:49 PST
(In reply to comment #35)
> (From update of attachment 125865 [details])
> The minimum required Qt5 hash for trunk is 11bdb40bfda68291b9767b7908a2ab7dc3b75786 after this change.
The correct hash is of course 63d681ee575c28c972f709af4a1c2b35e03064d8 for Qt5 :)