loadProgres tests are missing for DesktopWebView
Created attachment 107724 [details] DesktopWebView loadProgress test Added test to check loadProgress in DesktopWebView
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 > +2011-09-16 Gopal Raghavan <gopal.1.raghavan@nokia.com> > + [Qt] QDesktopWebView missing loadProgress tests Missing newline between these two lines.
Created attachment 107853 [details] updated patch with reviewer comments Fixed changelog.
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?
Created attachment 107869 [details] updated patch with reviewer comments Updated patch with reviewer comments. Cleanup loadProgress test. Added new test to check loadProgress signal.
Created attachment 107871 [details] updated patch with reviewer comments Updated with reviewer comments. - cleanup loadProgress test - added new test for loadProgress signal
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.
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.
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.
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
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)?
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?
(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.
Comment on attachment 107894 [details] updated patch with reviewer comments Clearing flags on attachment: 107894 Committed r95537: <http://trac.webkit.org/changeset/95537>
All reviewed patches have been landed. Closing bug.