Bug 87236 – [Qt][WK2] Fix failing qmltests::FitToView::test_basic()

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

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

[Kenneth Rohde Christiansen]

I'm already looking into this.

[Kenneth Rohde Christiansen]

I'm already looking into this.

[Kenneth Rohde Christiansen]

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.

[zalan]

Created attachment 143792 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

Sorry for the repeated messages.

[Kenneth Rohde Christiansen]

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

[zalan]

Created attachment 143799 [details]
Patch

[Caio Marcelo de Oliveira Filho]

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.

[Caio Marcelo de Oliveira Filho]

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

[Caio Marcelo de Oliveira Filho]

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

[Kenneth Rohde Christiansen]

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

[zalan]

Created attachment 143870 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[zalan]

Created attachment 143989 [details]
Patch

[WebKit Review Bot]

Comment on attachment 143989 [details]
Patch

Clearing flags on attachment: 143989

Committed r118493: <http://trac.webkit.org/changeset/118493>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

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.

[Kenneth Rohde Christiansen]

Created attachment 144490 [details]
Patch

[WebKit Review Bot]

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.

[Caio Marcelo de Oliveira Filho]

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?

[Simon Hausmann]

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?

[Kenneth Rohde Christiansen]

Created attachment 144504 [details]
Patch

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

Committed in 118758