Bug 73994

Summary: [Qt][WK2] Fix tst_QQuickWebView::scrollRequest() API test
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: Tools / TestsAssignee: Michael Brüning <michael.bruning>
Status: RESOLVED FIXED    
Severity: Normal CC: ahf, kenneth, michael.bruning, ossy, webkit.review.bot, zoltan
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 70236    
Attachments:
Description Flags
Proposed patch
none
Patch to fix the problem. none

Description Csaba Osztrogonác 2011-12-07 03:48:49 PST
FAIL!  : tst_QQuickWebView::scrollRequest() 'webView()->page()->pos().y() == y' returned FALSE. ()
   Loc: [/ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp(267)]
QWARN  : tst_QQuickWebView::show() QQuickCanvas: platform does not support threaded rendering!
Comment 1 Csaba Osztrogonác 2011-12-07 03:50:31 PST
The QWARN line is unrelated to this bug, I pastes it accidentally.
Comment 2 Kenneth Rohde Christiansen 2011-12-07 04:08:00 PST
Source/WebKit2/UIProcess/API/qt/tests/html/scroll.html is in there twice, weird.

This seems to be related to the suspended check. 

Cc'ing Alex as he looked into that yesterday.
Comment 3 Michael Brüning 2011-12-07 04:15:57 PST
It already happened before Alex's patch landed (saw it yesterday)...
Comment 4 Michael Brüning 2011-12-07 04:21:21 PST
I'll take a look at it.
Comment 5 Michael Brüning 2011-12-08 09:05:20 PST
Created attachment 118401 [details]
Proposed patch

Patch to fix the test case and correct the html for the test.
Reloading test case etc coming up separately.
Comment 6 Kenneth Rohde Christiansen 2011-12-08 14:10:33 PST
Comment on attachment 118401 [details]
Proposed patch

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

r+ as this is an improvement, but this could be even better

> Source/WebKit2/UIProcess/API/qt/tests/html/scroll.html:26
> +This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page This is a test page 

There are other ways to make the page long.... for instance a div with height set. (color as well would be preferred)

> Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:263
> +    QTest::qWait(200);

?This seems more like a workaround. Why is there no comment why this is needed, as it is not obvious?

> Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:273
> +    int y = -qRound(50 * webView()->page()->scale());
>      QVERIFY(webView()->page()->pos().y() == y);
>  }

Please add a reload of the page here and verify again that it is at the right place. Can be another patch.
Comment 7 WebKit Review Bot 2011-12-09 07:19:20 PST
Comment on attachment 118401 [details]
Proposed patch

Clearing flags on attachment: 118401

Committed r102453: <http://trac.webkit.org/changeset/102453>
Comment 8 WebKit Review Bot 2011-12-09 07:19:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Csaba Osztrogonác 2011-12-12 03:11:32 PST
Reopen, because this test still fails on the bot.
Comment 10 Michael Brüning 2011-12-21 07:00:29 PST
Created attachment 120176 [details]
Patch to fix the problem.

Found that the problem was actually in a wrong/missing viewport definition, which cause the page to be scaled and not scrollable in xvfb.
Added a viewport meta tag and found out that parts of the previous patch to the test weren't necessary, hence reverted those (see Source/WebKit2/UIProcess/API/qt/test/qquickwebview/tst_qquickwebview.cpp for details).
Removed unnecessary "This is a test page" text and corrected width and height in the div style definition. 

Tested on host and in xvfb.
Comment 11 WebKit Review Bot 2011-12-21 09:03:30 PST
Comment on attachment 120176 [details]
Patch to fix the problem.

Clearing flags on attachment: 120176

Committed r103410: <http://trac.webkit.org/changeset/103410>
Comment 12 WebKit Review Bot 2011-12-21 09:03:37 PST
All reviewed patches have been landed.  Closing bug.