29867 2009-09-29 06:00:19 -0700 [Qt] QWebPage autotest has three failures 2011-04-28 17:25:04 -0700 1 1 1 Unclassified WebKit WebKit API 528+ (Nightly build) All All RESOLVED FIXED Qt P2 Normal --- 29803 29844 29799 1 hausmann webkit-unassigned cmarcelo jedrzej.nowacki kenneth laszlo.gombos oldest_to_newest 150887 0 hausmann 2009-09-29 06:00:19 -0700 Current two tests are failing in tst_qwebpage: * testEnablePersistentStorage * testOptionalJSObjects 150899 1 hausmann 2009-09-29 07:09:32 -0700 testOptionalJSObjects() is also failing. Jedrzej is looking into it :) 151227 2 40363 jedrzej.nowacki 2009-09-30 05:35:50 -0700 Created attachment 40363 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? 151229 3 kenneth 2009-09-30 05:49:01 -0700 I suggest you make that comment to the other bug. 151481 4 40428 jedrzej.nowacki 2009-10-01 01:39:12 -0700 Created attachment 40428 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. 151833 5 laszlo.gombos 2009-10-02 05:51:01 -0700 (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. 152331 6 jedrzej.nowacki 2009-10-05 09:43:08 -0700 > 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. 153037 7 40428 hausmann 2009-10-07 07:51:47 -0700 Comment on attachment 40428 patch - test change We should be using an XFAIL instead, as also suggested by Laszlo (implicitly :-) and agreed with in the office :) 153336 8 40864 jedrzej.nowacki 2009-10-08 04:23:56 -0700 Created attachment 40864 xfail xfail patch 153346 9 40864 commit-queue 2009-10-08 06:02:55 -0700 Comment on attachment 40864 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 153348 10 hausmann 2009-10-08 06:10:07 -0700 Committed r49294: <http://trac.webkit.org/changeset/49294> 40363 2009-09-30 05:35:50 -0700 2009-10-01 01:39:12 -0700 idea of patch dom.diff text/plain 584 jedrzej.nowacki ZGlmZiAtLWdpdCBhL1dlYkNvcmUvcGFnZS9ET01XaW5kb3cuY3BwIGIvV2ViQ29yZS9wYWdlL0RP TVdpbmRvdy5jcHAKaW5kZXggNWFjNDA0OS4uMDAxYzQ0OCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9w YWdlL0RPTVdpbmRvdy5jcHAKKysrIGIvV2ViQ29yZS9wYWdlL0RPTVdpbmRvdy5jcHAKQEAgLTUz OCw3ICs1MzgsNyBAQCBDb25zb2xlKiBET01XaW5kb3c6OmNvbnNvbGUoKSBjb25zdAogI2lmIEVO QUJMRShPRkZMSU5FX1dFQl9BUFBMSUNBVElPTlMpCiBET01BcHBsaWNhdGlvbkNhY2hlKiBET01X aW5kb3c6OmFwcGxpY2F0aW9uQ2FjaGUoKSBjb25zdAogewotICAgIGlmICghbV9hcHBsaWNhdGlv bkNhY2hlKQorICAgIGlmICghbV9hcHBsaWNhdGlvbkNhY2hlICYmIG1fZnJhbWUgJiYgbV9mcmFt ZS0+c2V0dGluZ3MoKSAmJiBtX2ZyYW1lLT5zZXR0aW5ncygpLT5vZmZsaW5lV2ViQXBwbGljYXRp b25DYWNoZUVuYWJsZWQoKSkgCiAgICAgICAgIG1fYXBwbGljYXRpb25DYWNoZSA9IERPTUFwcGxp Y2F0aW9uQ2FjaGU6OmNyZWF0ZShtX2ZyYW1lKTsKICAgICByZXR1cm4gbV9hcHBsaWNhdGlvbkNh Y2hlLmdldCgpOwogfQo= 40428 2009-10-01 01:39:12 -0700 2009-10-08 04:23:56 -0700 patch - test change appcache.diff text/plain 2403 jedrzej.nowacki ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9DaGFuZ2VMb2cgYi9XZWJLaXQvcXQvQ2hhbmdlTG9nCmlu ZGV4IGIyNTJkOGMuLjZkZmE5NzUgMTAwNjQ0Ci0tLSBhL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysr IGIvV2ViS2l0L3F0L0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDA5LTEwLTAxICBKZWRy emVqIE5vd2Fja2kgIDxqZWRyemVqLm5vd2Fja2lAbm9raWEuY29tPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFFXZWJQYWdlJ3MgdGVzdE9wdGlvbmFs SlNPYmplY3RzIGF1dG90ZXN0IGZpeC4KKyAgICAgICAgCisgICAgICAgIEF1dG90ZXN0IHdhcyBi YXNlZCBvbiBpbnRlcm5hbHMuIEFzIGFwcGxpY2F0aW9uQ2FjaGUKKyAgICAgICAgZmVhdHVyZSBp cyBjb3ZlcmVkIGJ5IExheW91dFRlc3RzLCBvbmx5IFF0aXNoIEFQSSBzaG91bGQgYmUKKyAgICAg ICAgY2hlY2tlZC4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9Mjk4NjcKKworICAgICAgICAqIHRlc3RzL3F3ZWJwYWdlL3RzdF9xd2VicGFnZS5jcHA6 CisgICAgICAgICh0c3RfUVdlYlBhZ2U6OnRlc3RPcHRpb25hbEpTT2JqZWN0cyk6CisKIDIwMDkt MDktMjkgIEFuZHJhcyBCZWNzaSAgPGJlY3NpLmFuZHJhc0BzdHVkLnUtc3plZ2VkLmh1PgogCiAg ICAgICAgIFJldmlld2VkIGJ5IFRvciBBcm5lIFZlc3Riw7guCmRpZmYgLS1naXQgYS9XZWJLaXQv cXQvdGVzdHMvcXdlYnBhZ2UvdHN0X3F3ZWJwYWdlLmNwcCBiL1dlYktpdC9xdC90ZXN0cy9xd2Vi cGFnZS90c3RfcXdlYnBhZ2UuY3BwCmluZGV4IDI4Mzk1MGUuLmMwYjNhODEgMTAwNjQ0Ci0tLSBh L1dlYktpdC9xdC90ZXN0cy9xd2VicGFnZS90c3RfcXdlYnBhZ2UuY3BwCisrKyBiL1dlYktpdC9x dC90ZXN0cy9xd2VicGFnZS90c3RfcXdlYnBhZ2UuY3BwCkBAIC0xMjY0LDEwICsxMjY0LDEzIEBA IHZvaWQgdHN0X1FXZWJQYWdlOjp0ZXN0T3B0aW9uYWxKU09iamVjdHMoKQogICAgIHdlYlBhZ2Ux LmN1cnJlbnRGcmFtZSgpLT5zZXRIdG1sKFFTdHJpbmcoIjxodG1sPjxib2R5PnRlc3Q8L2JvZHk+ PC9odG1sPiIpLCBRVXJsKCkpOwogICAgIHdlYlBhZ2UyLmN1cnJlbnRGcmFtZSgpLT5zZXRIdG1s KFFTdHJpbmcoIjxodG1sPjxib2R5PnRlc3Q8L2JvZHk+PC9odG1sPiIpLCBRVXJsKCkpOwogCi0g ICAgUUNPTVBBUkUodGVzdEZsYWcod2ViUGFnZTEsIFFXZWJTZXR0aW5nczo6T2ZmbGluZVdlYkFw cGxpY2F0aW9uQ2FjaGVFbmFibGVkLCAiYXBwbGljYXRpb25DYWNoZSIsIGZhbHNlKSwgZmFsc2Up OworICAgIHdlYlBhZ2UxLnNldHRpbmdzKCktPnNldEF0dHJpYnV0ZShRV2ViU2V0dGluZ3M6Ok9m ZmxpbmVXZWJBcHBsaWNhdGlvbkNhY2hlRW5hYmxlZCwgZmFsc2UpOworICAgIHdlYlBhZ2UyLnNl dHRpbmdzKCktPnNldEF0dHJpYnV0ZShRV2ViU2V0dGluZ3M6Ok9mZmxpbmVXZWJBcHBsaWNhdGlv bkNhY2hlRW5hYmxlZCwgdHJ1ZSk7CisgICAgLy8gQ2hlY2sgaW50ZXJmZXJhbmNlLgorICAgIFFD T01QQVJFKHdlYlBhZ2UxLnNldHRpbmdzKCktPnRlc3RBdHRyaWJ1dGUoUVdlYlNldHRpbmdzOjpP ZmZsaW5lV2ViQXBwbGljYXRpb25DYWNoZUVuYWJsZWQpLCBmYWxzZSk7CisgICAgUUNPTVBBUkUo d2ViUGFnZTIuc2V0dGluZ3MoKS0+dGVzdEF0dHJpYnV0ZShRV2ViU2V0dGluZ3M6Ok9mZmxpbmVX ZWJBcHBsaWNhdGlvbkNhY2hlRW5hYmxlZCksIHRydWUpOworICAgIC8vIENoZWNrIGlmIEFwcGxp Y2F0aW9uQ2FjaGUgaXMgYWNjZXNzb2JsZSBmcm9tIEpTIEFQSSBhZnRlciBlbmFibGluZyBvZmZs aW5lIHdlYiBhcHBsaWNhdGlvbiBjYWNoZQogICAgIFFDT01QQVJFKHRlc3RGbGFnKHdlYlBhZ2Uy LCBRV2ViU2V0dGluZ3M6Ok9mZmxpbmVXZWJBcHBsaWNhdGlvbkNhY2hlRW5hYmxlZCwgImFwcGxp Y2F0aW9uQ2FjaGUiLCB0cnVlKSwgIHRydWUpOwotICAgIFFDT01QQVJFKHRlc3RGbGFnKHdlYlBh Z2UxLCBRV2ViU2V0dGluZ3M6Ok9mZmxpbmVXZWJBcHBsaWNhdGlvbkNhY2hlRW5hYmxlZCwgImFw cGxpY2F0aW9uQ2FjaGUiLCBmYWxzZSksIGZhbHNlKTsKLSAgICBRQ09NUEFSRSh0ZXN0RmxhZyh3 ZWJQYWdlMiwgUVdlYlNldHRpbmdzOjpPZmZsaW5lV2ViQXBwbGljYXRpb25DYWNoZUVuYWJsZWQs ICJhcHBsaWNhdGlvbkNhY2hlIiwgZmFsc2UpLCB0cnVlKTsKIAogICAgIFFDT01QQVJFKHRlc3RG bGFnKHdlYlBhZ2UxLCBRV2ViU2V0dGluZ3M6OkxvY2FsU3RvcmFnZUVuYWJsZWQsICJsb2NhbFN0 b3JhZ2UiLCBmYWxzZSksIGZhbHNlKTsKICAgICBRQ09NUEFSRSh0ZXN0RmxhZyh3ZWJQYWdlMiwg UVdlYlNldHRpbmdzOjpMb2NhbFN0b3JhZ2VFbmFibGVkLCAibG9jYWxTdG9yYWdlIiwgdHJ1ZSks ICB0cnVlKTsK 40864 2009-10-08 04:23:56 -0700 2009-10-08 06:02:55 -0700 xfail XFail.diff text/plain 2269 jedrzej.nowacki ZGlmZiAtLWdpdCBhL1dlYktpdC9xdC9DaGFuZ2VMb2cgYi9XZWJLaXQvcXQvQ2hhbmdlTG9nCmlu ZGV4IGVmZDc0ZTQuLjIzMTc3ZTYgMTAwNjQ0Ci0tLSBhL1dlYktpdC9xdC9DaGFuZ2VMb2cKKysr IGIvV2ViS2l0L3F0L0NoYW5nZUxvZwpAQCAtMiw2ICsyLDI0IEBACiAKICAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCiAKKyAgICAgICAgUGFydCBvZiB0ZXN0T3B0aW9uYWxKU09i amVjdHMgYXV0b3Rlc3Qgd2FzIG1hcmtlZCBhcyAiZXhwZWN0IHRvIGZhaWwiLgorICAgICAgICAK KyAgICAgICAgVHdvIHBsYWNlcyBpbiB0c3RfUVdlYlBhZ2U6OnRlc3RPcHRpb25hbEpTT2JqZWN0 cyB3ZXJlIG1hcmtlZCBhcworICAgICAgICBleHBlY3RlZCB0byBmYWlsLiBUaGUgcHJvYmxlbSBj b25jZXJuIGNoZWNraW5nIGlmIGEgZmVhdHVyZSBpcyBlbmFibGVkCisgICAgICAgIG9yIGRpc2Fi bGVkLiBBY2NvcmRpbmcgdG8gZGlzY3Vzc2lvbiBvbiB3ZWJraXQgZGV2IG1haWxpbmcgbGlzdAor ICAgICAgICBhIGRpc2FibGVkIGZlYXR1cmUgc2hvdWxkIGJlIGludmlzaWJsZSBmcm9tIGphdmEg c2NyaXB0IGxldmVsLCBidXQKKyAgICAgICAgdGhlcmUgYXJlIGV4Y2VwdGlvbnMgZnJvbSB0aGUg cnVsZS4gU28gd2UgZGVjaWRlZCB0byBkaXNhYmxlIHRoZSB0ZXN0CisgICAgICAgIGZvciBub3cu CisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTI5ODY3 CisKKyAgICAgICAgKiB0ZXN0cy9xd2VicGFnZS90c3RfcXdlYnBhZ2UuY3BwOgorICAgICAgICAo dHN0X1FXZWJQYWdlOjp0ZXN0T3B0aW9uYWxKU09iamVjdHMpOgorCisyMDA5LTEwLTA4ICBKZWRy emVqIE5vd2Fja2kgIDxqZWRyemVqLm5vd2Fja2lAbm9raWEuY29tPgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCiAgICAgICAgIFFXZWJQYWdlJ3MgY3JlYXRlVmlld2xl c3NQbHVnaW4gYXV0b3Rlc3QgY3Jhc2ggZml4LgogCiAgICAgICAgIEEgcGx1Zy1pbnMgcmV0dXJu aW5nIHdpZGdldHMgKFFXaWRnZXQgb3IgUUdyYXBoaWNzV2lkZ2V0KSBtaWdodCBiZQpkaWZmIC0t Z2l0IGEvV2ViS2l0L3F0L3Rlc3RzL3F3ZWJwYWdlL3RzdF9xd2VicGFnZS5jcHAgYi9XZWJLaXQv cXQvdGVzdHMvcXdlYnBhZ2UvdHN0X3F3ZWJwYWdlLmNwcAppbmRleCBkNmM3M2ViLi40YjQ4NGIz IDEwMDY0NAotLS0gYS9XZWJLaXQvcXQvdGVzdHMvcXdlYnBhZ2UvdHN0X3F3ZWJwYWdlLmNwcAor KysgYi9XZWJLaXQvcXQvdGVzdHMvcXdlYnBhZ2UvdHN0X3F3ZWJwYWdlLmNwcApAQCAtMTM4OSw4 ICsxMzg5LDEwIEBAIHZvaWQgdHN0X1FXZWJQYWdlOjp0ZXN0T3B0aW9uYWxKU09iamVjdHMoKQog ICAgIHdlYlBhZ2UxLmN1cnJlbnRGcmFtZSgpLT5zZXRIdG1sKFFTdHJpbmcoIjxodG1sPjxib2R5 PnRlc3Q8L2JvZHk+PC9odG1sPiIpLCBRVXJsKCkpOwogICAgIHdlYlBhZ2UyLmN1cnJlbnRGcmFt ZSgpLT5zZXRIdG1sKFFTdHJpbmcoIjxodG1sPjxib2R5PnRlc3Q8L2JvZHk+PC9odG1sPiIpLCBR VXJsKCkpOwogCisgICAgUUVYUEVDVF9GQUlMKCIiLCJGZWF0dXJlIGVuYWJsZWQvZGlzYWJsZWQg Y2hlY2tpbmcgcHJvYmxlbS4gTG9vayBhdCBidWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTI5ODY3IiwgQ29udGludWUpOwogICAgIFFDT01QQVJFKHRlc3RGbGFnKHdlYlBhZ2UxLCBRV2Vi U2V0dGluZ3M6Ok9mZmxpbmVXZWJBcHBsaWNhdGlvbkNhY2hlRW5hYmxlZCwgImFwcGxpY2F0aW9u Q2FjaGUiLCBmYWxzZSksIGZhbHNlKTsKICAgICBRQ09NUEFSRSh0ZXN0RmxhZyh3ZWJQYWdlMiwg UVdlYlNldHRpbmdzOjpPZmZsaW5lV2ViQXBwbGljYXRpb25DYWNoZUVuYWJsZWQsICJhcHBsaWNh dGlvbkNhY2hlIiwgdHJ1ZSksICB0cnVlKTsKKyAgICBRRVhQRUNUX0ZBSUwoIiIsIkZlYXR1cmUg ZW5hYmxlZC9kaXNhYmxlZCBjaGVja2luZyBwcm9ibGVtLiBMb29rIGF0IGJ1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9Mjk4NjciLCBDb250aW51ZSk7CiAgICAgUUNPTVBBUkUodGVzdEZs YWcod2ViUGFnZTEsIFFXZWJTZXR0aW5nczo6T2ZmbGluZVdlYkFwcGxpY2F0aW9uQ2FjaGVFbmFi bGVkLCAiYXBwbGljYXRpb25DYWNoZSIsIGZhbHNlKSwgZmFsc2UpOwogICAgIFFDT01QQVJFKHRl c3RGbGFnKHdlYlBhZ2UyLCBRV2ViU2V0dGluZ3M6Ok9mZmxpbmVXZWJBcHBsaWNhdGlvbkNhY2hl RW5hYmxlZCwgImFwcGxpY2F0aW9uQ2FjaGUiLCBmYWxzZSksIHRydWUpOwogCg==