Bug 62108 - [Qt] Failing test media/video-document-types.html
Summary: [Qt] Failing test media/video-document-types.html
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on: 62089
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-05 06:29 PDT by Robert Hogan
Modified: 2011-11-09 09:37 PST (History)
6 users (show)

See Also:


Attachments
patch (2.23 KB, patch)
2011-11-02 03:55 PDT, Deepak Sherveghar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2011-06-05 06:29:56 PDT
Do the Qt equivalent of  https://bugs.webkit.org/show_bug.cgi?id=31352 and http://trac.webkit.org/changeset/51104
Comment 1 Deepak Sherveghar 2011-11-02 03:55:11 PDT
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.
Comment 2 Jesus Sanchez-Palencia 2011-11-02 07:28:01 PDT
(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!
Comment 3 Simon Hausmann 2011-11-02 09:30:33 PDT
Comment on attachment 113300 [details]
patch

r=me
Comment 4 WebKit Review Bot 2011-11-02 09:44:53 PDT
Comment on attachment 113300 [details]
patch

Clearing flags on attachment: 113300

Committed r99071: <http://trac.webkit.org/changeset/99071>
Comment 5 WebKit Review Bot 2011-11-02 09:44:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2011-11-02 12:37:51 PDT
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?
Comment 7 Deepak Sherveghar 2011-11-03 04:37:26 PDT
(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.
Comment 8 Csaba Osztrogonác 2011-11-03 04:50:32 PDT
Thanks for debugging. I agree with you, please file a separate bug.
Comment 9 Csaba Osztrogonác 2011-11-03 05:06:18 PDT
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.
Comment 10 Deepak Sherveghar 2011-11-04 05:39:06 PDT
(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.
Comment 11 Deepak Sherveghar 2011-11-04 05:40:47 PDT
(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.
Comment 12 Deepak Sherveghar 2011-11-09 09:21:38 PST
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.
Comment 13 Csaba Osztrogonác 2011-11-09 09:37:13 PST
Thanks for the fix.