The codec parameter in MediaPlayerPrivateQt::supportsType is not parsed before being passed to the QMediaPlayer::hasSupport function which takes a QStringList. See below: MediaPlayer::SupportsType MediaPlayerPrivateQt::supportsType(const String& mime, const String& codec) ... int QtMediaPlayerSupport = QMediaPlayer::hasSupport(mime, QStringList(codec)); I will be uploading a patch soon.
Created attachment 88930 [details] Patch to parse codec string The codec parameter in MediaPlayerPrivateQt::supportsType was not parsing the 'codec' QString into a QStringList which is required for the QMediaPlayer::hasSupport function. This patch parses and trims the codec parameter into a QStringList.
Comment on attachment 88930 [details] Patch to parse codec string Do you need to unskip a test or update results?
(In reply to comment #2) > (From update of attachment 88930 [details]) > Do you need to unskip a test or update results? Eric, My goal is to get the video-can-play-type.html test to pass on Qt. See this bug: https://bugs.webkit.org/show_bug.cgi?id=42094 But during my investigations I noticed general issues in this area with parsing, etc. This bug is one of them. These are the others: Misuse of "e: https://bugs.webkit.org/show_bug.cgi?id=57728 (misue of ") Improper parsing of codecs param: https://bugs.webkit.org/show_bug.cgi?id=53275 I'm trying to fix the issues incrementally. Right now, I can't pass video-can-play-type.html for Qt because there are some other issues in the return type (either in Qt Multimedia or QtWebkit - I'm still investigating) Maybe I should make this error a dependency on 42094? I'm open to other suggestions what is the proper thing to do.
Comment on attachment 88930 [details] Patch to parse codec string View in context: https://bugs.webkit.org/attachment.cgi?id=88930&action=review Review express ... > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:87 > + // Parse and trim codecs Missing dot. > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:89 > + QString codecStr = codec; Why do you need to copy the string? > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:90 > + codecList = codecStr.split(QLatin1String(","), QString::SkipEmptyParts); That should be a QLatin1Char(','). Why do you create the empty QStringList just to assign it another value 2 lines later? > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:92 > + foreach (const QString &codecStrNotTrimmed, codecList) { Coding style.
Created attachment 89030 [details] Modified patch based on Benjamin's comments I modified the patch based on Benjamin's comments. I believe I addressed all comments except: 1. Why do you need to copy the string? I needed to created a QString from a String so that I could use the "split" function. 2. Coding style. I ran the "check-webkit-style" script and saw no style errors. If you could be more specific I will fix it.
Comment on attachment 89030 [details] Modified patch based on Benjamin's comments View in context: https://bugs.webkit.org/attachment.cgi?id=89030&action=review I am ok with the change. It is not very solid but hey, that is the way QMediaPlayer is defined. Just clean the patch before it can land. > Source/WebCore/ChangeLog:9 > + a QStringList. This change parses and trims the list. Two spaces after the dot. > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:91 > + foreach (const QString &codecStrNotTrimmed, codecList) { coding style: const QString& codecStrNotTrimmed. > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:93 > + codecListTrimmed.append(codecStrTrimmed); Couldn't you have an empty string here. If codecs = "foo, , bar, ". The parts containing only spaces will be null string after trimmed() and will be inserted in the list.
Created attachment 89200 [details] Modified patch based on Benjamin's comments Modified patch based on Benjamin's comments. Added a test for a null string in the codecs paramter based on Benjamin's comments.
Created attachment 89229 [details] Add expected results
Comment on attachment 89229 [details] Add expected results lgtm
The commit-queue encountered the following flaky tests while processing attachment 89229 [details]: http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: alice.liu@apple.com) The commit-queue is continuing to process your patch.
Comment on attachment 89229 [details] Add expected results Clearing flags on attachment: 89229 Committed r83836: <http://trac.webkit.org/changeset/83836>
All reviewed patches have been landed. Closing bug.