Summary: | [Qt][WK2] Infinite loop on history navigation, when panning. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||||||
Component: | WebKit Qt | Assignee: | Andras Becsi <abecsi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abecsi, hausmann, kenneth, menard, webkit.review.bot, zoltan | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 80029, 80425 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
zalan
2012-02-21 08:57:39 PST
The infinite loop happens due to MiniBrowser's touch mocking code also sending TouchBegin/TouchUpdate events for MouseButtonDblClick events, which do not get handled and end up as mouse events again in notify where the loop starts again. I'm currently working on changing the MiniBrowser touch mocking code that it only delivers touch events to the canvas, this way we do not end up in a mess of having both touch and mouse events when using the flickable web view. Created attachment 129248 [details]
proposed patch
Attachment 129248 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:32: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 129251 [details]
proposed patch with corrected style
Comment on attachment 129248 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=129248&action=review > Source/WebKit2/ChangeLog:9 > + Because the WebView item accepts all touch events it receives and sends > + them to the web process before propagating them to the gesture recognizers You don't say whether this is the right thing to do. Is it? > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:124 > + // Do the touch to mouse event conversion for the Flickable here > + // to prevent infinite loops between the WebView and this function. Why not explain how that could happen instead? > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:151 > + // to the Flickable. This makes sure that the Flickable grabbs grabbs??? grabs? :-) > Tools/MiniBrowser/qt/BrowserWindow.cpp:78 > - return rootObject()->property("webview").value<QQuickWebView*>(); > + return m_webView.read(rootObject()).value<QQuickWebView*>(); Could you explain the difference? > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:129 > + // If the event is not meant for the WebView send it directly to the target. , after WebView? the target -> its target? > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:134 > + if (QQuickWebView* webView = browserWindow->webView()) { > + QRectF viewportRect = webView->mapRectToScene(webView->boundingRect()); > + if (viewportRect.isEmpty() || !viewportRect.contains(mouseEvent->localPos())) > + return QGuiApplication::notify(target, event); > + } could it be covered by something else? why would this work in that case? (In reply to comment #5) > (From update of attachment 129248 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=129248&action=review > > > Source/WebKit2/ChangeLog:9 > > + Because the WebView item accepts all touch events it receives and sends > > + them to the web process before propagating them to the gesture recognizers > > You don't say whether this is the right thing to do. Is it? Yes, this is the right thing to do. > > > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:124 > > + // Do the touch to mouse event conversion for the Flickable here > > + // to prevent infinite loops between the WebView and this function. > > Why not explain how that could happen instead? Will do. > > > Source/WebKit2/UIProcess/qt/QtFlickProvider.cpp:151 > > + // to the Flickable. This makes sure that the Flickable grabbs > > grabbs??? grabs? :-) d'oh, thanks. > > > Tools/MiniBrowser/qt/BrowserWindow.cpp:78 > > - return rootObject()->property("webview").value<QQuickWebView*>(); > > + return m_webView.read(rootObject()).value<QQuickWebView*>(); > > Could you explain the difference? Will add a comment about avoiding string look-ups > > > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:134 > > + if (QQuickWebView* webView = browserWindow->webView()) { > > + QRectF viewportRect = webView->mapRectToScene(webView->boundingRect()); > > + if (viewportRect.isEmpty() || !viewportRect.contains(mouseEvent->localPos())) > > + return QGuiApplication::notify(target, event); > > + } > > could it be covered by something else? why would this work in that case? This is a corner case of the desktop use-case an is needed because the WebView is the only item which suppors touch events natively. Previously we always sent both, touch and mouse, which sometimes ended up in the described infinite loop. I also tried doing touch mocking for every mouse event and let Qt convert the touch back to mouse if noone accepted it, but QGuiApplication cashes the global position and button state of the real (initial) mouse event and does not send it again when it was converted back from a touch event. So this is the only solution I see for making the MiniBrowser multi-touch mocking work. Created attachment 129264 [details]
proposed patch
(In reply to comment #6) > (In reply to comment #5) > > could it be covered by something else? why would this work in that case? Sorry, I misunderstood the point of the question. We need a mechanism to prevent touch events being accepted by the WebView in the case of dialogs for example, which depend on mouse events, because they would never be converted to mouse events on the device (and neither on the desktop). As far as I know this issue was also present in grob and was solved with a boolean flag in the WebView, but I think this case would need a better solution in Qt which is out of scope for this change. There are already ongoing discussions about the issues arising from QQuickItems not supporting touch events. Comment on attachment 129264 [details]
proposed patch
Remove from review queue until further discussions have a conclusion on whether or not to support multi-touch mocking in MiniBrowser.
Comment on attachment 129264 [details]
proposed patch
If we create the touch events in MiniBrowserApplication and do not use QWindowSystemInterface to create them we can send the original mouse event in case the touch event was not accepted. I think this would be a better solution than the hit-testing.
(In reply to comment #10) > (From update of attachment 129264 [details]) > If we create the touch events in MiniBrowserApplication and do not use QWindowSystemInterface to create them we can send the original mouse event in case the touch event was not accepted. I think this would be a better solution than the hit-testing. I agree. We do need a workaround for this for the time being (for us to be able to continue development), until QQ accepts touch and mouse events equally without intermediate synthetization. Created attachment 129712 [details]
proposed patch
Created attachment 129877 [details]
updated patch
Comment on attachment 129877 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=129877&action=review r=me with a few quirks :) > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:41 > + static QRectF touchRect(0, 0, 40, 40); I think the static keyboard should be removed. I doubt that there's a performance impact here, and IMO it makes the code harder to read ("What does he mean with static here?") > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:138 > + static QPointF lastPos; Why not make this a member variable? IMHO that's cleaner. > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:147 > + static QPointF lastScreenPos; Same here. > Tools/MiniBrowser/qt/MiniBrowserApplication.cpp:168 > + event->accept(); Perhaps a comment here (as well as below) would be good here, to explain why we _swallow_ the event instead of delivering it. (flickable mess...) Committed r109575: <http://trac.webkit.org/changeset/109575> Comment on attachment 129877 [details]
updated patch
Clearing flags from attachment.
|