Bug 102801 – [WK2] Viewport meta tag broken after r134801

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Zeno Albisser]

Loading any page that makes use of the viewport meta tag causes flickering and misplaced textures, when resizing the window.
In some cases textures are also being displayed upside down, right after loading the page. (without any resizing)
This might be an initialisation issue with a projection matrix or similar.

[Kenneth Rohde Christiansen]

How did you come to blame this commit? I don't see how it can affect this and everything works perfectly on EFL. Do you have some example pages that triggers this issue, and maybe screenshots?

[Thiago Marcos P. Santos]

Do you have a test page? Works fine here: http://people.opera.com/andreasb/viewport/ex02.html

[Zeno Albisser]

(In reply to comment #1)
> How did you come to blame this commit? I don't see how it can affect this and everything works perfectly on EFL. Do you have some example pages that triggers this issue, and maybe screenshots?
Git bisect.

[Zeno Albisser]

(In reply to comment #2)
> Do you have a test page? Works fine here: http://people.opera.com/andreasb/viewport/ex02.html

http://www.albisser.org/test/viewport.html
http://alteredqualia.com/three/examples/webgl_pasta.html

[Thiago Marcos P. Santos]

(In reply to comment #4)
> (In reply to comment #2)
> > Do you have a test page? Works fine here: http://people.opera.com/andreasb/viewport/ex02.html
> 
> http://www.albisser.org/test/viewport.html

Indeed. This site is broken on Qt and EFL.

[Mikhail Pozdnyakov]

Created attachment 175242 [details]
preliminary patch.

[Kenneth Rohde Christiansen]

Comment on attachment 175242 [details]
preliminary patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=175242&action=review

> Source/WebKit2/ChangeLog:11
> +        Problem turned up as after m_rawAttributes.initialScale was set to '-1', the function
> +        WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(); was invoked from
> +        PageViewportController::didChangeViewportAttributes resetting also min and max scale
> +        factors to '-1'. 

Shouldn't we remove that call instead?

[Andras Becsi]

(In reply to comment #7)
> (From update of attachment 175242 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175242&action=review
> 
> > Source/WebKit2/ChangeLog:11
> > +        Problem turned up as after m_rawAttributes.initialScale was set to '-1', the function
> > +        WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(); was invoked from
> > +        PageViewportController::didChangeViewportAttributes resetting also min and max scale
> > +        factors to '-1'. 
> 
> Shouldn't we remove that call instead?

No, that call is needed. I have a complete patch to fix this issue, uploading it soon.

[Andras Becsi]

The issue is actually a bit more complicated than I thought.

The main problem with leaving the initial scale -1 is that WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable is called by almost all ports (qt, efl, gtk, chromium) and it seems to me, that all callers assume that the scale values in the provided attributes are valid. This was also a reason why I previously introduced a bool flag for this case.

AFAICT the solution for this is one of the following:

- we change all the call sides to calculate the initial scale if it is <=0 and assert in restrictScaleFactorToInitialScaleIfNotUserScalable for initialScale

- we add a fallbackScale parameter to restrictScaleFactorToInitialScaleIfNotUserScalable and use it if initialScale <= 0

- we revert r134801

I think adding a new parameter (second solution) is the proper fix for this.

Kenneth, am I right here?

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> The issue is actually a bit more complicated than I thought.
> 
> The main problem with leaving the initial scale -1 is that WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable is called by almost all ports (qt, efl, gtk, chromium) and it seems to me, that all callers assume that the scale values in the provided attributes are valid. This was also a reason why I previously introduced a bool flag for this case.
> 
> AFAICT the solution for this is one of the following:
> 
> - we change all the call sides to calculate the initial scale if it is <=0 and assert in restrictScaleFactorToInitialScaleIfNotUserScalable for initialScale
> 
> - we add a fallbackScale parameter to restrictScaleFactorToInitialScaleIfNotUserScalable and use it if initialScale <= 0
> 
> - we revert r134801
> 
> I think adding a new parameter (second solution) is the proper fix for this.
> 
> Kenneth, am I right here?

I would also go for number two. Basically restricting the min and max scale factors to the initial one is not part of the viewport meta algorithm, but more of an implementation detail.

Maybe just restrict to the minimum scale value then?

[Mikhail Pozdnyakov]

(In reply to comment #10)
> (In reply to comment #9)
> > The issue is actually a bit more complicated than I thought.
> > 
> > The main problem with leaving the initial scale -1 is that WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable is called by almost all ports (qt, efl, gtk, chromium) and it seems to me, that all callers assume that the scale values in the provided attributes are valid. This was also a reason why I previously introduced a bool flag for this case.
> > 
> > AFAICT the solution for this is one of the following:
> > 
> > - we change all the call sides to calculate the initial scale if it is <=0 and assert in restrictScaleFactorToInitialScaleIfNotUserScalable for initialScale
> > 
> > - we add a fallbackScale parameter to restrictScaleFactorToInitialScaleIfNotUserScalable and use it if initialScale <= 0
> > 
> > - we revert r134801
> > 
> > I think adding a new parameter (second solution) is the proper fix for this.
> > 
> > Kenneth, am I right here?
> 
> I would also go for number two. Basically restricting the min and max scale factors to the initial one is not part of the viewport meta algorithm, but more of an implementation detail.
> 
> Maybe just restrict to the minimum scale value then?
Also would like to add that initial scale is set to '-1' only for WK2 at the moment, think the same approach should be spread for all ports.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > The issue is actually a bit more complicated than I thought.
> > > 
> > > The main problem with leaving the initial scale -1 is that WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable is called by almost all ports (qt, efl, gtk, chromium) and it seems to me, that all callers assume that the scale values in the provided attributes are valid. This was also a reason why I previously introduced a bool flag for this case.
> > > 
> > > AFAICT the solution for this is one of the following:
> > > 
> > > - we change all the call sides to calculate the initial scale if it is <=0 and assert in restrictScaleFactorToInitialScaleIfNotUserScalable for initialScale
> > > 
> > > - we add a fallbackScale parameter to restrictScaleFactorToInitialScaleIfNotUserScalable and use it if initialScale <= 0
> > > 
> > > - we revert r134801
> > > 
> > > I think adding a new parameter (second solution) is the proper fix for this.
> > > 
> > > Kenneth, am I right here?
> > 
> > I would also go for number two. Basically restricting the min and max scale factors to the initial one is not part of the viewport meta algorithm, but more of an implementation detail.
> > 
> > Maybe just restrict to the minimum scale value then?
> Also would like to add that initial scale is set to '-1' only for WK2 at the moment, think the same approach should be spread for all ports.

There is outcommented code in ViewportArguments class to do this for everyone

[Mikhail Pozdnyakov]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > The issue is actually a bit more complicated than I thought.
> > > > 
> > > > The main problem with leaving the initial scale -1 is that WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable is called by almost all ports (qt, efl, gtk, chromium) and it seems to me, that all callers assume that the scale values in the provided attributes are valid. This was also a reason why I previously introduced a bool flag for this case.
> > > > 
> > > > AFAICT the solution for this is one of the following:
> > > > 
> > > > - we change all the call sides to calculate the initial scale if it is <=0 and assert in restrictScaleFactorToInitialScaleIfNotUserScalable for initialScale
> > > > 
> > > > - we add a fallbackScale parameter to restrictScaleFactorToInitialScaleIfNotUserScalable and use it if initialScale <= 0
> > > > 
> > > > - we revert r134801
> > > > 
> > > > I think adding a new parameter (second solution) is the proper fix for this.
> > > > 
> > > > Kenneth, am I right here?
> > > 
> > > I would also go for number two. Basically restricting the min and max scale factors to the initial one is not part of the viewport meta algorithm, but more of an implementation detail.
> > > 
> > > Maybe just restrict to the minimum scale value then?
> > Also would like to add that initial scale is set to '-1' only for WK2 at the moment, think the same approach should be spread for all ports.
> 
> There is outcommented code in ViewportArguments class to do this for everyone
If we do not want to set initial scale to -1 for everyone hence we don't need to touch restrictScaleFactorToInitialScaleIfNotUserScalable, and my "preliminary patch" is acceptable, as it treats WK2 only which is only broken.

[Kenneth Rohde Christiansen]

Comment on attachment 175242 [details]
preliminary patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=175242&action=review

> Source/WebKit2/UIProcess/PageViewportController.cpp:220
>  
>      m_rawAttributes = newAttributes;
> +    if (m_rawAttributes.initialScale < 0)
> +        m_rawAttributes.initialScale = m_minimumScaleToFit;
>      WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(m_rawAttributes);
>  
>      m_allowsUserScaling = !!m_rawAttributes.userScalable;

Are you sure m_minimumScaleToFit is set correctly at this point? I believe this is called before contentSize is changed

[Andras Becsi]

Created attachment 175413 [details]
Patch

[Andras Becsi]

Created attachment 175414 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 175414 [details]
Patch

isn't it worth adding layout tests?

[Andras Becsi]

(In reply to comment #17)
> (From update of attachment 175414 [details])
> isn't it worth adding layout tests?

Layout tests do not use this code-path on Qt (fit-to-width), but extending the QML unit tests would be possible.

[Kenneth Rohde Christiansen]

(In reply to comment #18)
> (In reply to comment #17)
> > (From update of attachment 175414 [details] [details])
> > isn't it worth adding layout tests?
> 
> Layout tests do not use this code-path on Qt (fit-to-width), but extending the QML unit tests would be possible.

It should be possible to test that with layout tests now. Thiago Santos added support for that, though I am not sure the patch for Qt landed yet.

[Thiago Marcos P. Santos]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 175414 [details] [details] [details])
> > > isn't it worth adding layout tests?
> > 
> > Layout tests do not use this code-path on Qt (fit-to-width), but extending the QML unit tests would be possible.
> 
> It should be possible to test that with layout tests now. Thiago Santos added support for that, though I am not sure the patch for Qt landed yet.

Landed on bug 102811.

[Andras Becsi]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (From update of attachment 175414 [details] [details] [details])
> > > isn't it worth adding layout tests?
> > 
> > Layout tests do not use this code-path on Qt (fit-to-width), but extending the QML unit tests would be possible.
> 
> It should be possible to test that with layout tests now. Thiago Santos added support for that, though I am not sure the patch for Qt landed yet.

Indeed, I missed that change.

[Andras Becsi]

Comment on attachment 175414 [details]
Patch

Clearing flags on attachment: 175414

Committed r135399: <http://trac.webkit.org/changeset/135399>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.

[Andras Becsi]

(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > (From update of attachment 175414 [details] [details] [details] [details])
> > > > isn't it worth adding layout tests?
> > > 
> > > Layout tests do not use this code-path on Qt (fit-to-width), but extending the QML unit tests would be possible.
> > 
> > It should be possible to test that with layout tests now. Thiago Santos added support for that, though I am not sure the patch for Qt landed yet.
> 
> Indeed, I missed that change.

Since the preferred event handling behaviour for the flickable webview is not completely established yet, I think using fixed layout for layout testing is still somewhat unreliable on Qt, also because some initialization for Flickable depends on the onComponentComplete signal which is only emitted if the view was instantiated using QML.

So if we add layout tests that use the feature we should be careful with the behaviour.
As soon as we have free resources we're going to address these issues.

[Thiago Marcos P. Santos]

This patch caused regression on the EFL bots:

crash log for WebKitTestRunner (pid 4198):
STDOUT: <empty>
STDERR: ASSERTION FAILED: m_rawAttributes.initialScale > 0
STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/PageViewportController.cpp(152) : void WebKit::PageViewportController::pageTransitionViewportReady()
STDERR: 1   0x7f6af371abfa WebKit::PageViewportController::pageTransitionViewportReady()
STDERR: 2   0x7f6af393e405 WebKit::PageClientDefaultImpl::pageTransitionViewportReady()
STDERR: 3   0x7f6af37703d5 WebKit::WebPageProxy::pageTransitionViewportReady()
STDERR: 4   0x7f6af397672a void CoreIPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::Arguments0 const&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
STDERR: 5   0x7f6af3972d36 void CoreIPC::handleMessage<Messages::WebPageProxy::PageTransitionViewportReady, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)()>(CoreIPC::MessageDecoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)())
STDERR: 6   0x7f6af396c149 WebKit::WebPageProxy::didReceiveWebPageProxyMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 7   0x7f6af376b779 WebKit::WebPageProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 8   0x7f6af37ac895 WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 9   0x7f6af3694d04 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 10  0x7f6af3694e70 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
STDERR: 11  0x7f6af36950bb CoreIPC::Connection::dispatchOneMessage()
STDERR: 12  0x7f6af369f25c WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
STDERR: 13  0x7f6af369f062 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
STDERR: 14  0x7f6afa840dca WTF::Function<void ()>::operator()() const
STDERR: 15  0x7f6af6d09baa WebCore::RunLoop::performWork()
STDERR: 16  0x7f6af77274c7 WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
STDERR: 17  0x7f6af2aa8621
STDERR: 18  0x7f6af2aa7571
STDERR: 19  0x7f6af2aa7ab7 ecore_main_loop_begin
STDERR: 20  0x43362b WTR::TestController::platformRunUntil(bool&, double)
STDERR: 21  0x41e94e WTR::TestController::runUntil(bool&, WTR::TestController::TimeoutDuration)
STDERR: 22  0x41de6b WTR::TestController::resetStateToConsistentValues()
STDERR: 23  0x425ac2 WTR::TestInvocation::invoke()
STDERR: 24  0x41e686 WTR::TestController::runTest(char const*)
STDERR: 25  0x41e7bf WTR::TestController::runTestingServerLoop()
STDERR: 26  0x41e859 WTR::TestController::run()
STDERR: 27  0x41c291 WTR::TestController::TestController(int, char const**)
STDERR: 28  0x4337c6 main
STDERR: 29  0x7f6af160876d __libc_start_main
STDERR: 30  0x41ac09
STDERR: LEAK: 4 WebCoreNode