62108 2011-06-05 06:29:56 -0700 [Qt] Failing test media/video-document-types.html 2011-11-09 09:37:13 -0800 1 1 1 Unclassified WebKit WebKit Qt 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Qt, QtTriaged P2 Normal --- 62089 1 robert webkit-unassigned bpwv64 hausmann jesus menard ossy webkit.review.bot oldest_to_newest 415594 0 robert 2011-06-05 06:29:56 -0700 Do the Qt equivalent of https://bugs.webkit.org/show_bug.cgi?id=31352 and http://trac.webkit.org/changeset/51104 494541 1 113300 bpwv64 2011-11-02 03:55:11 -0700 Created attachment 113300 patch media/video-document-types.html (when removed from the QT skipped list) runs fine now with the latest code base. But logically, we still need to cancel the main resource load, if the data is handled by MediaDocument. Webkit2 as well as all other ports (GTK/Chromium etc) do that. In my patch I didn't un-skip the media/video-document-types.html because the media folder itself is in skipped list. 494616 2 jesus 2011-11-02 07:28:01 -0700 (In reply to comment #1) > Created an attachment (id=113300) [details] > patch > > media/video-document-types.html (when removed from the QT skipped list) runs fine now with the latest code base. > > But logically, we still need to cancel the main resource load, if the data is handled by MediaDocument. > Webkit2 as well as all other ports (GTK/Chromium etc) do that. > > In my patch I didn't un-skip the media/video-document-types.html because the media folder itself is in > skipped list. LGTM. I was going to fix our WebKit1 shouldFallBack implementation, so thanks! :) I'm not 100% sure about the commitLoad part but I did a quick check and other ports are handling it like that, as you said. Isn't there any other tests we can un-skip after this? By the way, can't we un-skip the media folder and this test and then just skip everything else in there? Thanks again! 494665 3 113300 hausmann 2011-11-02 09:30:33 -0700 Comment on attachment 113300 patch r=me 494673 4 113300 webkit.review.bot 2011-11-02 09:44:53 -0700 Comment on attachment 113300 patch Clearing flags on attachment: 113300 Committed r99071: <http://trac.webkit.org/changeset/99071> 494674 5 webkit.review.bot 2011-11-02 09:44:57 -0700 All reviewed patches have been landed. Closing bug. 494816 6 ossy 2011-11-02 12:37:51 -0700 Reopen, because it broke 4 layout tests on the Qt bot: http://build.webkit.org/results/Qt%20Linux%20Release/r99073%20%2839226%29/results.html Could you check it please? 495346 7 bpwv64 2011-11-03 04:37:26 -0700 (In reply to comment #6) > Reopen, because it broke 4 layout tests on the Qt bot: > http://build.webkit.org/results/Qt%20Linux%20Release/r99073%20%2839226%29/results.html > > Could you check it please? I spent some time debugging this issue and found the root cause. The problem is not with my patch, but the enum that we have borrowed from mac (code from FrameLoaderClientQt.cpp:930) // This was copied from file "WebKit/Source/WebKit/mac/Misc/WebKitErrors.h". enum { WebKitErrorCannotShowMIMEType = 100, WebKitErrorCannotShowURL = 101, WebKitErrorFrameLoadInterruptedByPolicyChange = 102, WebKitErrorCannotUseRestrictedPort = 103, WebKitErrorCannotFindPlugIn = 200, WebKitErrorCannotLoadPlugIn = 201, WebKitErrorJavaUnavailable = 202, WebKitErrorPluginWillHandleLoad = 203 }; shouldFallBack() tells the DOM if it should attempt to render the next nested <object> if its parent fails to load. Now when a url fails to load, QNetworkReplyHandler sets the QNetworkReply::NetworkError ( reply->error() any one of all possible error conditions found during the processing of the request) and converts that to ResourceError object setting appropriate fields like domain, error code, description etc. The QNetworkReply::NetworkError constant values clash with values selected for our enum (listed above). ie: QNetworkReply::NetworkError constant values : Constant Value ===================================== ProxyConnectionRefusedError 101 ProxyConnectionClosedError 102 ProxyNotFoundError 103 ProxyTimeoutError 104 ProxyAuthenticationRequiredError105 ContentAccessDenied 201 ContentOperationNotPermittedError202 ContentNotFoundError 203 AuthenticationRequiredError 204 ContentReSendError 205 ProtocolUnknownError 301 ProtocolInvalidOperationError 302 UnknownNetworkError 99 UnknownProxyError 199 UnknownContentError 299 ProtocolFailure 399 In the failing test case we get error.errorCode() as 203, which with respect to network error is 'ContentNotFoundError' and with respect to our enum is 'WebKitErrorPluginWillHandleLoad'. Changing the enum values for WebKitError's to some other constant values like WebKitErrorPluginWillHandleLoad = 2003, Then the failing test cases work fine. I think to track changes better we should file a separate bug for resolving this enum issue and close the current reopened issue. Let me know if you have any comments ???? also let me know if I can go ahead and file a separate bug and a patch for that. 495352 8 ossy 2011-11-03 04:50:32 -0700 Thanks for debugging. I agree with you, please file a separate bug. 495361 9 ossy 2011-11-03 05:06:18 -0700 I added the failing tests to the skipped list to make buildbot happier: http://trac.webkit.org/changeset/99168 . Please unskip them with the fix. 496173 10 bpwv64 2011-11-04 05:39:06 -0700 (In reply to comment #9) > I added the failing tests to the skipped list to make buildbot happier: > http://trac.webkit.org/changeset/99168 . Please unskip them with the fix. Thanks Csaba Osztrogonac. Done. https://bugs.webkit.org/show_bug.cgi?id=71554 (In reply to comment #2) > By the way, can't we un-skip the media folder and this test and then just skip > everything else in there? I actually tried that with my local QT build. Many of the skipped test cases for media do work fine. Atleast on my local qt linux build. I wasn't sure of the build-bot environment/configuration, hence didn't suggest this :). With support of gstreamer coming in and extension map now supporting local media files, I think your suggestion makes absolute sense. 496174 11 bpwv64 2011-11-04 05:40:47 -0700 (In reply to comment #9) > I added the failing tests to the skipped list to make buildbot happier: > http://trac.webkit.org/changeset/99168 . Please unskip them with the fix. BTW . I feel this issue can now be closed. 499048 12 bpwv64 2011-11-09 09:21:38 -0800 Hi Csaba , bug 71554 is now resolved fixed. Request you to close this bug as well. Let me know if you need something from my side. 499073 13 ossy 2011-11-09 09:37:13 -0800 Thanks for the fix. 113300 2011-11-02 03:55:11 -0700 2011-11-02 09:44:53 -0700 patch patch.diff text/plain 2286 bpwv64 SW5kZXg6IFNvdXJjZS9XZWJLaXQvcXQvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJLaXQvcXQvQ2hhbmdlTG9nCShyZXZpc2lvbiA5OTA1MSkKKysrIFNvdXJjZS9XZWJLaXQvcXQv Q2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMTEtMDIgIERl ZXBhayBTaGVydmVnaGFyICA8YnB3djY0QG1vdG9yb2xhLmNvbT4KKworICAgICAgICBbUXRdIEZh aWxpbmcgdGVzdCBtZWRpYS92aWRlby1kb2N1bWVudC10eXBlcy5odG1sCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD02MjEwOAorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1hdGNoIHdoYXQgTWFjIGhhcyBiZWVu IGRvaW5nIHNpbmNlIHIzNjAwMTogY2FuY2VsIHRoZSBtYWluIGxvYWQsCisgICAgICAgIGFuZCBo YW5kbGUgZmFpbHVyZXMgY2F1c2VkIGJ5ICd3aWxsIGJlIGhhbmRsZWQgYnkgcGx1Z2luJyBlcnJv cnMuCisKKyAgICAgICAgKiBXZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudFF0LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkZyYW1lTG9hZGVyQ2xpZW50UXQ6OmNvbW1pdHRlZExvYWQpOgor ICAgICAgICAoV2ViQ29yZTo6RnJhbWVMb2FkZXJDbGllbnRRdDo6c2hvdWxkRmFsbEJhY2spOgor CiAyMDExLTExLTAxICBTaW1vbiBIYXVzbWFubiAgPHNpbW9uLmhhdXNtYW5uQG5va2lhLmNvbT4K IAogICAgICAgICBbUXRdW1dLMl0gRml4IGJ1aWxkIG9mIFdlYktpdFRlc3RSdW5uZXIgd2l0aCBF TEYgc3ltYm9sIHZpc2liaWxpdHkKSW5kZXg6IFNvdXJjZS9XZWJLaXQvcXQvV2ViQ29yZVN1cHBv cnQvRnJhbWVMb2FkZXJDbGllbnRRdC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYktpdC9x dC9XZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRlckNsaWVudFF0LmNwcAkocmV2aXNpb24gOTkwNDgp CisrKyBTb3VyY2UvV2ViS2l0L3F0L1dlYkNvcmVTdXBwb3J0L0ZyYW1lTG9hZGVyQ2xpZW50UXQu Y3BwCSh3b3JraW5nIGNvcHkpCkBAIC04OTksNyArODk5LDExIEBAIHZvaWQgRnJhbWVMb2FkZXJD bGllbnRRdDo6Y29tbWl0dGVkTG9hZCgKIHsKICAgICBpZiAoIW1fcGx1Z2luVmlldykKICAgICAg ICAgbG9hZGVyLT5jb21taXREYXRhKGRhdGEsIGxlbmd0aCk7Ci0gICAgCisKKyAgICAvLyBJZiB3 ZSBhcmUgc2VuZGluZyBkYXRhIHRvIE1lZGlhRG9jdW1lbnQsIHdlIHNob3VsZCBzdG9wIGhlcmUg YW5kIGNhbmNlbCB0aGUgcmVxdWVzdC4KKyAgICBpZiAobV9mcmFtZS0+ZG9jdW1lbnQoKS0+aXNN ZWRpYURvY3VtZW50KCkpCisgICAgICAgIGxvYWRlci0+Y2FuY2VsTWFpblJlc291cmNlTG9hZChw bHVnaW5XaWxsSGFuZGxlTG9hZEVycm9yKGxvYWRlci0+cmVzcG9uc2UoKSkpOworCiAgICAgLy8g V2UgcmUtY2hlY2sgaGVyZSBhcyB0aGUgcGx1Z2luIGNhbiBoYXZlIGJlZW4gY3JlYXRlZC4KICAg ICBpZiAobV9wbHVnaW5WaWV3ICYmIG1fcGx1Z2luVmlldy0+aXNQbHVnaW5WaWV3KCkpIHsKICAg ICAgICAgaWYgKCFtX2hhc1NlbnRSZXNwb25zZVRvUGx1Z2luKSB7CkBAIC05NzQsNyArOTc4LDcg QEAgV2ViQ29yZTo6UmVzb3VyY2VFcnJvciBGcmFtZUxvYWRlckNsaWVudAogCiBib29sIEZyYW1l TG9hZGVyQ2xpZW50UXQ6OnNob3VsZEZhbGxCYWNrKGNvbnN0IFdlYkNvcmU6OlJlc291cmNlRXJy b3ImIGVycm9yKQogewotICAgIHJldHVybiAhKGVycm9yLmlzQ2FuY2VsbGF0aW9uKCkgfHwgKGVy cm9yLmVycm9yQ29kZSgpID09IFdlYktpdEVycm9yRnJhbWVMb2FkSW50ZXJydXB0ZWRCeVBvbGlj eUNoYW5nZSkpOworICAgIHJldHVybiAhKGVycm9yLmlzQ2FuY2VsbGF0aW9uKCkgfHwgKGVycm9y LmVycm9yQ29kZSgpID09IFdlYktpdEVycm9yRnJhbWVMb2FkSW50ZXJydXB0ZWRCeVBvbGljeUNo YW5nZSkgfHwgKGVycm9yLmVycm9yQ29kZSgpID09IFdlYktpdEVycm9yUGx1Z2luV2lsbEhhbmRs ZUxvYWQpKTsKIH0KIAogV1RGOjpQYXNzUmVmUHRyPFdlYkNvcmU6OkRvY3VtZW50TG9hZGVyPiBG cmFtZUxvYWRlckNsaWVudFF0OjpjcmVhdGVEb2N1bWVudExvYWRlcihjb25zdCBXZWJDb3JlOjpS ZXNvdXJjZVJlcXVlc3QmIHJlcXVlc3QsIGNvbnN0IFN1YnN0aXR1dGVEYXRhJiBzdWJzdGl0dXRl RGF0YSkK