Bug 68280 – [Qt] QDeskWebView missing loadProgress tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Gopal Raghavan]

loadProgres tests are missing for DesktopWebView

[Gopal Raghavan]

Created attachment 107724 [details]
DesktopWebView loadProgress test

Added test to check loadProgress in DesktopWebView

[Andreas Kling]

Comment on attachment 107724 [details]
DesktopWebView loadProgress test

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

Test is fine, but let's not break the ChangeLog.

> Source/WebKit2/ChangeLog:2
> [phone]  Gopal Raghavan  <[e-mail]>
> +        [Qt] QDesktopWebView missing loadProgress tests

Missing newline between these two lines.

[Gopal Raghavan]

Created attachment 107853 [details]
updated patch with reviewer comments

Fixed changelog.

[Andreas Kling]

Comment on attachment 107853 [details]
updated patch with reviewer comments

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadProgress.qml:24
> +        function test_loadProgress() {
> +            compare(spy.count, 0)
> +            var testUrl = Qt.resolvedUrl("../common/test1.html")
> +            webView.load(testUrl)
> +            compare(webView.loadProgress, 0)
> +            spy.wait()
> +            compare(webView.loadProgress, 100)
> +        }

A couple of things:

- The 'testUrl' variable is not necessary, just webView.load(Qt.resolvedUrl(...))
- We should check the value of loadProgress before calling load() as well.
- This test only exercises the READ function of the loadProgress property. Why not also test that we get notified when it changes?

[Gopal Raghavan]

Created attachment 107869 [details]
updated patch with reviewer comments

Updated patch with reviewer comments.
Cleanup loadProgress test.
Added new test to check loadProgress signal.

[Gopal Raghavan]

Created attachment 107871 [details]
updated patch with reviewer comments

Updated with reviewer comments.
- cleanup loadProgress test
- added new test for loadProgress signal

[Alexis Menard (darktears)]

Comment on attachment 107871 [details]
updated patch with reviewer comments

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadProgressSignal.qml:27
> +            spyProgress.wait()

Can't it already be 100 right after this call? So the next line may fail?
Also seems like https://gitorious.org/webkit/webkit/blobs/master/Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_loadProgress.qml has possibly the same problem. In any case look at the touchview and make sure they are in sync.

[Gopal Raghavan]

This is checking loadProgressChanged signal. You are right the progress could hit 100. Typically, I am seeing it give values like 40, 50 etc., I am planning to change that range. Let me upload another patch. Thanks.

[Gopal Raghavan]

Created attachment 107894 [details]
updated patch with reviewer comments

Modified logic to cover edge cases.
This test will catch loadProgressChanged signal between range 0 to 100 and validate it.

[Gopal Raghavan]

Here is the test case results:
********* Start testing of qmltests *********
Config: Using QTest library 5.0.0, Qt 5.0.0
PASS   : DesktopWebViewLoadProgress::initTestCase()
QWARN  : DesktopWebViewLoadProgress::test_loadProgress() QSocketNotifier: socket notifiers cannot be enabled from another thread
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : DesktopWebViewLoadProgress::test_loadProgress()
PASS   : DesktopWebViewLoadProgress::cleanupTestCase()
PASS   : DesktopWebViewLoadProgressSignal::initTestCase()
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : DesktopWebViewLoadProgressSignal::test_loadProgressSignal()
PASS   : DesktopWebViewLoadProgressSignal::cleanupTestCase()
QWARN  : qmltests::UnknownTestFunc() QSocketNotifier: socket notifiers cannot be enabled from another thread
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : DesktopWebViewNavigationPolicyForUrl::initTestCase()
QWARN  : DesktopWebViewNavigationPolicyForUrl::test_ignorePolicy() QObject::startTimer: timers cannot be started from another thread
PASS   : DesktopWebViewNavigationPolicyForUrl::test_ignorePolicy()
PASS   : DesktopWebViewNavigationPolicyForUrl::test_usePolicy()
PASS   : DesktopWebViewNavigationPolicyForUrl::cleanupTestCase()
PASS   : DesktopWebViewProperties::initTestCase()
QWARN  : DesktopWebViewProperties::test_title() QSocketNotifier: socket notifiers cannot be enabled from another thread
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : DesktopWebViewProperties::test_title()
PASS   : DesktopWebViewProperties::test_url()
PASS   : DesktopWebViewProperties::cleanupTestCase()
PASS   : TouchWebViewLoad::initTestCase()
QWARN  : TouchWebViewLoad::test_load() QSocketNotifier: socket notifiers cannot be enabled from another thread
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : TouchWebViewLoad::test_load()
PASS   : TouchWebViewLoad::cleanupTestCase()
PASS   : TouchWebViewLoad::initTestCase()
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : TouchWebViewLoad::test_loadNegativeSizeView()
PASS   : TouchWebViewLoad::cleanupTestCase()
PASS   : TouchWebViewLoadProgress::initTestCase()
QWARN  : TouchWebViewLoadProgress::test_loadProgress() QSocketNotifier: socket notifiers cannot be enabled from another thread
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : TouchWebViewLoadProgress::test_loadProgress()
PASS   : TouchWebViewLoadProgress::cleanupTestCase()
PASS   : TouchWebViewLoad::initTestCase()
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : TouchWebViewLoad::test_loadZeroSizeView()
PASS   : TouchWebViewLoad::cleanupTestCase()
PASS   : TouchWebViewProperties::initTestCase()
QSocketNotifier: socket notifiers cannot be enabled from another thread
PASS   : TouchWebViewProperties::test_title()
PASS   : TouchWebViewProperties::test_url()
PASS   : TouchWebViewProperties::cleanupTestCase()
Totals: 30 passed, 0 failed, 0 skipped
********* Finished testing of qmltests *********
Number of leaked textures: 0
Number of leaked materials: 0
Number of leaked nodes: 0
LEAK: 9 WebContext
Number of leaked items: 0

[Alexis Menard (darktears)]

Comment on attachment 107894 [details]
updated patch with reviewer comments

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

Informal Review : r-

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadProgressSignal.qml:28
> +            compare(true, webView.loadProgress > -1 && webView.loadProgress < 101)

OMG This is just wrong. I said it may be that by the time you finish spyProgress.wait() the progress is already 100. Can't you just do compare(loadProgress != 0)?

[Alexis Menard (darktears)]

Comment on attachment 107894 [details]
updated patch with reviewer comments

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

>> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadProgressSignal.qml:28
>> +            compare(true, webView.loadProgress > -1 && webView.loadProgress < 101)
> 
> OMG This is just wrong. I said it may be that by the time you finish spyProgress.wait() the progress is already 100. Can't you just do compare(loadProgress != 0)?

Or is it possible that you get a first call with 0 as a value?

[Andreas Kling]

(In reply to comment #12)
> (From update of attachment 107894 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107894&action=review
> 
> >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadProgressSignal.qml:28
> >> +            compare(true, webView.loadProgress > -1 && webView.loadProgress < 101)
> > 
> > OMG This is just wrong. I said it may be that by the time you finish spyProgress.wait() the progress is already 100. Can't you just do compare(loadProgress != 0)?
> 
> Or is it possible that you get a first call with 0 as a value?

I rs+ the patch assuming that it works as-is.

[WebKit Review Bot]

Comment on attachment 107894 [details]
updated patch with reviewer comments

Clearing flags on attachment: 107894

Committed r95537: <http://trac.webkit.org/changeset/95537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.