Bug 62108 – [Qt] Failing test media/video-document-types.html

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Robert Hogan]

Do the Qt equivalent of  https://bugs.webkit.org/show_bug.cgi?id=31352 and http://trac.webkit.org/changeset/51104

[Deepak Sherveghar]

Created attachment 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.

[Jesus Sanchez-Palencia]

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

[Simon Hausmann]

Comment on attachment 113300 [details]
patch

r=me

[WebKit Review Bot]

Comment on attachment 113300 [details]
patch

Clearing flags on attachment: 113300

Committed r99071: <http://trac.webkit.org/changeset/99071>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Deepak Sherveghar]

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

[Csaba Osztrogonác]

Thanks for debugging. I agree with you, please file a separate bug.

[Csaba Osztrogonác]

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.

[Deepak Sherveghar]

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

[Deepak Sherveghar]

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

[Deepak Sherveghar]

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.

[Csaba Osztrogonác]

Thanks for the fix.