This test introduced in http://trac.webkit.org/changeset/117942 and fails from the beginning: FAIL! : qmltests::FitToView::test_basic() Compared values are not the same Actual (): 0.5 Expected (): 1 Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_fitToView.qml(96)]
I'm already looking into this.
https://gist.github.com/2780751 fixes this but contains many separate changes. Work is ongoing to upstream this ASAP with help of Zalan and Alex.
Created attachment 143792 [details] Patch
Comment on attachment 143792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143792&action=review r=me with the changes > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:127 > - emit d->viewportItem->experimental()->test()->contentsScaleCommitted(); > + emit d->viewportItem->experimental()->test()->contentsScaleChanged(); unrelated, should be part of another patch > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:930 > - // Emits contentsScaleCommitted(); > + // Emits contentsScaleChanged(); Other patch?
Comment on attachment 143792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143792&action=review > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:297 > + m_hadUserInteraction = true; Question: do we still need to set it to true in the gesture specific functions: panGestureStarted(), pinchGestureStarted()? Suggestion: this seems to be the important fix in the patch, maybe deserve some mention in the ChangeLog why had user interaction was missing here. > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:360 > + ASSERT(m_hadUserInteraction); 8 lines after the code sets m_hadUserInteraction to true again, maybe that's not needed anymore.
Sorry for the repeated messages.
(In reply to comment #8) > (From update of attachment 143792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=143792&action=review > > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:297 > > + m_hadUserInteraction = true; > > Question: do we still need to set it to true in the gesture specific functions: panGestureStarted(), pinchGestureStarted()? They should not be needed anymore, but I am not sure how correct our current code is, so we could replace them with asserts. > Suggestion: this seems to be the important fix in the patch, maybe deserve some mention in the ChangeLog why had user interaction was missing here. the touchBegin was added by Jocelyn to fix a bug, but he forgot to add the = true, which breaks one of our tests.
Created attachment 143799 [details] Patch
Comment on attachment 143799 [details] Patch Sorry. I think at least assigning true after the ASSERT true should be fixed, otherwise people reading this code in the future will get confused.
(In reply to comment #10) > (In reply to comment #8) > > (From update of attachment 143792 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=143792&action=review > > > > > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:297 > > > + m_hadUserInteraction = true; > > > > Question: do we still need to set it to true in the gesture specific functions: panGestureStarted(), pinchGestureStarted()? > > They should not be needed anymore, but I am not sure how correct our current code is, so we could replace them with asserts. If the assignments are redundant I think they should be removed before landing the patch with assertions.
(In reply to comment #13) > If the assignments are redundant I think they should be removed before landing the patch with assertions. the patch, with assertions added in their places.
(In reply to comment #14) > (In reply to comment #13) > > If the assignments are redundant I think they should be removed before landing the patch with assertions. > > the patch, with assertions added in their places. He means from pinch and panGesture...
Created attachment 143870 [details] Patch
Comment on attachment 143870 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143870&action=review r=me with that change > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:-503 > - m_hadUserInteraction = true; Please make these asserts instead. The idea is that touchBegin gets called before panGestureStarted, so lets make sure that never change by making this into an assert ASSERT(m_hadUserInteraction); // Set by perceding touchBegin call. > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:-567 > - m_hadUserInteraction = true; Same here.
Created attachment 143989 [details] Patch
Comment on attachment 143989 [details] Patch Clearing flags on attachment: 143989 Committed r118493: <http://trac.webkit.org/changeset/118493>
All reviewed patches have been landed. Closing bug.
Reopen as the test doesn't pass yet and I have local code to make it do so. Unfortunately this will first be fixed tuesday.
Created attachment 144490 [details] Patch
Attachment 144490 [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/API/qt/qwebkittest_p.h:26: 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.
Comment on attachment 144490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144490&action=review > Source/WebKit2/ChangeLog:7 > + I'm missing a big picture comment about the fix you are making here. What was wrong to make the test fail? > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:887 > + static float lastCommittedScale = -1; If we have two WebViews in the application, won't this static cause trouble?
Comment on attachment 144490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144490&action=review > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:890 > + static float lastCommittedScale = -1; > + if (scale != lastCommittedScale) > + emit q->experimental()->test()->contentsScaleCommitted(); > + lastCommittedScale = scale; Urgh, any chance of making this a member variable instead? ;( Maybe it belongs as a member into the test object? > Source/WebKit2/UIProcess/API/qt/qwebkittest.cpp:57 > +bool QWebKitTest::sendTouchEvent(QQuickWebView* window, QEvent::Type type, QList<QTouchEvent::TouchPoint>& points, ulong timestamp) I don't like APIs that have mutable parameters where you can't see them on the caller side. I would prefer if points was a _pointer_ to a QList, so that it's easier to imagine on the caller side that the list will be modified in this function. Although it doesn't look like that this functionality (removing released touch points) is used anywhere?
Created attachment 144504 [details] Patch
Attachment 144504 [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/API/qt/qwebkittest_p.h:25: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed in 118758