63236 2011-06-23 01:55:08 -0700 [Qt] Fix tst_QWebFrame::renderGeometry() API test 2011-07-01 07:47:15 -0700 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) All All RESOLVED FIXED Qt, QtTriaged P2 Normal --- 38654 1 ossy rafael.lobo ademar kling luiz menard ossy rafael.lobo webkit.review.bot oldest_to_newest 426026 0 ossy 2011-06-23 01:55:08 -0700 http://build.webkit.org/builders/Qt%20Linux%20Release/builds/34519 FAIL! : tst_QWebFrame::renderGeometry() Compared values are not the same Actual (size.width()): 100 Expected (picture.boundingRect().width() + frame->scrollBarGeometry(Qt::Vertical).width()): 0 Loc: [/ramdisk/qt-linux-release/build/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2912)] 426107 1 ossy 2011-06-23 05:51:43 -0700 Three more fails: FAIL! : tst_QWebFrame::renderGeometry() Compared values are not the same Actual (size.height()): 100 Expected (picture.boundingRect().height() + frame->scrollBarGeometry(Qt::Horizontal).height()): 0 Loc: [/home/oszi/WebKit/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2915)] FAIL! : tst_QWebFrame::renderGeometry() Compared values are not the same Actual (size.width()): 100 Expected (picture.boundingRect().width()): 0 Loc: [/home/oszi/WebKit/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2924)] FAIL! : tst_QWebFrame::renderGeometry() Compared values are not the same Actual (size.height()): 100 Expected (picture.boundingRect().height()): 0 Loc: [/home/oszi/WebKit/Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp(2927)] Marked as expected fails: http://trac.webkit.org/changeset/89561 428924 2 rafael.lobo 2011-06-28 09:27:09 -0700 It looks like a regression, but I don't know what patch caused this. When you load with substitute data and there are local resources on it, it is no longer emitting loadFinished signal; it might not be trying to load them. In this test, the image inside the inner frame is not being loaded, then the content size remains the same as the frame's size. The expected behavior is to put the image inside, let it expand the content size to 120x120, and then you'll need scrollbars, as the frame size is only 100x100. Without this expansion, the scrollbars are not created, and there's also no painting inside the frame, as there's no content. So in the end, the sizes sum is zero. This is happening only for local resources, like file or qrc protocol, that are supposed to get loaded as soon as the substitute data lands. If you set the substitute data and inside it there's an image that uses a http protocol, for example, then it will try to request it normally. If instead of substitute data, you load the page as a local file (with file protocol), then everything works fine too. This issue is probably causing tst_QWebFrame::setHtmlWithResource to fail as well. It would be really nice to identify what caused this regression. 430047 3 rafael.lobo 2011-06-29 18:00:24 -0700 I think I've finally got what is going wrong. It's a security origin issue. When we load substitute data in that test without defining its base url, it won't have an origin that can load local files (by default only local files can load local files). For some yet unknown reason, we're not enforcing our setting defined in QWebPagePrivate's constructor that we should allow local loads for local files and for substitute data. That's what I'm currently investigating. A quick fix for this test is to put some local file as base url for the iframe, like "file://path/to/local". 430757 4 99384 rafael.lobo 2011-06-30 15:57:26 -0700 Created attachment 99384 We need a proper security origin in order to load local resources, so specified a local file to be its baseUrl. It took me a while, but this patch made this "bug" appears: https://bugs.webkit.org/show_bug.cgi?id=61494. I've explained there how it is happening, and it looks like it didn't affect the other ports. But sincerely I think we've been using setHtml in the wrong way. On QWebPagePrivate's constructor, you can see it's setting a SecurityOrigin policy to allow local files and substitute data to load local resources, but the user of the API is not aware of this (correct me if I'm wrong), and until that patch they were able to do exactly like the test was using. And I've already seen many other circumstances where I would think a certain behavior would happen and then had to specify a valid baseUrl to do it (local storage for example). So if we agree with this current behavior, then it looks like our API docs must reflect this in a better way. The other option to fix this is to correct what was introduced by that patch, but still it looks awkward (for me) that we're doing this little exception here to download local resources, and to everything else we must get a proper security origin. The other bug (https://bugs.webkit.org/show_bug.cgi?id=63235) could also be fixed like this, and I could also add there a case where the user specify the substitute data but do not provide a valid origin, so we can test both behaviors (as the other test is actually testing substitute data with local resources, differently from this one). Waiting for feedback. :) 430768 5 99384 benjamin 2011-06-30 16:04:49 -0700 Comment on attachment 99384 We need a proper security origin in order to load local resources, so specified a local file to be its baseUrl. Good catch. 430812 6 webkit.review.bot 2011-06-30 16:29:00 -0700 The commit-queue encountered the following flaky tests while processing attachment 99384: http/tests/local/formdata/upload-events.html bug 63357 (author: jianli@chromium.org) The commit-queue is continuing to process your patch. 430815 7 99384 webkit.review.bot 2011-06-30 16:30:30 -0700 Comment on attachment 99384 We need a proper security origin in order to load local resources, so specified a local file to be its baseUrl. Clearing flags on attachment: 99384 Committed r90181: <http://trac.webkit.org/changeset/90181> 430816 8 webkit.review.bot 2011-06-30 16:30:36 -0700 All reviewed patches have been landed. Closing bug. 431154 9 ademar 2011-07-01 07:47:15 -0700 Revision r90181 cherry-picked into qtwebkit-2.2 with commit 173b2e2 <http://gitorious.org/webkit/qtwebkit/commit/173b2e2> 99384 2011-06-30 15:57:26 -0700 2011-06-30 16:30:30 -0700 We need a proper security origin in order to load local resources, so specified a local file to be its baseUrl. renderGeometry-origin.diff text/plain 2718 rafael.lobo ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvcXQvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9x dC9DaGFuZ2VMb2cKaW5kZXggZGY2YWNkNi4uYTViNzAyOSAxMDA2NDQKLS0tIGEvU291cmNlL1dl YktpdC9xdC9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxMS0wNi0zMCAgUmFmYWVsIEJyYW5kYW8gIDxyYWZhZWwubG9ib0BvcGVu Ym9zc2Eub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFtRdF0gRml4IHRzdF9RV2ViRnJhbWU6OnJlbmRlckdlb21ldHJ5KCkgQVBJIHRlc3QKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYzMjM2CisgICAg ICAgIAorICAgICAgICBUaGlzIHRlc3QgcmVxdWlyZWQgYSBzZWN1cml0eSBvcmlnaW4gd2l0aCBn cmFudGVkIHBlcm1pc3Npb24gdG8gcmVxdWVzdCBsb2NhbCByZXNvdXJjZXMuCisgICAgICAgIEJ5 IGRlZmF1bHQsIG9ubHkgbG9jYWwgZmlsZXMgY2FuIGxvYWQgbG9jYWwgcmVzb3VyY2VzLiBTbyBt b2RpZmllZCBiYXNlVXJsIHRvIGJlIGEgbG9jYWwgZmlsZS4KKworICAgICAgICAqIHRlc3RzL3F3 ZWJmcmFtZS90c3RfcXdlYmZyYW1lLmNwcDoKKyAgICAgICAgKHRzdF9RV2ViRnJhbWU6OnJlbmRl ckdlb21ldHJ5KToKKwogMjAxMS0wNi0zMCAgRmFicml6aW8gTWFjaGFkbyAgPGZhYnJpemlvLm1h Y2hhZG9Abm9raWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEJlbmphbWluIFBvdWxhaW4u CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L3F0L3Rlc3RzL3F3ZWJmcmFtZS90c3RfcXdlYmZy YW1lLmNwcCBiL1NvdXJjZS9XZWJLaXQvcXQvdGVzdHMvcXdlYmZyYW1lL3RzdF9xd2ViZnJhbWUu Y3BwCmluZGV4IGYyYmRjZWUuLjIyMGUxNGMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvcXQv dGVzdHMvcXdlYmZyYW1lL3RzdF9xd2ViZnJhbWUuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvcXQv dGVzdHMvcXdlYmZyYW1lL3RzdF9xd2ViZnJhbWUuY3BwCkBAIC0yODk4LDcgKzI4OTgsMTAgQEAg dm9pZCB0c3RfUVdlYkZyYW1lOjpyZW5kZXJHZW9tZXRyeSgpCiAgICAgUUxpc3Q8UVdlYkZyYW1l Kj4gZnJhbWVzID0gcGFnZS5tYWluRnJhbWUoKS0+Y2hpbGRGcmFtZXMoKTsKICAgICBRV2ViRnJh bWUgKmZyYW1lID0gZnJhbWVzLmF0KDApOwogICAgIFFTdHJpbmcgaW5uZXJIdG1sKCI8Ym9keSBz dHlsZT0nbWFyZ2luOiAwcHg7Jz48aW1nIHNyYz0ncXJjOi9pbWFnZS5wbmcnLz48L2JvZHk+Iik7 Ci0gICAgZnJhbWUtPnNldEh0bWwoaW5uZXJIdG1sKTsKKworICAgIC8vIEJ5IGRlZmF1bHQsIG9u bHkgc2VjdXJpdHkgb3JpZ2lucyBvZiBsb2NhbCBmaWxlcyBjYW4gbG9hZCBsb2NhbCByZXNvdXJj ZXMuCisgICAgLy8gU28gd2Ugc2hvdWxkIHNwZWNpZnkgYmFzZVVybCB0byBiZSBhIGxvY2FsIGZp bGUgaW4gb3JkZXIgdG8gZ2V0IGEgcHJvcGVyIG9yaWdpbi4KKyAgICBmcmFtZS0+c2V0SHRtbChp bm5lckh0bWwsIFFVcmwoImZpbGU6Ly8vcGF0aC90by9maWxlIikpOwogICAgIHdhaXRGb3JTaWdu YWwoZnJhbWUsIFNJR05BTChsb2FkRmluaXNoZWQoYm9vbCkpLCAyMDApOwogCiAgICAgUVBpY3R1 cmUgcGljdHVyZTsKQEAgLTI5MTEsOSArMjkxNCw3IEBAIHZvaWQgdHN0X1FXZWJGcmFtZTo6cmVu ZGVyR2VvbWV0cnkoKQogICAgIGZyYW1lLT5yZW5kZXIoJnBhaW50ZXIxLCBRV2ViRnJhbWU6OkNv bnRlbnRzTGF5ZXIpOwogICAgIHBhaW50ZXIxLmVuZCgpOwogCi0gICAgUUVYUEVDVF9GQUlMKCIi LCAiaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYzMjM2IiwgQ29udGlu dWUpOwogICAgIFFDT01QQVJFKHNpemUud2lkdGgoKSwgcGljdHVyZS5ib3VuZGluZ1JlY3QoKS53 aWR0aCgpICsgZnJhbWUtPnNjcm9sbEJhckdlb21ldHJ5KFF0OjpWZXJ0aWNhbCkud2lkdGgoKSk7 Ci0gICAgUUVYUEVDVF9GQUlMKCIiLCAiaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTYzMjM2IiwgQ29udGludWUpOwogICAgIFFDT01QQVJFKHNpemUuaGVpZ2h0KCksIHBp Y3R1cmUuYm91bmRpbmdSZWN0KCkuaGVpZ2h0KCkgKyBmcmFtZS0+c2Nyb2xsQmFyR2VvbWV0cnko UXQ6Okhvcml6b250YWwpLmhlaWdodCgpKTsKIAogICAgIC8vIHJlbmRlciBldmVyeXRoaW5nLCBz aG91bGQgYmUgdGhlIHNpemUgb2YgdGhlIGlmcmFtZQpAQCAtMjkyMSw5ICsyOTIyLDcgQEAgdm9p ZCB0c3RfUVdlYkZyYW1lOjpyZW5kZXJHZW9tZXRyeSgpCiAgICAgZnJhbWUtPnJlbmRlcigmcGFp bnRlcjIsIFFXZWJGcmFtZTo6QWxsTGF5ZXJzKTsKICAgICBwYWludGVyMi5lbmQoKTsKIAotICAg IFFFWFBFQ1RfRkFJTCgiIiwgImh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD02MzIzNiIsIENvbnRpbnVlKTsKICAgICBRQ09NUEFSRShzaXplLndpZHRoKCksIHBpY3R1cmUu Ym91bmRpbmdSZWN0KCkud2lkdGgoKSk7ICAgLy8gd2lkdGg6IDEwMHB4Ci0gICAgUUVYUEVDVF9G QUlMKCIiLCAiaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYzMjM2Iiwg Q29udGludWUpOwogICAgIFFDT01QQVJFKHNpemUuaGVpZ2h0KCksIHBpY3R1cmUuYm91bmRpbmdS ZWN0KCkuaGVpZ2h0KCkpOyAvLyBoZWlnaHQ6IDEwMHB4CiB9CiAK