Bug 29867 – [Qt] QWebPage autotest has three failures

• Component: WebKit API
• Status: RESOLVED FIXED

[Simon Hausmann]

Current two tests are failing in tst_qwebpage:

    * testEnablePersistentStorage
    * testOptionalJSObjects

[Simon Hausmann]

testOptionalJSObjects() is also failing. Jedrzej is looking into it :)

[Jędrzej Nowacki]

Created attachment 40363 [details]
idea of patch

Ok, results of "looking"...

testEnablePersistentStorage works after fix for 29844 bug.

test testOptionalJSObjects is broken because of DOMWindow.cpp. I attached patch. 
The patch SHOULD _NOT_ be committed, only reviewed. I think it is good, but it was reverted in bug 28359
(https://bugs.webkit.org/show_bug.cgi?id=28359) because:
"made a change to return 'null' for window.applicationCache when the feature is disabled in the preferences."

According to w3 documentation (http://www.whatwg.org/specs/web-apps/current-work/#application-cache-api) window.applicationCache should return an object to AplicationCache. So I think it is pretty normal to return null if cache is disabled (there is no object).

so, why this patch is bad?

[Kenneth Rohde Christiansen]

I suggest you make that comment to the other bug.

[Jędrzej Nowacki]

Created attachment 40428 [details]
patch - test change

After short discussion with Simon, we decided that test was buggy. It checked undocumented feature. Application cache is tested in Layout Tests, so we should check only Qt API.

[Laszlo Gombos]

(In reply to comment #4)
> Created an attachment (id=40428) [details]
> patch - test change
> 
> After short discussion with Simon, we decided that test was buggy. It checked
> undocumented feature. Application cache is tested in Layout Tests, so we should
> check only Qt API.

Not a reviewer myself but as the author of the test here are some comments:

1./ I'm not sure if there is a need to test interference between QWebPage settings instances. My intention was to test the interference on the JS engine level (as the JS engine is shared to some degree); that is that evaluateJavaScript() able to detect the presence/lack of the particular JS object. This of course only possible if there is a case where evaluateJavaScript() detects that the JS object is not present, which does not seems to be the case for App cache - which brings to my second point.

2./ Layoutests only tests if App cache (when enabled) functions properly. If there is a setting to turn App cache off, than there is to be a definition/spec/test for the case when it is turned off. 

If the 

-QCOMPARE(testFlag(webPage1, QWebSettings::OfflineWebApplicationCacheEnabled, "applicationCache", false), false);

test is removed than there is no behavior test for what happens if App Cache is turned off.

3./ In general JS should be able to detect if a particular JS API is enabled or not; this was also a goal of this test. The proposed patch does not demonstrates this.

----

This bug relates to a recent webkit-dev discussion - https://lists.webkit.org/pipermail/webkit-dev/2009-September/009982.html

For now we should maybe just comment out the failing line as I do agree that it is a priority that all autotests are passing.

[Jędrzej Nowacki]

> Not a reviewer myself but as the author of the test here are some comments:
> 
> 1./ I'm not sure if there is a need to test interference between QWebPage
> settings instances. My intention was to test the interference on the JS engine
> level (as the JS engine is shared to some degree); that is that
> evaluateJavaScript() able to detect the presence/lack of the particular JS
> object. This of course only possible if there is a case where
> evaluateJavaScript() detects that the JS object is not present, which does not
> seems to be the case for App cache - which brings to my second point.

I suppose that testing interference between instances of JSC on QWebPage is enough and it do the work.

> 2./ Layoutests only tests if App cache (when enabled) functions properly. If
> there is a setting to turn App cache off, than there is to be a
> definition/spec/test for the case when it is turned off. 
> 
> If the 
> 
> -QCOMPARE(testFlag(webPage1, QWebSettings::OfflineWebApplicationCacheEnabled,
> "applicationCache", false), false);
> 
> test is removed than there is no behavior test for what happens if App Cache is
> turned off.
> 3./ In general JS should be able to detect if a particular JS API is enabled or
> not; this was also a goal of this test. The proposed patch does not
> demonstrates this.
> 

I agree it is bad.

> 
> This bug relates to a recent webkit-dev discussion -
> https://lists.webkit.org/pipermail/webkit-dev/2009-September/009982.html
> 

Thanks for link, it was really useful.
------------------------------------------------------------

Ok. First I will try to divide the test to simpler cases and than fight the problem.

[Simon Hausmann]

Comment on attachment 40428 [details]
patch - test change

We should be using an XFAIL instead, as also suggested by Laszlo (implicitly :-) and agreed with in the office :)

[Jędrzej Nowacki]

Created attachment 40864 [details]
xfail

xfail patch

[WebKit Commit Bot]

Comment on attachment 40864 [details]
xfail

Rejecting patch 40864 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
mitting to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKit/qt/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/libexec/git-core//git-svn line 469

[Simon Hausmann]

Committed r49294: <http://trac.webkit.org/changeset/49294>