Need negative size view tests similar to TouchWebView
Created attachment 109179 [details] Added test to cover netgaive size view Test is similar to TouchWebView. Here is the result: ********* Start testing of qmltests ********* PASS : DesktopWebViewLoad::initTestCase() QWARN : DesktopWebViewLoad::test_load() QSocketNotifier: socket notifiers cannot be enabled from another thread QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : DesktopWebViewLoad::test_load() PASS : DesktopWebViewLoad::cleanupTestCase() PASS : DesktopWebViewLoadFail::initTestCase() QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : DesktopWebViewLoadFail::test_fail() PASS : DesktopWebViewLoadFail::cleanupTestCase() PASS : DesktopWebViewLoad::initTestCase() QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : DesktopWebViewLoad::test_loadNegativeSizeView() PASS : DesktopWebViewLoad::cleanupTestCase() 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() PASS : DesktopWebViewLoad::initTestCase() QWARN : DesktopWebViewLoad::test_loadZeroSizeView() QSocketNotifier: socket notifiers cannot be enabled from another thread QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : DesktopWebViewLoad::test_loadZeroSizeView() PASS : DesktopWebViewLoad::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() QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : TouchWebViewLoad::test_load() PASS : TouchWebViewLoad::cleanupTestCase() PASS : TouchWebViewLoadFail::initTestCase() SKIP : TouchWebViewLoadFail::test_fail() Fails due to https://bugreports.qt.nokia.com/browse/QTBUG-21537 Loc: [/home/gopal/wk/wk3/Source/WebKit2/UIProcess/API/qt/tests/qmltests/TouchWebView/tst_loadFail.qml(21)] PASS : TouchWebViewLoadFail::test_fail() PASS : TouchWebViewLoadFail::cleanupTestCase() QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : TouchWebViewLoad::initTestCase() QWARN : TouchWebViewLoad::test_loadNegativeSizeView() QSocketNotifier: socket notifiers cannot be enabled from another thread QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : TouchWebViewLoad::test_loadNegativeSizeView() PASS : TouchWebViewLoad::cleanupTestCase() PASS : TouchWebViewLoadProgress::initTestCase() QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : TouchWebViewLoadProgress::test_loadProgress() PASS : TouchWebViewLoadProgress::cleanupTestCase() PASS : TouchWebViewLoadProgressSignal::initTestCase() QWARN : TouchWebViewLoadProgressSignal::test_loadProgressSignal() QSocketNotifier: socket notifiers cannot be enabled from another thread QSocketNotifier: socket notifiers cannot be enabled from another thread PASS : TouchWebViewLoadProgressSignal::test_loadProgressSignal() PASS : TouchWebViewLoadProgressSignal::cleanupTestCase() PASS : TouchWebViewLoad::initTestCase() QWARN : TouchWebViewLoad::test_loadZeroSizeView() QSocketNotifier: socket notifiers cannot be enabled from another thread 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()
I don't understand why we should support negative web view sizes. What's the use case?
Same reason why Benjamin insisted for TouchWebView: to catch regression.
Comment on attachment 109179 [details] Added test to cover netgaive size view View in context: https://bugs.webkit.org/attachment.cgi?id=109179&action=review > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadNegativeSizeView.qml:25 > + compare(webView.width, -400) > + compare(webView.height, -600) Is that generally correct with QML? If you use negative width and height on a QML image element, do you get the element inverted? Having a defined behavior for negative size is a good idea. But supporting negative size is another problem altogether.
Comment on attachment 109179 [details] Added test to cover netgaive size view View in context: https://bugs.webkit.org/attachment.cgi?id=109179&action=review >> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopWebView/tst_loadNegativeSizeView.qml:25 > > Is that generally correct with QML? > If you use negative width and height on a QML image element, do you get the element inverted? > > Having a defined behavior for negative size is a good idea. But supporting negative size is another problem altogether. This is just not needed. Rectangle { width:-200; height:-30000 } is like doing QObject*obj = 0; obj->foo(); it is bad code.
(In reply to comment #5) > This is just not needed. Rectangle { width:-200; height:-30000 } is like doing QObject*obj = 0; obj->foo(); it is bad code. I agree, but does QML raise an error for that at least?
Comment on attachment 109179 [details] Added test to cover netgaive size view Like Alexis and Benjamin I also fail to see the value of a webview with a negative width/height. As such I don't see what value this test adds that is specific to WebKit. (Maybe you want to submit a unit test to the qtdeclarative repository to verify QtQuick's behaviour on negative dimensions?)
Then we should probably remove the test of TouchWebView already landed which test negative values.
I would actually like to modify and rewrite these tests to catch the error. Ideally, with negative values, the load should have failed. What do you think ?
(In reply to comment #9) > I would actually like to modify and rewrite these tests to catch the error. Ideally, with negative values, the load should have failed. > What do you think ? I think it should assert but It should be done in Qt Quick level to my opinion. Not sure if the Qt Quick team want that though. The API allows in theory to set negative size as the width and height are qreal which means double in most cases.
(In reply to comment #7) > (From update of attachment 109179 [details]) > Like Alexis and Benjamin I also fail to see the value of a webview with a negative width/height. As such I don't see what value this test adds that is specific to WebKit. > > (Maybe you want to submit a unit test to the qtdeclarative repository to verify QtQuick's behaviour on negative dimensions?) +1 here. I'm failing to see the point of this test... (In reply to comment #8) > Then we should probably remove the test of TouchWebView already landed which test negative values. +1 here as well. Let's do it while refactoring the qml tests. They need some love after the merge of both WebViews. If no one opposes to, I would like to close this bug.
Closing as invalid based on reviewer comments.