Bug 70110 – [Qt] fast/events/media-focus-in-standalone-media-document.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

fast/events/media-focus-in-standalone-media-document.html 
introduced in http://trac.webkit.org/changeset/97367 and fails

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/events/media-focus-in-standalone-media-document-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/events/media-focus-in-standalone-media-document-actual.txt 
@@ -1,13 +1,4 @@
+FAIL: Timed out waiting for notifyDone to be called
 This tests that media element in a standalone media document cannot be focused directly using focus() method or by mouse click.
 
 
-
-*** Should not focus video element by calling focus() method.
-EXPECTED (standaloneMediaDocument.activeElement != '[object HTMLVideoElement]') OK
-
-*** Should not focus video element by mouse click.
-*** Video element clicked.
-EXPECTED (standaloneMediaDocument.activeElement != '[object HTMLVideoElement]') OK
-
-END OF TEST
-

[Deepak Sherveghar]

After setting up QT WebKit port on Linux and debugging it, I found two issues as to why this test case is failing.

1. Standalone media element is not supported in QT WebKit port on Linux.
If we give a Standalone media url (eg: http://abc/test.mp4 or .mov) to QTTestbrowser, the FrameLoaderClientQt.cpp sets the response policy as "download" instead of "use". Since canShowMIMEType() looks up the mimetype for only SupportedImage, SupportedNonImage and MIMETypeRegistered with plugins. It should also check for SupportedMediaMIMEType().
ie: a check in FrameLoaderClientQt::canShowMIMEType()

if (MIMETypeRegistry::isSupportedMediaMIMEType(type))
        return true;

Only then the policy would be set to use and Standalone media elements will be rendered using the MediaDocument architecture.

I am not not sure as to why standalone media element is not supported as of yet in QT. Is it because of some design decision that may have happened earlier or a feature QT port doesn't wish to support??

2. QT MIMETypeRegistry doesn't list media mime types (audio as well video) in its extension map.
When loading the standalone media element (in this case iframe's src set to an mp4 file), Qt's NetworkReplyHandler receives the content type header (QNetworkRequest::ContentTypeHeader) as null. Since this is null, it tries to set the mimetype using the url extension (MIMETypeRegistry::getMIMETypeForPath()). In this case it would try to look up the registry using extension ".mp4".
But since this extension is not listed, the load fails. In the failing test case notifydone() is called in the onload handler of this iframe load. Since the load fails, onload handler is not called, and the test case times out.

I think we should publish the media mimetypes in the extensionmap. Something like 
ie: in MIMETypeRegistryQT.cpp

static const ExtensionMap extensionMap[] = {
... 
{ "mp4", "video/mp4" },
{ "m4v", "video/mp4" },
{ "m4a", "audio/x-m4a"},
{ "mp3", "audio/mp3"},
{ "ogv", "video/ogg"},
{ "oga", "audio/ogg"},
{ "webm", "video/webm},
{ "webm", "audio/webm},
{ "wav", "audio/wav"},
..
};

In GTK, soup library returns appropriate content type. (soup_request_get_content_type()). Hence the test case works fine in GTK.

With the above two fixes, test case runs fine. Both the above two points are necessary to fix this failing test case in QT.

Let me know your thoughts on this.
If it seems reasonable enough will upload the patch.

[Jesus Sanchez-Palencia]

(In reply to comment #1)
Thanks for taking a look at this!

> if (MIMETypeRegistry::isSupportedMediaMIMEType(type))
>         return true;

Are the ports in WebKit1 checking for this as well? I took a quick look at WebKit2 and unless I'm missing something, the common implementation of WebPageProxy::canShowMIMEType doesn't check for this.

> I am not not sure as to why standalone media element is not supported as of yet in QT. Is it because of some design decision that may have happened earlier or a feature QT port doesn't wish to support??

I'm not sure whether it is supported or not, sorry. 


> 
> I think we should publish the media mimetypes in the extensionmap. Something like 
> ie: in MIMETypeRegistryQT.cpp
> 
> static const ExtensionMap extensionMap[] = {
> ... 
> { "mp4", "video/mp4" },
> { "m4v", "video/mp4" },
> { "m4a", "audio/x-m4a"},
> { "mp3", "audio/mp3"},
> { "ogv", "video/ogg"},
> { "oga", "audio/ogg"},
> { "webm", "video/webm},
> { "webm", "audio/webm},
> { "wav", "audio/wav"},
> ..
> };

It seems something that would be good to have. But maybe Alexis will have a better opinion on this.


> With the above two fixes, test case runs fine. Both the above two points are necessary to fix this failing test case in QT.
> 
> Let me know your thoughts on this.
> If it seems reasonable enough will upload the patch.

Well, if you say the test works with the above fixes then I guess we can say that standalone media element is supported by Qt, right? Maybe it would be a good idea to take a look at the patch and once it's reviewed and landed I (or even you, if you are up to) can investigate if it fits on our WebKit2 port as well.

Thanks again!

[Deepak Sherveghar]

(In reply to comment #2)

> Are the ports in WebKit1 checking for this as well? I took a quick look at WebKit2 and unless I'm missing something, the common implementation of WebPageProxy::canShowMIMEType doesn't check for this.

Atleast the GTK port does for WebKit1.
Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp
bool FrameLoaderClient::canShowMIMEType(const String& type) const
{
    return (MIMETypeRegistry::isSupportedImageMIMEType(type)
            || MIMETypeRegistry::isSupportedNonImageMIMEType(type)
            || MIMETypeRegistry::isSupportedMediaMIMEType(type)
            || PluginDatabase::installedPlugins()->isMIMETypeRegistered(type));
}
 
 
> > With the above two fixes, test case runs fine. Both the above two points are necessary to fix this failing test case in QT.
> > 
> > Let me know your thoughts on this.
> > If it seems reasonable enough will upload the patch.
> 
> Well, if you say the test works with the above fixes then I guess we can say that standalone media element is supported by Qt, right? Maybe it would be a good idea to take a look at the patch and once it's reviewed and landed I (or even you, if you are up to) can investigate if it fits on our WebKit2 port as well.

Yes, with the above fixes QT does render standalone media elements as well as test case runs as expected.

[Deepak Sherveghar]

Created attachment 111954 [details]
patch

[Antonio Gomes]

Comment on attachment 111954 [details]
patch

Looks good to me. Alexis?

[Simon Hausmann]

Comment on attachment 111954 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111954&action=review

> Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp:64
> +    { "mp4", "video/mp4" },
> +    { "m4v", "video/mp4" },
> +    { "m4a", "audio/x-m4a" },
> +    { "mp3", "audio/mp3" },
> +    { "ogv", "video/ogg" },
> +    { "oga", "audio/ogg" },
> +    { "ogm", "audio/ogg" },
> +    { "ogg", "audio/ogg" },
> +    { "webm", "video/webm" },
> +    { "webm", "audio/webm" },
> +    { "wav", "audio/wav" },
> +    { "mov", "video/quicktime" },

I think that this part is correct and badly needed :)

> Source/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:666
> +    if (MIMETypeRegistry::isSupportedMediaMIMEType(type))
> +        return true;
> +

canShowMIMEType is called _after_ dispatchDecidePolicyForResponse. If DecidePolicyForResponse returns PolicyUse but canShowMIMEType suddenly would return false, then that
would sound like a bug to me. I think that is why for example the WK2 implementation always returns true and we probably should do the same in WK1. Your patch "improves" this
by increasing the probability of returning true ;-), but in the long run I wonder if canShowMIMEType should be removed from the FrameLoaderClient altogether?

[Simon Hausmann]

Just to clarify: I gave the r+ because it improves the extension map (which improves local file support) and it fixes a skipped test. The canShowMIMEType _can_ be done differently, but it's certainly an improvement.

[WebKit Review Bot]

Comment on attachment 111954 [details]
patch

Clearing flags on attachment: 111954

Committed r98231: <http://trac.webkit.org/changeset/98231>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Deepak Sherveghar]

Thanks Jesus Sanchez-Palencia, Antonio, Simon for review and guidance !!