Bug 69096 – [Qt] QDesktopWebView load negative size view test missing

• Component: WebKit2
• Status: RESOLVED INVALID

[Gopal Raghavan]

Need negative size view tests similar to TouchWebView

[Gopal Raghavan]

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

[Andreas Kling]

I don't understand why we should support negative web view sizes. What's the use case?

[Gopal Raghavan]

Same reason why Benjamin insisted for TouchWebView: to catch regression.

[Benjamin Poulain]

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.

[Alexis Menard (darktears)]

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.

[Jesus Sanchez-Palencia]

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

[Simon Hausmann]

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

[Alexis Menard (darktears)]

Then we should probably remove the test of TouchWebView already landed which test negative values.

[Gopal Raghavan]

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 ?

[Alexis Menard (darktears)]

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

[Jesus Sanchez-Palencia]

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

[Caio Marcelo de Oliveira Filho]

Closing as invalid based on reviewer comments.