Bug 102801

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?

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

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

(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

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

Created attachment 175242 [details] preliminary patch.

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?

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

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?

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

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

(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

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

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

Created attachment 175413 [details] Patch

Created attachment 175414 [details] Patch

Comment on attachment 175414 [details] Patch isn't it worth adding layout tests?

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

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

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

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

Comment on attachment 175414 [details] Patch Clearing flags on attachment: 175414 Committed r135399: <http://trac.webkit.org/changeset/135399>

All reviewed patches have been landed. Closing bug.

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

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