70110 2011-10-14 08:07:51 -0700 [Qt] fast/events/media-focus-in-standalone-media-document.html fails 2011-10-25 11:03:19 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED Qt, QtTriaged P2 Normal --- 1 ossy webkit-unassigned bpwv64 hausmann jesus leandro menard rakuco tonikitoo webkit.review.bot oldest_to_newest 483989 0 ossy 2011-10-14 08:07:51 -0700 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 - 486718 1 bpwv64 2011-10-19 06:14:50 -0700 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. 486841 2 jesus 2011-10-19 09:27:32 -0700 (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! 486874 3 bpwv64 2011-10-19 10:35:30 -0700 (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. 488336 4 111954 bpwv64 2011-10-21 05:30:45 -0700 Created attachment 111954 patch 488354 5 111954 tonikitoo 2011-10-21 06:52:09 -0700 Comment on attachment 111954 patch Looks good to me. Alexis? 489237 6 111954 hausmann 2011-10-24 02:23:28 -0700 Comment on attachment 111954 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? 489262 7 hausmann 2011-10-24 03:03:47 -0700 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. 489266 8 111954 webkit.review.bot 2011-10-24 03:28:33 -0700 Comment on attachment 111954 patch Clearing flags on attachment: 111954 Committed r98231: <http://trac.webkit.org/changeset/98231> 489267 9 webkit.review.bot 2011-10-24 03:28:50 -0700 All reviewed patches have been landed. Closing bug. 490149 10 bpwv64 2011-10-25 11:03:19 -0700 Thanks Jesus Sanchez-Palencia, Antonio, Simon for review and guidance !! 111954 2011-10-21 05:30:45 -0700 2011-10-24 03:28:33 -0700 patch patch.diff text/plain 4476 bpwv64 SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDk4MDk0KQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMTAtMjEgIERlZXBhayBT aGVydmVnaGFyICA8YnB3djY0QG1vdG9yb2xhLmNvbT4KKworICAgICAgICBbUXRdIGZhc3QvZXZl bnRzL21lZGlhLWZvY3VzLWluLXN0YW5kYWxvbmUtbWVkaWEtZG9jdW1lbnQuaHRtbCBmYWlscwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzAxMTAKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBFeHRlbnNpb25N YXAgc2hvdWxkIGFsc28gaW5jbHVkZSBtZWRpYSBmaWxlcyBtYXBwaW5nLgorCisgICAgICAgICog cGxhdGZvcm0vcXQvTUlNRVR5cGVSZWdpc3RyeVF0LmNwcDogQWRkZWQgbWVkaWEgZmlsZXMgbWFw cGluZy4KKwogMjAxMS0xMC0yMCAgQW5kcmV5IEtvc3lha292ICA8Y2FzZXFAY2hyb21pdW0ub3Jn PgogCiAgICAgICAgIFdlYiBJbnNwZWN0b3I6IG1ha2UgZXh0ZW5zaW9uIHRlc3RzIHBhc3Mgb24g Y2hyb21pdW0KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3F0L01JTUVUeXBlUmVnaXN0 cnlRdC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvTUlNRVR5 cGVSZWdpc3RyeVF0LmNwcAkocmV2aXNpb24gOTgwOTQpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9xdC9NSU1FVHlwZVJlZ2lzdHJ5UXQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01MCw2ICs1 MCwxOCBAQCBzdGF0aWMgY29uc3QgRXh0ZW5zaW9uTWFwIGV4dGVuc2lvbk1hcFtdCiAgICAgeyAi anBnIiwgImltYWdlL2pwZWciIH0sCiAgICAgeyAianMiLCAiYXBwbGljYXRpb24veC1qYXZhc2Ny aXB0IiB9LAogICAgIHsgIm1uZyIsICJ2aWRlby94LW1uZyIgfSwKKyAgICB7ICJtcDQiLCAidmlk ZW8vbXA0IiB9LAorICAgIHsgIm00diIsICJ2aWRlby9tcDQiIH0sCisgICAgeyAibTRhIiwgImF1 ZGlvL3gtbTRhIiB9LAorICAgIHsgIm1wMyIsICJhdWRpby9tcDMiIH0sCisgICAgeyAib2d2Iiwg InZpZGVvL29nZyIgfSwKKyAgICB7ICJvZ2EiLCAiYXVkaW8vb2dnIiB9LAorICAgIHsgIm9nbSIs ICJhdWRpby9vZ2ciIH0sCisgICAgeyAib2dnIiwgImF1ZGlvL29nZyIgfSwKKyAgICB7ICJ3ZWJt IiwgInZpZGVvL3dlYm0iIH0sCisgICAgeyAid2VibSIsICJhdWRpby93ZWJtIiB9LAorICAgIHsg IndhdiIsICJhdWRpby93YXYiIH0sCisgICAgeyAibW92IiwgInZpZGVvL3F1aWNrdGltZSIgfSwK ICAgICB7ICJwYm0iLCAiaW1hZ2UveC1wb3J0YWJsZS1iaXRtYXAiIH0sCiAgICAgeyAicGdtIiwg ImltYWdlL3gtcG9ydGFibGUtZ3JheW1hcCIgfSwKICAgICB7ICJwZGYiLCAiYXBwbGljYXRpb24v cGRmIiB9LApJbmRleDogU291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL1dlYktpdC9xdC9DaGFuZ2VMb2cJKHJldmlzaW9uIDk4MDk0KQorKysgU291cmNlL1dl YktpdC9xdC9DaGFuZ2VMb2cJKHdvcmtpbmcgY29weSkKQEAgLTEsMyArMSwxNSBAQAorMjAxMS0x MC0yMSAgRGVlcGFrIFNoZXJ2ZWdoYXIgIDxicHd2NjRAbW90b3JvbGEuY29tPgorCisgICAgICAg IFtRdF0gZmFzdC9ldmVudHMvbWVkaWEtZm9jdXMtaW4tc3RhbmRhbG9uZS1tZWRpYS1kb2N1bWVu dC5odG1sIGZhaWxzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD03MDExMAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlIHNob3VsZCBjaGVjayBmb3Igc3VwcG9ydGVkIG1lZGlhIG1pbWUgdHlwZXMgYXMgd2Vs bCBpbiB0aGUgTUlNRVR5cGVSZWdpc3RyeS4KKworICAgICAgICAqIFdlYkNvcmVTdXBwb3J0L0Zy YW1lTG9hZGVyQ2xpZW50UXQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RnJhbWVMb2FkZXJDbGll bnRRdDo6Y2FuU2hvd01JTUVUeXBlKTogY2hlY2sgZm9yIHN1cHBvcnRlZCBtZWRpYSBtaW1lIHR5 cGUuCisKIDIwMTEtMTAtMjEgIENzYWJhIE9zenRyb2dvbsOhYyAgPG9zc3lAd2Via2l0Lm9yZz4K IAogICAgICAgICBbUXRdW1dLMl0gVHlwbyBmaXguCkluZGV4OiBTb3VyY2UvV2ViS2l0L3F0L1dl YkNvcmVTdXBwb3J0L0ZyYW1lTG9hZGVyQ2xpZW50UXQuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XZWJLaXQvcXQvV2ViQ29yZVN1cHBvcnQvRnJhbWVMb2FkZXJDbGllbnRRdC5jcHAJKHJldmlz aW9uIDk4MDk0KQorKysgU291cmNlL1dlYktpdC9xdC9XZWJDb3JlU3VwcG9ydC9GcmFtZUxvYWRl ckNsaWVudFF0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjYxLDYgKzY2MSw5IEBAIGJvb2wgRnJh bWVMb2FkZXJDbGllbnRRdDo6Y2FuU2hvd01JTUVUeXAKICAgICBpZiAoTUlNRVR5cGVSZWdpc3Ry eTo6aXNTdXBwb3J0ZWROb25JbWFnZU1JTUVUeXBlKHR5cGUpKQogICAgICAgICByZXR1cm4gdHJ1 ZTsKIAorICAgIGlmIChNSU1FVHlwZVJlZ2lzdHJ5Ojppc1N1cHBvcnRlZE1lZGlhTUlNRVR5cGUo dHlwZSkpCisgICAgICAgIHJldHVybiB0cnVlOworCiAgICAgaWYgKG1fZnJhbWUgJiYgbV9mcmFt ZS0+c2V0dGluZ3MoKSAgJiYgbV9mcmFtZS0+c2V0dGluZ3MoKS0+YXJlUGx1Z2luc0VuYWJsZWQo KQogICAgICAgICAmJiBQbHVnaW5EYXRhYmFzZTo6aW5zdGFsbGVkUGx1Z2lucygpLT5pc01JTUVU eXBlUmVnaXN0ZXJlZCh0eXBlKSkKICAgICAgICAgcmV0dXJuIHRydWU7CkluZGV4OiBMYXlvdXRU ZXN0cy9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCShyZXZp c2lvbiA5ODA5NCkKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAt MSwzICsxLDE0IEBACisyMDExLTEwLTIxICBEZWVwYWsgU2hlcnZlZ2hhciAgPGJwd3Y2NEBtb3Rv cm9sYS5jb20+CisKKyAgICAgICAgW1F0XSBmYXN0L2V2ZW50cy9tZWRpYS1mb2N1cy1pbi1zdGFu ZGFsb25lLW1lZGlhLWRvY3VtZW50Lmh0bWwgZmFpbHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcwMTEwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9C T0RZIChPT1BTISkuCisKKyAgICAgICAgUmVtb3ZlIG1lZGlhLWZvY3VzLWluLXN0YW5kYWxvbmUt bWVkaWEtZG9jdW1lbnQuaHRtbCBmcm9tIFF0IFNraXBwZWQgdGVzdCBsaXN0LgorCisgICAgICAg ICogcGxhdGZvcm0vcXQvU2tpcHBlZDoKKwogMjAxMS0xMC0yMSAgWm9sdGFuIEhvcnZhdGggIDx6 b2x0YW5Ad2Via2l0Lm9yZz4KIAogICAgICAgICBbUXRdIGZhc3QvZXZlbnRzL3RvdWNoL3BhZ2Ut c2NhbGVkLXRvdWNoLWdlc3R1cmUtY2xpY2suaHRtbCBpcyBmYWlsaW5nIApJbmRleDogTGF5b3V0 VGVzdHMvcGxhdGZvcm0vcXQvU2tpcHBlZAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0cy9wbGF0 Zm9ybS9xdC9Ta2lwcGVkCShyZXZpc2lvbiA5ODA5NCkKKysrIExheW91dFRlc3RzL3BsYXRmb3Jt L3F0L1NraXBwZWQJKHdvcmtpbmcgY29weSkKQEAgLTI0NDQsMTAgKzI0NDQsNiBAQCBodHRwL3Rl c3RzL2luc3BlY3Rvci9yZXNvdXJjZS10cmVlL3Jlc291CiBodHRwL3Rlc3RzL2luc3BlY3Rvci9y ZXNvdXJjZS10cmVlL3Jlc291cmNlLXRyZWUtZnJhbWUtbmF2aWdhdGUuaHRtbAogaHR0cC90ZXN0 cy9pbnNwZWN0b3IvcmVzb3VyY2UtdHJlZS9yZXNvdXJjZS10cmVlLW5vbi11bmlxdWUtdXJsLmh0 bWwKIAotIyBbUXRdIGZhc3QvZXZlbnRzL21lZGlhLWZvY3VzLWluLXN0YW5kYWxvbmUtbWVkaWEt ZG9jdW1lbnQuaHRtbCBmYWlscwotIyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NzAxMTAKLWZhc3QvZXZlbnRzL21lZGlhLWZvY3VzLWluLXN0YW5kYWxvbmUtbWVkaWEt ZG9jdW1lbnQuaHRtbAotCiAjIGluc3BlY3Rvci9jb29raWUtcGFyc2VyLmh0bWwgaXMgYSBmbGFr eSBjcmFzaAogIyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjI2NjIK IGluc3BlY3Rvci9jb29raWUtcGFyc2VyLmh0bWwK