Bug 76275

Created attachment 122970 [details] proposed patch

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

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

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

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

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

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

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

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

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

Created attachment 123940 [details] proposed patch

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

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

Created attachment 124299 [details] proposed patch

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

Created attachment 124306 [details] rebased patch

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.

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

Created attachment 125865 [details] proposed patch

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

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

You didnt answer this question from Simon: Is the Qt::DirectConnection necessary?

(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 on attachment 125865 [details] proposed patch Committed in http://trac.webkit.org/changeset/107232.

Comment on attachment 125865 [details] proposed patch The minimum required Qt5 hash for trunk is 11bdb40bfda68291b9767b7908a2ab7dc3b75786 after this change.

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