Bug 103889 – [Qt][WK2] REGRESSION(r135399): It made qmltests::DoubleTapToZoom::test_double_zoomInAndBack() API test fail

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

FAIL!  : qmltests::DoubleTapToZoom::test_double_zoomInAndBack() 'wait for signal contentsScaleCommitted' returned FALSE. ()
   Loc: [/home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_doubleTapToZoom.qml(80)]

[Andras Becsi]

Created attachment 177492 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 177492 [details]
Patch

I had a chat with Allan and maybe it is wrong that we call clearRelativeZoomState() in PageViewportControllerClientQt::didChangeViewportAttributes. I think that the initial intention was to catch when the controller adjust the content to fit because of it being zoomed out too much.
The fact that we are mixing those things together is leading to this kind of bug, so maybe we should put that line somewhere else.

[Andras Becsi]

(In reply to comment #2)
> (From update of attachment 177492 [details])
> I had a chat with Allan and maybe it is wrong that we call clearRelativeZoomState() in PageViewportControllerClientQt::didChangeViewportAttributes. I think that the initial intention was to catch when the controller adjust the content to fit because of it being zoomed out too much.
> The fact that we are mixing those things together is leading to this kind of bug, so maybe we should put that line somewhere else.

In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then.
We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes.

[Andras Becsi]

Comment on attachment 177492 [details]
Patch

Clearing flags on attachment: 177492

Committed r136668: <http://trac.webkit.org/changeset/136668>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Jocelyn Turcotte]

(In reply to comment #3)
> In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then.
> We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes.

loadCommitted might be more appropriate then.
PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about.

[Andras Becsi]

(In reply to comment #6)
> (In reply to comment #3)
> > In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then.
> > We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes.
> 
> loadCommitted might be more appropriate then.
> PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about.

(In reply to comment #6)
> (In reply to comment #3)
> > In the original design we used to clean the stack when applying the initial scale, but a lot of things changed since then.
> > We might need to add a new function to the client interface to be able to explicitly clear the stack without relying on the viewport attributes changes.
> 
> loadCommitted might be more appropriate then.
> PageViewportControllerClientQt::setContentsScale also resets the stack for the initial scale, which might be the case you are talking about.

Yes, but since we defer setting the scale to didRenderFrame we do not actually use the isInitial flag in setContentsScale, so this information is lost.

The problem with load committed is that on complex pages the user might still double-tap _after_ loadCommitted but _before_ pageTransitionViewportReady and the stack would not be reset.
So I guess the right spot for explicitly clearing the scale stack would be in pageTransitionViewportReady, or to be more precise: we should reset if we apply the initial scale.
I will try to clean this up after the release.

[Jocelyn Turcotte]

(In reply to comment #7)
Ok I see, that makes sense.

pageTransitionViewportReady is still something that has no direct effect on the client, the user is still seeing the old page at this point. We could also try to carry the flag through applyScaleAfterRenderingContents.

[Andras Becsi]

(In reply to comment #8)
> (In reply to comment #7)
> Ok I see, that makes sense.
> 
> pageTransitionViewportReady is still something that has no direct effect on the client, the user is still seeing the old page at this point. We could also try to carry the flag through applyScaleAfterRenderingContents.

Yepp, that's what I actually meant by "we should reset if we apply the initial scale."