51195 2010-12-16 10:16:09 -0800 MediaPlayer should look for MIME type in data: URL 2010-12-23 01:26:05 -0800 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 eric.carlson eric.carlson abarth darin eric jberlin pnormand webkit.review.bot oldest_to_newest 323247 0 eric.carlson 2010-12-16 10:16:09 -0800 MediaPlayer::load should try to get the MIME type from the url when passed a data: url. 323278 1 76786 eric.carlson 2010-12-16 10:52:20 -0800 Created attachment 76786 Proposed patch 323287 2 76786 darin 2010-12-16 10:59:06 -0800 Comment on attachment 76786 Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=76786&action=review Can we make tests for this? > WebCore/platform/graphics/MediaPlayer.cpp:306 > + // If the MIME type is missing or is not meaningful, try to figure it out from the url. > + if (type.isEmpty() && protocolIs(url, "data")) The comment says “is not meaningful”, but the code only checks for empty strings. Aren’t there other non-meaningful MIME types? I suggest capitalizing URL in the comment. I suggest we add protocolIsData to KURL.h. We already have it for actual KURL objects and I’d like to have it for strings. It’s not good to have the actual "data" string in the code. We should also adopt protocolIsData in KURL.cpp, KURLGoogle.cpp, DataURL.cpp, ResourceHandleSoup.cpp. None of that needs to be done right away or by you. 323296 3 eric.carlson 2010-12-16 11:09:30 -0800 (In reply to comment #2) > > Can we make tests for this? > The MIME type is only used internally within MediaPlayer so I couldn't think of a way to create a test. > > WebCore/platform/graphics/MediaPlayer.cpp:306 > > + // If the MIME type is missing or is not meaningful, try to figure it out from the url. > > + if (type.isEmpty() && protocolIs(url, "data")) > > The comment says “is not meaningful”, but the code only checks for empty strings. Aren’t there other non-meaningful MIME types? > Good point, we try to infer the MIME type from the file extension when passed application/octetstream or text/plain, so we should look for type in a data url when passed those types as well. 325689 4 eric.carlson 2010-12-22 14:13:38 -0800 http://trac.webkit.org/changeset/74504 325730 5 jberlin 2010-12-22 15:28:39 -0800 (In reply to comment #4) > http://trac.webkit.org/changeset/74504 I think this change broke media/audio-data-url.html in the Windows 7 Release Tests: http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74504%20(7657)/media/audio-data-url-pretty-diff.html 325736 6 webkit.review.bot 2010-12-22 15:32:42 -0800 http://trac.webkit.org/changeset/74504 might have broken GTK Linux 64-bit Debug The following tests are not passing: media/audio-data-url.html 325869 7 jberlin 2010-12-22 20:02:49 -0800 Bug for the failing Windows 7 Release Tests https://bugs.webkit.org/show_bug.cgi?id=51518 325984 8 pnormand 2010-12-23 01:26:05 -0800 (In reply to comment #6) > http://trac.webkit.org/changeset/74504 might have broken GTK Linux 64-bit Debug > The following tests are not passing: > media/audio-data-url.html https://bugs.webkit.org/show_bug.cgi?id=51525 ... 76786 2010-12-16 10:52:20 -0800 2010-12-16 10:59:05 -0800 Proposed patch patch_1.txt text/plain 1797 eric.carlson SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3NDE5OSkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTggQEAKKzIwMTAtMTItMTYgIEVyaWMgQ2FybHNvbiAgPGVyaWMuY2FybHNvbkBh cHBsZS5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgTWVkaWFQbGF5ZXIgc2hvdWxkIGxvb2sgZm9yIE1JTUUgdHlwZSBpbiBkYXRhOiB1cmwKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTUxMTk1CisKKyAg ICAgICAgSWYgbm8gTUlNRSB0eXBlIGlzIHBhc3NlZCBmcm9tIHRoZSBtZWRpYSBlbmdpbmUgZm9y IGEgZGF0YTogdXJsLCB0cnkgdG8gZ2V0IGl0IGZyb20KKyAgICAgICAgdGhlIHVybCBpdHNlbGYu CisKKyAgICAgICAgTm8gbmV3IHRlc3RzIGFyZSBwb3NzaWJsZSBiZWNhdXNlIHRoZSBNSU1FIHR5 cGUgaXMganVzdCBwYXNzZWQgdG8gdGhlIG1lZGlhIGVuZ2luZS4KKworICAgICAgICAqIHBsYXRm b3JtL2dyYXBoaWNzL01lZGlhUGxheWVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxh eWVyOjpsb2FkKToKKwogMjAxMC0xMi0xNiAgUGF2ZWwgRmVsZG1hbiAgPHBmZWxkbWFuQGNocm9t aXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBZdXJ5IFNlbWlraGF0c2t5LgpJbmRleDog V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9NZWRpYVBsYXllci5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9NZWRpYVBsYXllci5jcHAJKHJldmlzaW9uIDc0MTE4 KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9NZWRpYVBsYXllci5jcHAJKHdvcmtpbmcg Y29weSkKQEAgLTMwMiw4ICszMDIsMTAgQEAgdm9pZCBNZWRpYVBsYXllcjo6bG9hZChjb25zdCBT dHJpbmcmIHVybAogICAgIFN0cmluZyB0eXBlID0gY29udGVudFR5cGUudHlwZSgpLmxvd2VyKCk7 CiAgICAgU3RyaW5nIHR5cGVDb2RlY3MgPSBjb250ZW50VHlwZS5wYXJhbWV0ZXIoY29kZWNzKCkp OwogCi0gICAgLy8gSWYgdGhlIE1JTUUgdHlwZSBpcyB1bmhlbHBmdWwsIHNlZSBpZiB0aGUgdHlw ZSByZWdpc3RyeSBoYXMgYSBtYXRjaCBmb3IgdGhlIGZpbGUgZXh0ZW5zaW9uLgotICAgIGlmICh0 eXBlLmlzRW1wdHkoKSB8fCB0eXBlID09IGFwcGxpY2F0aW9uT2N0ZXRTdHJlYW0oKSB8fCB0eXBl ID09IHRleHRQbGFpbigpKSB7CisgICAgLy8gSWYgdGhlIE1JTUUgdHlwZSBpcyBtaXNzaW5nIG9y IGlzIG5vdCBtZWFuaW5nZnVsLCB0cnkgdG8gZmlndXJlIGl0IG91dCBmcm9tIHRoZSB1cmwuCisg ICAgaWYgKHR5cGUuaXNFbXB0eSgpICYmIHByb3RvY29sSXModXJsLCAiZGF0YSIpKQorICAgICAg ICB0eXBlID0gbWltZVR5cGVGcm9tRGF0YVVSTCh1cmwpOworICAgIGVsc2UgaWYgKHR5cGUuaXNF bXB0eSgpIHx8IHR5cGUgPT0gYXBwbGljYXRpb25PY3RldFN0cmVhbSgpIHx8IHR5cGUgPT0gdGV4 dFBsYWluKCkpIHsKICAgICAgICAgc2l6ZV90IHBvcyA9IHVybC5yZXZlcnNlRmluZCgnLicpOwog ICAgICAgICBpZiAocG9zICE9IG5vdEZvdW5kKSB7CiAgICAgICAgICAgICBTdHJpbmcgZXh0ZW5z aW9uID0gdXJsLnN1YnN0cmluZyhwb3MgKyAxKTsK